Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for off-thread compilation of scripts #27365

Merged
merged 2 commits into from Jul 23, 2020
Merged

Fixes for off-thread compilation of scripts #27365

merged 2 commits into from Jul 23, 2020
+43 −44

Conversation

@jdm
Copy link
Member

jdm commented Jul 22, 2020

Fixes #27355. Fixes #27349.
@jdm
Pass C string pointer to CompileOptionsWrapper.
a271ed9
@highfive
Copy link

highfive commented Jul 22, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/globalscope.rs, components/script/dom/htmlscriptelement.rs
  • @KiChjang: components/script/dom/globalscope.rs, components/script/dom/htmlscriptelement.rs
@highfive
Copy link

highfive commented Jul 22, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm jdm changed the title Offthread fixes Fixes for off-thread compilation of scripts Jul 22, 2020
@jdm
Copy link
Member Author

jdm commented Jul 22, 2020

r? @CYBAI
@highfive highfive assigned CYBAI and unassigned asajeffrey Jul 22, 2020
@CYBAI
CYBAI approved these changes Jul 22, 2020
View changes
Copy link
Collaborator

CYBAI left a comment •
edited

Thanks! This all looks good to me! Only one question!

(Btw, the patch for CString reminds me that I did a redundant as_bytes for transforming a string into CString in script_module 😂)

Edit: as a reviewer, I should say r=me 😆
components/script/dom/globalscope.rs
);
}

let result = JS_ExecuteScript(*cx, compiled_script.handle(), rval);

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Jul 22, 2020

Collaborator

Because of the early return in SourceCode::Text branch above and also the null check for returning a network error in off_thread_compilation_callback, we should always get a non-null compiled_script.

I'm wondering, would it be worth adding an assert before executing the script 👀?

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jul 22, 2020

Author Member

Sounds reasonable!
@jdm
Ensure scripts compiled off-thread can import modules.
Loading status checks…
e3f0989
@jdm jdm force-pushed the jdm:offthread-fixes branch from 41a24f1 to e3f0989 Jul 22, 2020
@jdm
Copy link
Member Author

jdm commented Jul 22, 2020

@bors-servo r=cybai
@bors-servo
Copy link
Contributor

bors-servo commented Jul 22, 2020

📌 Commit e3f0989 has been approved by cybai
bors-servo added a commit that referenced this pull request Jul 22, 2020
@bors-servo
Auto merge of #27365 - jdm:offthread-fixes, r=cybai
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
95afc0e 
Fixes for off-thread compilation of scripts

Fixes #27355. Fixes #27349.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 22, 2020

⌛ Testing commit e3f0989 with merge 95afc0e...
@bors-servo
Copy link
Contributor

bors-servo commented Jul 22, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member Author

jdm commented Jul 22, 2020

@bors-servo retry
bors-servo added a commit that referenced this pull request Jul 22, 2020
@bors-servo
Auto merge of #27365 - jdm:offthread-fixes, r=cybai
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
6fc2eb1 
Fixes for off-thread compilation of scripts

Fixes #27355. Fixes #27349.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 22, 2020

⌛ Testing commit e3f0989 with merge 6fc2eb1...
@bors-servo
Copy link
Contributor

bors-servo commented Jul 22, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member Author

jdm commented Jul 22, 2020

@bors-servo retry
bors-servo added a commit that referenced this pull request Jul 22, 2020
@bors-servo
Auto merge of #27365 - jdm:offthread-fixes, r=cybai
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
7214afe 
Fixes for off-thread compilation of scripts

Fixes #27355. Fixes #27349.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 22, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member Author

jdm commented Jul 22, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jul 22, 2020

⌛ Testing commit e3f0989 with merge a16130d...
bors-servo added a commit that referenced this pull request Jul 22, 2020
@bors-servo
Auto merge of #27365 - jdm:offthread-fixes, r=cybai
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a16130d 
Fixes for off-thread compilation of scripts

Fixes #27355. Fixes #27349.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 22, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member Author

jdm commented Jul 23, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2020

⌛ Testing commit e3f0989 with merge 63c429b...
bors-servo added a commit that referenced this pull request Jul 23, 2020
@bors-servo
Auto merge of #27365 - jdm:offthread-fixes, r=cybai
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
63c429b 
Fixes for off-thread compilation of scripts

Fixes #27355. Fixes #27349.
@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member Author

jdm commented Jul 23, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2020

⌛ Testing commit e3f0989 with merge 7b01d40...
@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2020

☀️ Test successful - status-taskcluster
Approved by: cybai
Pushing 7b01d40 to master...
@bors-servo bors-servo merged commit 7b01d40 into servo:master Jul 23, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CYBAI CYBAI

Assignees

@CYBAI CYBAI

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Non-null terminated strings are being passed to CompileOptionsWrapper::new
Off-thread compiled scripts don't have dynamic import integration
5 participants
@jdm @highfive @bors-servo @CYBAI @asajeffrey
You can’t perform that action at this time.