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

Fix building with --feature js_backtrace #27042

Merged
merged 1 commit into from Jun 23, 2020

Conversation

@KallynGowdy
Copy link
Contributor

KallynGowdy commented Jun 22, 2020

This PR fixes an build error that I ran into when using the --features js_backtrace flag.

In particular, the error I ran into was this:

error[E0061]: this function takes 2 arguments but 1 argument was supplied
   --> components\script\dom\bindings\error.rs:113:45
    |
113 |         let js_stack = stack.and_then(|s| s.as_string(None));
    |                                             ^^^^^^^^^ ---- supplied 1 argument
    |                                             |
    |                                             expected 2 arguments

It seems that the mozjs crate updated the as_string() method on CapturedJSStack to take two parameters. I've chosen to use StackFormat::Default because that will presumably preserve the original intended behavior.

I additionally had to make the block wrapping the feature unsafe as I ran into a "must be marked as unsafe to call unsafe code" build error after fixing the above issue. Seems that this commit (0703a1a) forgot that part.

Other Notes

After these changes, I was able to build but ran into an access violation error during runtime. When a JS error hit throw_dom_exception() and the JS stack was pulled, some sort of issue was hit inside mozjs and MOZ_NoReturn() was called, which will obviously cause an issue.

It is worth noting that this only happened on my test platform (Hololens 2) and not on MacOS or in the Hololens 2 emulator. Additionally, the debug logs were not particularly helpful at identifying the culprit. Maybe it is worth making a full issue for?

Here's the full details:

Error message:

Exception thrown at 0x00007FFBCDD16784 (simpleservo.dll) in ServoApp.exe: 0xC0000005: Access violation writing location 0x0000000000000000.

Stack Trace:

simpleservo.dll!MOZ_NoReturn(int aLine) Line 236
	at E:\Projects\Yeti\servo\target\aarch64-uwp-windows-msvc\debug\build\mozjs_sys-aa48eb6c20c205d4\out\build\dist\include\mozilla\Assertions.h(236)
simpleservo.dll!js::SavedStacks::insertFrames(JSContext * cx, JS::MutableHandle<js::SavedFrame *> frame, mozilla::Variant<JS::AllFrames,JS::MaxFrames,JS::FirstSubsumedFrame> && capture) Line 0
	at E:\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\9a6d8fc\mozjs\js\src\vm\SavedStacks.cpp(0)
simpleservo.dll!js::SavedStacks::saveCurrentStack(JSContext * cx, JS::MutableHandle<js::SavedFrame *> frame, mozilla::Variant<JS::AllFrames,JS::MaxFrames,JS::FirstSubsumedFrame> && capture) Line 1292
	at E:\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\9a6d8fc\mozjs\js\src\vm\SavedStacks.cpp(1292)
simpleservo.dll!JS::CaptureCurrentStack(JSContext * cx, JS::MutableHandle<JSObject *> stackp, mozilla::Variant<JS::AllFrames,JS::MaxFrames,JS::FirstSubsumedFrame> && capture) Line 5926
	at E:\.cargo\git\checkouts\mozjs-fa11ffc7d4f1cc2d\9a6d8fc\mozjs\js\src\jsapi.cpp(5926)
simpleservo.dll!mozjs::rust::CapturedJSStack::new(mozjs::rust::RootedGuard<mut mozjs_sys::generated::root::JSObject*> cx, core::option::Option<u32> guard) Line 1337
	at E:\.cargo\git\checkouts\rust-mozjs-8611526964119dd6\28248e1\src\rust.rs(1337)
simpleservo.dll!script::dom::bindings::error::throw_dom_exception(script::script_runtime::JSContext cx, script::dom::globalscope::GlobalScope * global, script::dom::bindings::error::Error result) Line 114
	at E:\Projects\Yeti\servo\components\script\dom\bindings\error.rs(114)

Repro:

  1. Make a build on Windows for the Hololens 2.
    • This is the command I used: C:\Python27\python.exe mach build -d --uwp --win-arm64 --features js_backtrace
  2. Open the project in Visual Studio.
  3. Change the default URL in DefaultUrl.h to "http://yeticgi.casualos.com/?story=test1&pagePortal=home".
    • Note the lack of HTTPS. There's a separate issue that causes a websocket error. Was trying to debug when I ran into this issue.
    • The same issue occurs if you load from a build of this this repository. Follow the instructions in DEVELOPERS.md if you do.
  4. Run on the Hololens 2 via Visual Studio.
    • If the default URL doesn't take effect, you may have to uninstall the app from the Hololens 2 and reinstall.
  5. Wait for it to load and stop at a breakpoint for SavedStacks.cpp.
    • It will say the source cannot be found but its actually because the breakpoint doesn't have a related line in the source code.
    • If you continue then it will run into the access violation exception.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes OR
  • These changes do not require tests because they fix a build error.
- mozjs::jsapi::CapturedJSStack::as_string() (https://doc.servo.org/mozjs/rust/struct.CapturedJSStack.html#method.as_string) was updated to accept a second parameter which specifies which Stacktrace format to use. The default has been specified to preserve the (presumably) original behavior.
- The related code has also been placed in an unsafe block since the capture_stack macro calling the unsafe mozjs::jsapi::CapturedJSStack::new() function. Seems that this commit (0703a1a) forgot this part.
@highfive
Copy link

highfive commented Jun 22, 2020

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

@highfive
Copy link

highfive commented Jun 22, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/error.rs
  • @KiChjang: components/script/dom/bindings/error.rs
@highfive
Copy link

highfive commented Jun 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
Copy link
Member

jdm commented Jun 22, 2020

Please file an issue about the crash! That's very interesting.

@jdm
Copy link
Member

jdm commented Jun 23, 2020

@bors-servo r+
Thanks for fixing this! I keep doing it locally while investigating other things and forgetting to make a pull request.

@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2020

📌 Commit 6a5a77a has been approved by jdm

@highfive highfive assigned jdm and unassigned asajeffrey Jun 23, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2020

Testing commit 6a5a77a with merge 0916ae3...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 0916ae3 to master...

@bors-servo bors-servo merged commit 0916ae3 into servo:master Jun 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.