Bug 232387 - Callback functions / interfaces should use global object of its _value_ for errors and lifecycle
Summary: Callback functions / interfaces should use global object of its _value_ for e...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on: 240280 233043 233065
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-27 10:11 PDT by Alexey Shvayka
Modified: 2022-05-10 15:46 PDT (History)
21 users (show)

See Also:


Attachments
Patch (40.89 KB, patch)
2021-10-27 10:12 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch for landing (105.31 KB, patch)
2021-11-11 08:54 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (115.03 KB, patch)
2021-11-12 13:47 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (107.99 KB, patch)
2022-01-12 18:50 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (110.02 KB, patch)
2022-01-13 14:43 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (164.19 KB, patch)
2022-01-18 20:53 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (168.54 KB, patch)
2022-01-19 19:20 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2021-10-27 10:11:07 PDT
JSCallbackData should use lexical global object for errors and lifecycle
Comment 1 Alexey Shvayka 2021-10-27 10:12:07 PDT
Created attachment 442603 [details]
Patch
Comment 2 EWS Watchlist 2021-10-27 10:13:27 PDT
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 3 Geoffrey Garen 2021-10-27 10:46:14 PDT
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().
Comment 4 Radar WebKit Bug Importer 2021-11-03 10:12:17 PDT
<rdar://problem/84979685>
Comment 5 Alexey Shvayka 2021-11-11 08:54:04 PST
Created attachment 443952 [details]
Patch for landing
Comment 6 Alexey Shvayka 2021-11-11 08:54:20 PST
(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.
Comment 7 Alexey Shvayka 2021-11-11 10:29:20 PST
Committed r285639 (244140@main): <https://commits.webkit.org/244140@main>
Comment 8 Chris Dumez 2021-11-12 08:13:45 PST
This caused https://bugs.webkit.org/show_bug.cgi?id=233043.
Comment 9 WebKit Commit Bot 2021-11-12 13:19:45 PST
Re-opened since this is blocked by bug 233065
Comment 10 Alexey Shvayka 2021-11-12 13:47:18 PST
Created attachment 444108 [details]
Patch

Don't pass testRunner.setGeolocationPermission directly as geo error callback to test if that caused crashes.
Comment 11 Chris Dumez 2022-01-12 15:58:15 PST
Any update on this?

It looks like this was landed, then reverted then never re-landed?
However, the test did make it upstream.
Comment 12 Alexey Shvayka 2022-01-12 18:50:33 PST
Created attachment 449018 [details]
Patch

Add a null check to setGeolocationPermissionCallback().
Comment 13 Alexey Shvayka 2022-01-12 18:51:22 PST
(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 14 Chris Dumez 2022-01-13 10:30:23 PST
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.
Comment 15 Alexey Shvayka 2022-01-13 14:43:29 PST
Created attachment 449115 [details]
Patch

Check if a promise, returned from WebLockGrantedCallback, is suspended.
Comment 16 Alexey Shvayka 2022-01-13 14:54:02 PST
(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.
Comment 17 Alexey Shvayka 2022-01-18 20:53:50 PST
Created attachment 449463 [details]
Patch

Use global object of _value_, remove [SkipCallbackInvokeCheck] extended attribute, and add thorough test coverage.
Comment 18 Alexey Shvayka 2022-01-19 05:28:31 PST
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.
Comment 19 Alexey Shvayka 2022-01-19 05:52:43 PST
Committed r288197 (?): <https://commits.webkit.org/r288197>
Comment 20 Robert Jenner 2022-01-19 12:22:05 PST
Reverted r288197 for reason:

Broke a test, slowing down EWS

Committed r288229 (246187@trunk): <https://commits.webkit.org/246187@trunk>
Comment 21 Alexey Shvayka 2022-01-19 19:20:10 PST
Created attachment 449543 [details]
Patch

Fix IntersectionObserver not to rely on m_callback's context for timestamp.
Comment 22 Alexey Shvayka 2022-01-20 10:43:35 PST
Committed r288307 (?): <https://commits.webkit.org/r288307>
Comment 23 Alexey Shvayka 2022-01-28 12:00:51 PST
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!
Comment 24 Chris Dumez 2022-05-10 15:46:14 PDT
Reopening since this got reverted in https://commits.webkit.org/250452@main.