Skip to content
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

Introduce dynamic module #27026

Merged
merged 7 commits into from Jul 19, 2020
Merged

Introduce dynamic module #27026

merged 7 commits into from Jul 19, 2020

Conversation

CYBAI
Copy link
Member

@CYBAI CYBAI commented Jun 22, 2020


@highfive
Copy link

highfive commented Jun 22, 2020

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
Copy link

highfive commented Jun 22, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@CYBAI
Copy link
Member Author

CYBAI commented Jun 22, 2020

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

@bors-servo
Copy link
Contributor

bors-servo commented Jun 27, 2020

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

@CYBAI
Copy link
Member Author

CYBAI commented Jul 4, 2020

@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

image

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
Copy link
Collaborator

kunalmohan commented Jul 4, 2020

Awesome! Thank you @CYBAI!!

I can't see the red triangle though

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

@CYBAI
Copy link
Member Author

CYBAI commented Jul 4, 2020

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
Copy link
Collaborator

kunalmohan commented Jul 4, 2020

cc @kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jul 7, 2020

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

@kvark
Copy link
Member

kvark commented Jul 7, 2020

@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
Copy link
Member Author

CYBAI commented Jul 11, 2020

@bors-servo try=wpt

  • Show me if the compile then execute breaks anything!

@bors-servo
Copy link
Contributor

bors-servo commented Jul 11, 2020

Trying commit 84efc1e with merge 52aa3f4...

bors-servo added a commit that referenced this issue Jul 11, 2020
Introduce dynamic module

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 11, 2020

💔 Test failed - status-taskcluster

@CYBAI
Copy link
Member Author

CYBAI commented Jul 11, 2020

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

@jdm
Copy link
Member

jdm commented Jul 17, 2020

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
Copy link
Member

jdm commented Jul 17, 2020

I can reproduce the crash consistently.

@jdm
Copy link
Member

jdm commented Jul 17, 2020

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
Copy link
Member Author

CYBAI commented Jul 18, 2020

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

@jdm
Copy link
Member

jdm commented Jul 18, 2020

Ugh, you're absolutely right. Great catch!

@jdm
Copy link
Member

jdm commented Jul 18, 2020

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
Copy link
Member Author

CYBAI commented Jul 18, 2020

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!

CYBAI added a commit to CYBAI/rust-mozjs that referenced this issue Jul 18, 2020
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.
CYBAI added a commit to CYBAI/rust-mozjs that referenced this issue Jul 18, 2020
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.
CYBAI added a commit to CYBAI/rust-mozjs that referenced this issue Jul 19, 2020
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.
bors-servo added a commit to servo/rust-mozjs that referenced this issue Jul 19, 2020
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
Copy link
Member Author

CYBAI commented Jul 19, 2020

@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
Copy link
Contributor

bors-servo commented Jul 19, 2020

Trying commit 419cd53 with merge a9cb737...

bors-servo added a commit that referenced this issue Jul 19, 2020
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
Copy link
Contributor

bors-servo commented Jul 19, 2020

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

@jdm
Copy link
Member

jdm commented Jul 19, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2020

📌 Commit 419cd53 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2020

Testing commit 419cd53 with merge 086556e...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2020

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

@bors-servo bors-servo merged commit 086556e into servo:master Jul 19, 2020
2 checks passed
@CYBAI CYBAI deleted the dynamic-module branch Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants