Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign uptest-background-fetch-permission.html panics with "assertion failed: !JS_IsExceptionPending(cx)" when rejecting a promise #23645
Comments
|
This looks like this occurs because Permissions::create_descriptor calls PermissionDescriptor::new on the input permission descriptor JS object, which returns an Err value indicating that there is a new JS exception that has been raised as a result of the attempted conversion. We return an Error::Type from Permissions::create_descriptor as a result, which upon converting to a JSVal attempts to set a JS exception and panics because one is already present. |
|
I think we're not following step 1 of https://w3c.github.io/permissions/#dom-permissions-query correctly - it says "If this throws an exception, return a promise rejected with that exception and abort these steps", but we're discarding the actual exception value raised. There are a couple steps to fixing this:
This should give us much more sensible behaviour. |
|
Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the If you intend to work on this issue, then add |
… r=<try> Move tinyfiledialog call from script to embedder <!-- Please describe your changes on the following line: --> Moves the tinyfiledialog call from script to embedder. Factored out permission_state_invocation_results API to GlobalScope Instead of Window. I have implemented this for `ports/glutin/browser.rs` is there other ports that use this? --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #23057 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because: I have tested it manually and it seems to work as expected. The current relevant tests seem to fail due to #23645 <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23651) <!-- Reviewable:end -->
|
hey, would like to take this on! I see the thanks! |
|
@julientregoat Absolutely! Let me know if the description of how to reproduce and solve the problem is unclear at all! |
|
okay awesome, sounds clear enough! @highfive: assign me here we go! to the first of many, hopefully |
|
Hey @julientregoat! Thanks for your interest in working on this issue. It's now assigned to you! |
Run DOM permissions tests in WPT. We have an implementation, so we should be testing it to catch problems like #23645. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23647) <!-- Reviewable:end -->
|
hey @jdm! So I fixed the panic and the browser is loading the test results. Is that the entire scope of this issue, or am I meant to get the test passing as well? |
|
Nope, that was the scope! |
Run DOM permissions tests in WPT. We have an implementation, so we should be testing it to catch problems like #23645. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23647) <!-- Reviewable:end -->
Fix panic when running test-background-fetch-permission.html <!-- Please describe your changes on the following line: --> * Propagate `Error::JSFailed` in `Permissions::create_descriptor` * In `Error::to_jsval`, only check if a JS exception is not pending if `self` is not `Error::JSFailed` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23645 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [x] These changes do not require tests because it's a test runner, so by nature if the test runs the test runner is also working as expected. <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23654) <!-- Reviewable:end -->
Fix panic when running test-background-fetch-permission.html <!-- Please describe your changes on the following line: --> * Propagate `Error::JSFailed` in `Permissions::create_descriptor` * In `Error::to_jsval`, only check if a JS exception is not pending if `self` is not `Error::JSFailed` --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23645 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [x] These changes do not require tests because it's a test runner, so by nature if the test runs the test runner is also working as expected. <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23654) <!-- Reviewable:end -->
To reproduce, run
./mach test-wpt /permissions/test-background-fetch-permission.html --pref dom.permissions.enabled.Stack trace: