JSCallbackData should use lexical global object for errors and lifecycle
Created attachment 442603 [details]
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]
View in context: https://bugs.webkit.org/attachment.cgi?id=442603&action=review
r=me -- but please fix the visit issue before landing.
> + 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]
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]
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]
View in context: https://bugs.webkit.org/attachment.cgi?id=449018&action=review
> + 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]
Check if a promise, returned from WebLockGrantedCallback, is suspended.
(In reply to Chris Dumez from comment #14)
> Comment on attachment 449018 [details]
> View in context:
> > 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]
Use global object of _value_, remove [SkipCallbackInvokeCheck] extended attribute, and add thorough test coverage.
Comment on attachment 449463 [details]
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]
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.