Introduce dynamic module by CYBAI · Pull Request #27026 · servo/servo

CYBAI · 2020-06-22T11:28:13Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Support dynamic-import of module script #25439
There are tests for these changes

highfive · 2020-06-22T11:28:18Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmlscriptelement.rs, components/script/dom/servoparser/prefetch.rs, components/script/script_module.rs, components/script/dom/globalscope.rs, components/script/dom/bindings/trace.rs
@jgraham: tests/wpt/include.ini
@KiChjang: components/script/dom/htmlscriptelement.rs, components/script/dom/servoparser/prefetch.rs, components/script/script_module.rs, components/script/dom/globalscope.rs, components/script/dom/bindings/trace.rs

highfive · 2020-06-22T11:28:20Z

Warning

These commits modify unsafe code. Please review it carefully!

CYBAI · 2020-06-22T11:29:08Z

This is still WIP so just removed assignee and awaiting review label 🙏

components/script/script_module.rs

bors-servo · 2020-06-27T19:33:59Z

☔ The latest upstream changes (presumably #27084) made this pull request unmergeable. Please resolve the merge conflicts.

CYBAI · 2020-07-04T09:18:24Z

@jdm @kunalmohan Good news here!

About WebGPU

In current dynamic-module branch, I can't see the red triangle though, the https://austineng.github.io/webgpu-samples can be loaded successfully! (I remember I even can't scroll in previous impl) and if I click in the code block (which looks like a squashed code block), then I will get window.focus is not a function error.

→ ./mach run --pref=dom.webgpu.enabled https://austineng.github.io/webgpu-samples/\#helloTriangle
[2020-07-04T09:01:50Z ERROR script::dom::bindings::error] Error at https://austineng.github.io/webgpu-samples/dist/common-606af6.js:1:197397 window.focus is not a function
[2020-07-04T09:01:53Z ERROR script::dom::bindings::error] Error at https://austineng.github.io/webgpu-samples/dist/common-606af6.js:1:197397 window.focus is not a function

About Dynamic Import

I'm not sure if it's a good idea but I wonder it should be safer and easier to generate the DynamicModuleOwner from codegen? With using a codegen-ed version DynamicModuleOwner, codegen has already handled how to GC for us so we can just use it in script_module! The DynamicModuleOwner webidl is introduced in cc2a9e7 commit.

Then, with simply sending the IPC router to resource threads by global.resource_threads().sender() in f2e73b6, it has already fixed most of TIMEOUT!

But, there are still some unexpected FAIL and TIMEOUT! I'm going to observe the root cause for remaining FAIL and TIMEOUT!

Hope current branch can work fine for the webgpu project! 🙏

kunalmohan · 2020-07-04T09:45:00Z

Awesome! Thank you @CYBAI!!

I can't see the red triangle though

I wonder why that happens 🤔 . I'll try to investigate this.

CYBAI · 2020-07-04T09:49:12Z

I can't see the red triangle though

I wonder why that happens 🤔 . I'll try to investigate this.

It looks like the module is loaded but if you think it might be something wrong in dynamic module, feel free to let me know! 😄

kunalmohan · 2020-07-04T14:48:11Z

cc @kvark

bors-servo · 2020-07-07T05:48:54Z

☔ The latest upstream changes (presumably #27088) made this pull request unmergeable. Please resolve the merge conflicts.

kvark · 2020-07-07T17:33:45Z

@kunalmohan if you get stuck investigating this, please consider exposing a way for Servo to record the wgpu API trace. We'd be able to look at it outside of Servo and see if it captures the problem.

CYBAI · 2020-07-11T13:55:53Z

@bors-servo try=wpt

Show me if the compile then execute breaks anything!

bors-servo · 2020-07-11T13:55:57Z

⌛ Trying commit 84efc1e with merge 52aa3f4...

Introduce dynamic module  ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

...scripting-1/the-script-element/module/dynamic-import/no-active-script-manual-module.html.ini

bors-servo · 2020-07-11T14:34:40Z

💔 Test failed - status-taskcluster

CYBAI · 2020-07-11T14:48:12Z

Wow, many unexpected CRASH and ERROR; looks like I do break something 😂 Let me investigate it!

jdm · 2020-07-17T19:56:32Z

Huh, #27311 hit us again. I'm getting suspicious, so I'll try building this branch on my Windows machine and seeing if I can reproduce it.

jdm · 2020-07-17T20:39:07Z

I can reproduce the crash consistently.

jdm · 2020-07-17T20:45:13Z

Somehow, we're calling GetScriptPrivate on a null JSScript, despite checking for a null pointer first. That's... confusing.

servo!js::BaseScript::sourceObject+0x3
servo!JS::GetScriptPrivate+0x3
servo!script::dom::globalscope::{{impl}}::evaluate_script_on_global_with_result::{{closure}}+0x5f4
servo!profile_traits::time::profile<bool,closure-0>+0x13f
servo!script::dom::globalscope::GlobalScope::evaluate_script_on_global_with_result+0x222
servo!script::dom::htmlscriptelement::HTMLScriptElement::run_a_classic_script+0x383
servo!script::dom::htmlscriptelement::HTMLScriptElement::execute+0x717
servo!script::dom::htmlscriptelement::HTMLScriptElement::prepare+0x1d73
servo!script::dom::servoparser::ServoParser::tokenize<closure-0>+0x244
servo!script::dom::servoparser::ServoParser::do_parse_sync+0x289
servo!script::dom::servoparser::{{impl}}::parse_sync::{{closure}}+0x16
servo!profile_traits::time::profile<(),closure-0>+0xf7
servo!script::dom::servoparser::ServoParser::parse_sync+0x1ab
servo!script::dom::servoparser::ServoParser::parse_bytes_chunk+0xa9
servo!script::dom::servoparser::{{impl}}::process_response_chunk+0x11e
servo!script::script_thread::ScriptThread::handle_fetch_chunk+0x10b
servo!script::script_thread::ScriptThread::handle_msg_from_constellation+0x2b8
servo!script::script_thread::{{impl}}::handle_msgs::{{closure}}+0x13a
servo!script::script_thread::ScriptThread::profile_event<closure-5,core::option::Option<bool>>+0xcf
servo!script::script_thread::ScriptThread::handle_msgs+0x2507

CYBAI · 2020-07-18T01:55:34Z

Hmm, is it possible that it means we might have a bug in GetScriptPrivate on Windows 🤔?

jdm · 2020-07-18T02:14:55Z

Ugh, you're absolutely right. Great catch!

jdm · 2020-07-18T02:16:42Z

More specifically, it's our old nemesis "can't return JSValue by value on Windows". We'll need to add a method to jsapi::glue that takes a *mut JSValue like https://github.com/servo/rust-mozjs/blob/0caf5549cddbb34ad16abf35fb6bfbff824a4d14/src/jsglue.cpp#L1095-L1098.

CYBAI · 2020-07-18T02:53:58Z

More specifically, it's our old nemesis "can't return JSValue by value on Windows".

Oh, TIL 🤯 Thanks!

We'll need to add a method to jsapi::glue that takes a *mut JSValue like https://github.com/servo/rust-mozjs/blob/0caf5549cddbb34ad16abf35fb6bfbff824a4d14/src/jsglue.cpp#L1095-L1098.

Thanks! Let me try to send a PR!

While working on servo/servo#27026, we've noticed calling `GetScriptPrivate` will hit a CRASH consistently in Windows. jdm noticed that this is caused by "can't return JSValue by value on Windows"; thus, we need to expose these 2 glue methods so that we can handle the `GetScriptPrivate` and `GetModulePrivate` correctly.

Add glue methods to get JSValue of a private reference While working on servo/servo#27026, we've noticed calling `GetScriptPrivate` will hit a CRASH consistently in Windows. jdm noticed that this is caused by "can't return JSValue by value on Windows"; thus, we need to expose these 2 glue methods so that we can handle the `GetScriptPrivate` and `GetModulePrivate` correctly. --- This will help servo/servo#27026. We might want to handle the `ModulePrivate` much more correctly in the future so it might be worth adding it here as well?

Because MSVC uses a different calling conventions for functions that return non-POD values, we need to use the new exposed wrapper function so that `GetScriptPrivate` can be handled correctly on Windows.

CYBAI · 2020-07-19T07:28:50Z

@bors-servo try

I confirmed it works fine in my local though, I'd like to see if it also works fine in both Linux and Windows (it should fix the ANGLE smoketest on windows and have no impact to linux)

bors-servo · 2020-07-19T07:28:55Z

⌛ Trying commit 419cd53 with merge a9cb737...

Introduce dynamic module --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #25439 - [x] There are tests for these changes

bors-servo · 2020-07-19T08:30:08Z

☀️ Test successful - status-taskcluster
State: approved= try=True

jdm · 2020-07-19T12:48:23Z

@bors-servo r+

bors-servo · 2020-07-19T12:48:24Z

📌 Commit 419cd53 has been approved by jdm

bors-servo · 2020-07-19T13:29:53Z

⌛ Testing commit 419cd53 with merge 086556e...

bors-servo · 2020-07-19T13:31:10Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 086556e to master...

highfive assigned Manishearth Jun 22, 2020

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 22, 2020

CYBAI unassigned Manishearth Jun 22, 2020

CYBAI removed the S-awaiting-review There is new code that needs to be reviewed. label Jun 22, 2020

CYBAI mentioned this pull request Jun 22, 2020

Support dynamic-import of module script #25439

Closed

jdm reviewed Jun 24, 2020

View reviewed changes

components/script/script_module.rs Show resolved Hide resolved

highfive added the S-needs-rebase There are merge conflict errors. label Jun 27, 2020

CYBAI force-pushed the dynamic-module branch from d5808e0 to f2e73b6 Compare July 4, 2020 08:59

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 4, 2020

CYBAI force-pushed the dynamic-module branch from f2e73b6 to 84efc1e Compare July 11, 2020 13:54

CYBAI removed the S-needs-rebase There are merge conflict errors. label Jul 11, 2020

CYBAI commented Jul 11, 2020

View reviewed changes

...scripting-1/the-script-element/module/dynamic-import/no-active-script-manual-module.html.ini Outdated Show resolved Hide resolved

highfive added the S-tests-failed The changes caused existing tests to fail. label Jul 11, 2020

CYBAI force-pushed the dynamic-module branch from 84efc1e to 959750d Compare July 11, 2020 16:12

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jul 11, 2020

highfive added the S-tests-failed The changes caused existing tests to fail. label Jul 17, 2020

CYBAI mentioned this pull request Jul 18, 2020

Add glue methods to get JSValue of a private reference servo/rust-mozjs#515

Merged

Use JS_GetScriptPrivate to get a mutated return value

419cd53

Because MSVC uses a different calling conventions for functions that return non-POD values, we need to use the new exposed wrapper function so that `GetScriptPrivate` can be handled correctly on Windows.

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jul 19, 2020

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 19, 2020

bors-servo merged commit 086556e into servo:master Jul 19, 2020

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 19, 2020

bors-servo mentioned this pull request Jul 19, 2020

Report exceptions for async script executions to webdriver #27041

Merged

4 tasks

CYBAI deleted the dynamic-module branch July 19, 2020 13:38

Uh oh!

Conversation

CYBAI commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jun 22, 2020

Uh oh!

highfive commented Jun 22, 2020

Uh oh!

CYBAI commented Jun 22, 2020

Uh oh!

Uh oh!

bors-servo commented Jun 27, 2020

Uh oh!

CYBAI commented Jul 4, 2020

About WebGPU

About Dynamic Import

Uh oh!

kunalmohan commented Jul 4, 2020

Uh oh!

CYBAI commented Jul 4, 2020

Uh oh!

kunalmohan commented Jul 4, 2020

Uh oh!

bors-servo commented Jul 7, 2020

Uh oh!

kvark commented Jul 7, 2020

Uh oh!

CYBAI commented Jul 11, 2020

Uh oh!

bors-servo commented Jul 11, 2020

Uh oh!

Uh oh!

bors-servo commented Jul 11, 2020

Uh oh!

CYBAI commented Jul 11, 2020

Uh oh!

jdm commented Jul 17, 2020

Uh oh!

jdm commented Jul 17, 2020

Uh oh!

jdm commented Jul 17, 2020

Uh oh!

CYBAI commented Jul 18, 2020

Uh oh!

jdm commented Jul 18, 2020

Uh oh!

jdm commented Jul 18, 2020

Uh oh!

CYBAI commented Jul 18, 2020

Uh oh!

CYBAI commented Jul 19, 2020

Uh oh!

bors-servo commented Jul 19, 2020

Uh oh!

bors-servo commented Jul 19, 2020

Uh oh!

jdm commented Jul 19, 2020

Uh oh!

bors-servo commented Jul 19, 2020

Uh oh!

bors-servo commented Jul 19, 2020

Uh oh!

bors-servo commented Jul 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CYBAI commented Jun 22, 2020 •

edited

Loading