Bug 232387 – Callback functions / interfaces use incorrect global object for liveness and reporting errors

• Status: REOPENED
• Component: Bindings

[Alexey Shvayka]

JSCallbackData should use lexical global object for errors and lifecycle

[Alexey Shvayka]

Created attachment 442603 [details]
Patch

[EWS Watchlist]

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

[Geoffrey Garen]

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().

[Radar WebKit Bug Importer]

<rdar://problem/84979685>

[Alexey Shvayka]

Created attachment 443952 [details]
Patch for landing

[Alexey Shvayka]

(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.

[Alexey Shvayka]

Committed r285639 (244140@main): <https://commits.webkit.org/244140@main>

[Chris Dumez]

This caused https://bugs.webkit.org/show_bug.cgi?id=233043.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 233065

[Alexey Shvayka]

Created attachment 444108 [details]
Patch

Don't pass testRunner.setGeolocationPermission directly as geo error callback to test if that caused crashes.

[Chris Dumez]

Any update on this?

It looks like this was landed, then reverted then never re-landed?
However, the test did make it upstream.

[Alexey Shvayka]

Created attachment 449018 [details]
Patch

Add a null check to setGeolocationPermissionCallback().

[Alexey Shvayka]

(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.

[Chris Dumez]

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.

[Alexey Shvayka]

Created attachment 449115 [details]
Patch

Check if a promise, returned from WebLockGrantedCallback, is suspended.

[Alexey Shvayka]

(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.

[Alexey Shvayka]

Created attachment 449463 [details]
Patch

Use global object of _value_, remove [SkipCallbackInvokeCheck] extended attribute, and add thorough test coverage.

[Alexey Shvayka]

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.

[Alexey Shvayka]

Committed r288197 (?): <https://commits.webkit.org/r288197>

[Robert Jenner]

Reverted r288197 for reason:

Broke a test, slowing down EWS

Committed r288229 (246187@trunk): <https://commits.webkit.org/246187@trunk>

[Alexey Shvayka]

Created attachment 449543 [details]
Patch

Fix IntersectionObserver not to rely on m_callback's context for timestamp.

[Alexey Shvayka]

Committed r288307 (?): <https://commits.webkit.org/r288307>

[Alexey Shvayka]

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!

[Chris Dumez]

Reopening since this got reverted in https://commits.webkit.org/250452@main.

[Ahmad Saleem]

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