Summary: | Callback functions / interfaces use incorrect global object for liveness and reporting errors | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Shvayka <ashvayka> | ||||||||||||||||
Component: | Bindings | Assignee: | Alexey Shvayka <ashvayka> | ||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||
Severity: | Normal | CC: | ahmad.saleem792, benjamin, calvaris, cdumez, clopez, commit-queue, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, hi, jenner, jer.noble, joepeck, kangil.han, kondapallykalyan, mkwst, philipj, sergio, webkit-bug-importer, youennf | ||||||||||||||||
Priority: | P2 | Keywords: | BrowserCompat, InRadar, WPTImpact | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=235153 https://github.com/web-platform-tests/wpt/pull/32449 https://bugs.webkit.org/show_bug.cgi?id=235368 https://bugs.webkit.org/show_bug.cgi?id=235529 https://bugs.webkit.org/show_bug.cgi?id=237912 |
||||||||||||||||||
Bug Depends on: | 240280, 233043, 233065, 248161 | ||||||||||||||||||
Bug Blocks: | 163412 | ||||||||||||||||||
Attachments: |
|
Description
Alexey Shvayka
2021-10-27 10:11:07 PDT
Created attachment 442603 [details]
Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess Comment on attachment 442603 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442603&action=review r=me -- but please fix the visit issue before landing. > Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:75 > + JSValueInWrappedObject m_processCallback; We need to visit this value inside JSAudioWorkletProcessor::visitAdditionalChildren(). Created attachment 443952 [details]
Patch for landing
(In reply to Alexey Shvayka from comment #5) > Created attachment 443952 [details] > Patch for landing 1. Account Geoff's feedback on visitAdditionalChildren(). 2. Add [SkipCallbackInvokeCheck] to Geolocation's error callback as it's run for inactive contexts and revert erroneous test changes. 3. Add a test that ensures NodeFilter with disconnected incumbent object no longer crashes. 4. Adjust bindings tests for [SkipCallbackInvokeCheck] and reset expectations. 5. Revert resolving _incumbent_ in @whenSignalAborted as its `abort` callback can't invoke userland code. Committed r285639 (244140@main): <https://commits.webkit.org/244140@main> This caused https://bugs.webkit.org/show_bug.cgi?id=233043. Re-opened since this is blocked by bug 233065 Created attachment 444108 [details]
Patch
Don't pass testRunner.setGeolocationPermission directly as geo error callback to test if that caused crashes.
Any update on this? It looks like this was landed, then reverted then never re-landed? However, the test did make it upstream. Created attachment 449018 [details]
Patch
Add a null check to setGeolocationPermissionCallback().
(In reply to Chris Dumez from comment #11) > Any update on this? > > It looks like this was landed, then reverted then never re-landed? > However, the test did make it upstream. That's correct, sorry, will re-land once the updated patch passes the tests. Comment on attachment 449018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449018&action=review > Source/WebCore/Modules/geolocation/PositionErrorCallback.idl:27 > + SkipCallbackInvokeCheck, I think this is wrong. We shouldn't be adding new Webkit-specific IDL attributes to get a different Web-facing behavior for one specific API. This doesn't make sense. Either the behavior of SkipCallbackInvokeCheck is the correct one and what we want everywhere, or it is not. Created attachment 449115 [details]
Patch
Check if a promise, returned from WebLockGrantedCallback, is suspended.
(In reply to Chris Dumez from comment #14) > Comment on attachment 449018 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449018&action=review > > > Source/WebCore/Modules/geolocation/PositionErrorCallback.idl:27 > > + SkipCallbackInvokeCheck, > > I think this is wrong. We shouldn't be adding new Webkit-specific IDL > attributes to get a different Web-facing behavior for one specific API. This > doesn't make sense. Either the behavior of SkipCallbackInvokeCheck is the > correct one and what we want everywhere, or it is not. Looking at https://wpt.fyi/results/dom/traversal/TreeWalker-acceptNode-filter-cross-realm-null-browsing-context.html, there might be way we can drop this attribute rather than spreading it to Geolocation. I'm compiling a coverage like dom/traversal/TreeWalker-acceptNode-filter-cross-realm.html for all popular callbacks so we could see the whole web-compat picture. Should not take too long. Created attachment 449463 [details]
Patch
Use global object of _value_, remove [SkipCallbackInvokeCheck] extended attribute, and add thorough test coverage.
Comment on attachment 449463 [details]
Patch
Putting r+ on my own patch, already reviewed by Geoff on 2021-10-27, so the WPT GitHub bot would let me land the tests PR.
Committed r288197 (?): <https://commits.webkit.org/r288197> Reverted r288197 for reason: Broke a test, slowing down EWS Committed r288229 (246187@trunk): <https://commits.webkit.org/246187@trunk> Created attachment 449543 [details]
Patch
Fix IntersectionObserver not to rely on m_callback's context for timestamp.
Committed r288307 (?): <https://commits.webkit.org/r288307> AudioWorklet changes (storing callback w/o a JSC::Strong) caused a regression: https://bugs.webkit.org/show_bug.cgi?id=235529. Thank you Chris for taking care of it! Reopening since this got reverted in https://commits.webkit.org/250452@main. We are failing two WPT Tests: Only failing in Safari - http://wpt.live/dom/traversal/TreeWalker-acceptNode-filter-cross-realm-null-browsing-context.html Failing in Firefox & WebKit - http://wpt.live/dom/traversal/TreeWalker-acceptNode-filter-cross-realm.html |