Bug 163412 - Implement backup incumbent settings object stack and fix _entry_ realm so it's always correct
Summary: Implement backup incumbent settings object stack and fix _entry_ realm so it'...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on: 171145
Blocks: 162075
  Show dependency treegraph
 
Reported: 2016-10-13 15:34 PDT by lee.wang.zhong+wk
Modified: 2022-09-03 06:46 PDT (History)
34 users (show)

See Also:


Attachments
Example (576 bytes, text/html)
2016-12-19 11:21 PST, Daniel Bates
no flags Details
Patch and layout tests (35.70 KB, patch)
2017-03-13 14:24 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (36.15 KB, patch)
2017-03-13 15:11 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (35.88 KB, patch)
2017-04-19 20:36 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-elcapitan (1.86 MB, application/zip)
2017-04-19 22:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (22.50 MB, application/zip)
2017-04-19 23:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (920.18 KB, application/zip)
2017-04-20 00:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-elcapitan (951.72 KB, application/zip)
2017-04-20 03:02 PDT, Build Bot
no flags Details
Patch and layout tests (38.38 KB, patch)
2017-04-20 13:09 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (127.12 KB, patch)
2017-05-08 18:34 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (815.51 KB, application/zip)
2017-05-08 19:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (745.89 KB, application/zip)
2017-05-08 19:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (894.07 KB, application/zip)
2017-05-08 19:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (733.19 KB, application/zip)
2017-05-08 20:05 PDT, Build Bot
no flags Details
Patch and layout tests (132.75 KB, patch)
2017-05-12 12:01 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (956.21 KB, application/zip)
2017-05-12 13:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (893.15 KB, application/zip)
2017-05-12 13:28 PDT, Build Bot
no flags Details
Patch and layout tests (132.70 KB, patch)
2017-05-12 14:13 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (141.32 KB, patch)
2017-05-25 16:41 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (141.49 KB, patch)
2017-05-25 16:56 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
WIP (65.26 KB, patch)
2021-11-03 10:33 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
WIP (102.32 KB, patch)
2021-11-10 14:39 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (98.40 KB, patch)
2021-11-12 09:27 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (140.40 KB, patch)
2022-01-19 10:27 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (143.39 KB, patch)
2022-01-28 14:07 PST, Alexey Shvayka
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (150.23 KB, patch)
2022-01-28 14:14 PST, Alexey Shvayka
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Microbenchmark (157 bytes, application/x-javascript)
2022-01-28 15:02 PST, Alexey Shvayka
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description lee.wang.zhong+wk 2016-10-13 15:34:00 PDT
On iOS 10, a `postMessage` from 

This is a change from iOS 9.
Comment 1 lee.wang.zhong+wk 2016-10-13 15:45:18 PDT
[Sorry, early send.]

On iOS 10 Safari, a `postMessage` from an iframe with the same origin as the parent window results in a MessageEvent with the `.source` property set to the parent window. It should be set to the window of the message source.

This is a behavior change from iOS 9.

P.S.: Because `.source` is scrubbed, if the parent has two child frames with the parent's origin, there's no data on the event object which could distinguish one frame's messages from the other's.
Comment 2 Radar WebKit Bug Importer 2016-10-14 10:15:14 PDT
<rdar://problem/28776024>
Comment 3 Chris Dumez 2016-10-14 10:16:55 PDT
(In reply to comment #1)
> [Sorry, early send.]
> 
> On iOS 10 Safari, a `postMessage` from an iframe with the same origin as the
> parent window results in a MessageEvent with the `.source` property set to
> the parent window. It should be set to the window of the message source.
> 
> This is a behavior change from iOS 9.
> 
> P.S.: Because `.source` is scrubbed, if the parent has two child frames with
> the parent's origin, there's no data on the event object which could
> distinguish one frame's messages from the other's.

Do you have a test case / example?
Comment 4 Daniel Bates 2016-12-19 11:21:58 PST
Created attachment 297469 [details]
Example

Attached page that demonstrates this issue. Running the example in Safari version Version 10.0 (12602.1.50.0.10) I see "Got message from A: hello". In Chrome for Mac version 56.0.2924.28 beta (64-bit) and Firefox for Mac version 49.0.1 I see "Got message from B: hello". The expected result should be the latter, "Got message from B: hello".
Comment 5 Daniel Bates 2017-03-13 14:24:05 PDT
Created attachment 304301 [details]
Patch and layout tests
Comment 6 WebKit Commit Bot 2017-03-13 14:26:07 PDT Comment hidden (obsolete)
Comment 7 Daniel Bates 2017-03-13 15:11:51 PDT
Created attachment 304309 [details]
Patch and layout tests

Add forwarding header file for JSBoundFunction.h.
Comment 8 WebKit Commit Bot 2017-03-13 15:13:14 PDT Comment hidden (obsolete)
Comment 9 Daniel Bates 2017-04-19 20:25:21 PDT
From talking with Geoffrey Garen on or around 03/16/2017 about bug 162075, Geoff felt the implementation of callerDOMWindow() even after the changes made in attachment 304309 [details] has correctness issues with respect to builtin functions and pointed out that we can stop walking the stack once we hit a ScriptExecutable.
Comment 10 Daniel Bates 2017-04-19 20:36:57 PDT
Created attachment 307546 [details]
Patch and layout tests

Modified the implementation of callerDOMWindow() to use a similiar approach as ExecState::callerSourceOrigin(). This work was inspired by a patch by Geoffrey Garen.
Comment 11 Build Bot 2017-04-19 20:39:40 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-04-19 22:29:41 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-04-19 22:29:42 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-04-19 23:27:16 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-04-19 23:27:18 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2017-04-20 00:15:57 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-04-20 00:15:59 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2017-04-20 03:02:36 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2017-04-20 03:02:38 PDT Comment hidden (obsolete)
Comment 20 Daniel Bates 2017-04-20 13:09:03 PDT
Created attachment 307623 [details]
Patch and layout tests

Modified WebCore::callerDOMWindow() to return the first DOM window if did not find non-native code in the stack trace and the currently running code was not scheduled (say, via setTimeout()). This fixes the assertion failure and makes another subtest pass in LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html. This also preserves our current behavior of returng the first DOM window as the calling DOM window should WebCore::callerDOMWindow() be invoked when running Wasm code (see bug #165721).
Comment 21 Build Bot 2017-04-20 13:10:58 PDT Comment hidden (obsolete)
Comment 22 Geoffrey Garen 2017-04-21 14:03:12 PDT
Comment on attachment 307623 [details]
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=307623&action=review

It seems like we have two names for this new kind of global object: "incumbent" and "caller". I think we should try to use "incumbent" in as many places as possible, if that's the spec name.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:306
> +                return StackVisitor::Status::Done;

Seems like it would be better to continue in this case, and treat WASM like bound functions, rather than giving up. Giving up will always result in a less accurate result.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:336
> +        // The currently running code was scheduled to run, say via setTimeout(). Return the window
> +        // associated with the code that scheduled us.

This comment is not the same as the comment in VM.h. I prefer the comment in VM.h. I suggest removing this comment, and if there's something unique you like about it, moving that to VM.h, so there's a single place where we explain the concept of incumbentGlobalObject.

> Source/WebCore/bindings/js/ScheduledAction.cpp:135
> +    vm.setIncumbentGlobalObject(nullptr);

I wonder if you can make this automatic using something like WTF::SetForScope or bmalloc::ScopeExit. This is a strong reference, so it would be nice to have an automatic idiom for clearing it. You could also make a dedicated IncumbentGlobalObjectScope class.
Comment 23 Saam Barati 2017-04-21 15:44:45 PDT
Comment on attachment 307623 [details]
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=307623&action=review

> Source/WebCore/bindings/js/ScheduledAction.cpp:130
> +    vm.setIncumbentGlobalObject(m_callerGlobalObject.get());

I would assert it's nullptr here before setting the value, otherwise, the code below that sets it back to nullptr would be breaking something else.
Comment 24 Saam Barati 2017-04-21 16:00:08 PDT
I think it's also worth ensuring this works for Promises, requestAnimationFrame, etc. (I spoke to Dan in person).
Comment 25 Daniel Bates 2017-05-08 18:27:13 PDT
(In reply to Geoffrey Garen from comment #22)
> Comment on attachment 307623 [details]
> Patch and layout tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307623&action=review
> 
> It seems like we have two names for this new kind of global object:
> "incumbent" and "caller". I think we should try to use "incumbent" in as
> many places as possible, if that's the spec name.
> 

Split off this renaming into bug #171145.

> > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:306
> > +                return StackVisitor::Status::Done;
> 
> Seems like it would be better to continue in this case, and treat WASM like
> bound functions, rather than giving up. Giving up will always result in a
> less accurate result.
> 

Will fix.

> > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:336
> > +        // The currently running code was scheduled to run, say via setTimeout(). Return the window
> > +        // associated with the code that scheduled us.
> 
> This comment is not the same as the comment in VM.h. I prefer the comment in
> VM.h. I suggest removing this comment, and if there's something unique you
> like about it, moving that to VM.h, so there's a single place where we
> explain the concept of incumbentGlobalObject.
> 

I refactored this code and removed the comment.

> > Source/WebCore/bindings/js/ScheduledAction.cpp:135
> > +    vm.setIncumbentGlobalObject(nullptr);
> 
> I wonder if you can make this automatic using something like
> WTF::SetForScope or bmalloc::ScopeExit. This is a strong reference, so it
> would be nice to have an automatic idiom for clearing it. You could also
> make a dedicated IncumbentGlobalObjectScope class.

Will add a RAII object called BackupIncumbentScope to manage this. I chose to be more precise and call this class BackupIncumbentScope instead of IncumbentGlobalObjectScope because the scope is for the backup incumbent global object. The incumbent global object is defined to be the top-most author code global object falling back to the backup incumbent global object for scheduled author code.
Comment 26 Daniel Bates 2017-05-08 18:34:31 PDT
Created attachment 309455 [details]
Patch and layout tests

Updated patch based on feedback from Geoffrey Garen and Saam Barati. With this patch applied we compute the incumbent global object correctly for all WebIDL callbacks and promise callbacks, including requestAnimationFrame(). I also added more tests to cover these cases.
Comment 27 Build Bot 2017-05-08 18:37:54 PDT Comment hidden (obsolete)
Comment 28 Build Bot 2017-05-08 19:44:42 PDT Comment hidden (obsolete)
Comment 29 Build Bot 2017-05-08 19:44:43 PDT Comment hidden (obsolete)
Comment 30 Build Bot 2017-05-08 19:46:58 PDT Comment hidden (obsolete)
Comment 31 Build Bot 2017-05-08 19:46:59 PDT Comment hidden (obsolete)
Comment 32 Build Bot 2017-05-08 19:56:44 PDT Comment hidden (obsolete)
Comment 33 Build Bot 2017-05-08 19:56:45 PDT Comment hidden (obsolete)
Comment 34 Build Bot 2017-05-08 20:05:40 PDT Comment hidden (obsolete)
Comment 35 Build Bot 2017-05-08 20:05:42 PDT Comment hidden (obsolete)
Comment 36 Daniel Bates 2017-05-12 12:01:59 PDT
Created attachment 309922 [details]
Patch and layout tests
Comment 37 Build Bot 2017-05-12 12:04:58 PDT Comment hidden (obsolete)
Comment 38 Build Bot 2017-05-12 13:22:24 PDT Comment hidden (obsolete)
Comment 39 Build Bot 2017-05-12 13:22:25 PDT Comment hidden (obsolete)
Comment 40 Build Bot 2017-05-12 13:28:35 PDT Comment hidden (obsolete)
Comment 41 Build Bot 2017-05-12 13:28:37 PDT Comment hidden (obsolete)
Comment 42 Daniel Bates 2017-05-12 14:13:45 PDT
Created attachment 309943 [details]
Patch and layout tests

Updated patch to have JSEventListener hold a weak pointer to the incumbent global object so as to fix the test failure fast/workers/worker-document-leak.html.
Comment 43 Build Bot 2017-05-12 14:16:30 PDT Comment hidden (obsolete)
Comment 44 Geoffrey Garen 2017-05-15 11:46:47 PDT
Comment on attachment 309943 [details]
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=309943&action=review

r=me

> Source/JavaScriptCore/runtime/VM.cpp:467
> +JSGlobalObject* VM::topScriptHavingGlobalObject()

Let's use a shared helper function that we share with CallFrame::callerSourceOrigin. It can return a CodeBlock* or nullptr.

Let's rename this to callerGlobalObject.

> Source/JavaScriptCore/runtime/VM.h:707
> +    // Global object of the "topmost entry of the JavaScript execution context stack that has
> +    // a non-null ScriptOrModule component, or null if there is no such entry in the JavaScript
> +    // execution context stack."
> +    // <https://html.spec.whatwg.org/multipage/webappapis.html#topmost-script-having-execution-context> (5 May 2017)
> +    JSGlobalObject* topScriptHavingGlobalObject();
> +
> +    void incrementSkipWhenDeterminingIncumbentCounter() { ++m_skipWhenDeterminingIncumbentCounter; }
> +    void decrementSkipWhenDeterminingIncumbentCounter()
> +    {
> +        ASSERT(m_skipWhenDeterminingIncumbentCounter);
> +        --m_skipWhenDeterminingIncumbentCounter;
> +    }
> +    bool shouldUseBackupIncumbentGlobalObject() const { return m_skipWhenDeterminingIncumbentCounter > 0; }

This part of the algorithm is super crazy in two ways:

(1) Not all entry-points agree on whether they override the incumbent global object on the JavaScript stack or not. For example, mutation event listeners do override, but timers don't. That behavior is surprising and bad.

(2) It's never desirable -- for security or clarity -- to contradict what's actually on the stack.

You should open a spec ticket to change this behavior. We want to always honor the top of the JS stack if it's available.
Comment 45 Daniel Bates 2017-05-24 12:00:31 PDT
(In reply to Geoffrey Garen from comment #44)
> > Source/JavaScriptCore/runtime/VM.cpp:467
> > +JSGlobalObject* VM::topScriptHavingGlobalObject()
> 
> Let's use a shared helper function that we share with
> CallFrame::callerSourceOrigin. It can return a CodeBlock* or nullptr.
> 

Will do.

> Let's rename this to callerGlobalObject.
> 

Will do.

> > Source/JavaScriptCore/runtime/VM.h:707
> > +    // Global object of the "topmost entry of the JavaScript execution context stack that has
> > +    // a non-null ScriptOrModule component, or null if there is no such entry in the JavaScript
> > +    // execution context stack."
> > +    // <https://html.spec.whatwg.org/multipage/webappapis.html#topmost-script-having-execution-context> (5 May 2017)
> > +    JSGlobalObject* topScriptHavingGlobalObject();
> > +
> > +    void incrementSkipWhenDeterminingIncumbentCounter() { ++m_skipWhenDeterminingIncumbentCounter; }
> > +    void decrementSkipWhenDeterminingIncumbentCounter()
> > +    {
> > +        ASSERT(m_skipWhenDeterminingIncumbentCounter);
> > +        --m_skipWhenDeterminingIncumbentCounter;
> > +    }
> > +    bool shouldUseBackupIncumbentGlobalObject() const { return m_skipWhenDeterminingIncumbentCounter > 0; }
> 
> This part of the algorithm is super crazy in two ways:
> 
> (1) Not all entry-points agree on whether they override the incumbent global
> object on the JavaScript stack or not. For example, mutation event listeners
> do override, but timers don't. That behavior is surprising and bad.
> 
> (2) It's never desirable -- for security or clarity -- to contradict what's
> actually on the stack.
> 
> You should open a spec ticket to change this behavior. We want to always
> honor the top of the JS stack if it's available.

Filed <https://github.com/whatwg/html/issues/2706>.
Comment 46 Daniel Bates 2017-05-25 16:38:30 PDT
(In reply to Daniel Bates from comment #45)
> > You should open a spec ticket to change this behavior. We want to always
> > honor the top of the JS stack if it's available.
> 
> Filed <https://github.com/whatwg/html/issues/2706>.

In discussing <https://github.com/whatwg/html/issues/2706> I realized that the patch (attachment #309943 [details]) is incorrect because it stores the skip-when-determining-incumbent counter on the VM. But the skip-when-determining-incumbent counter is specific to each script-having execution context (ScriptExecutable?).
Comment 47 Daniel Bates 2017-05-25 16:41:38 PDT
Created attachment 311315 [details]
Patch and layout tests

Updated patch that moves the skip-when-determining-incumbent counter to ScriptExecutable and addresses feedback from Geoffrey Garen. I also added test case "Invoke window B postMessage() from window A function called via DOM event dispatched to window A from event handler added by window B" to file LayoutTests/fast/dom/Window/post-message-source-srcdoc-iframe.html.
Comment 48 Daniel Bates 2017-05-25 16:56:07 PDT
Created attachment 311319 [details]
Patch and layout tests

Rebase patch after <https://trac.webkit.org/changeset/217451> (bug #172601).
Comment 49 Build Bot 2017-05-25 16:58:39 PDT Comment hidden (obsolete)
Comment 50 Geoffrey Garen 2017-05-25 16:59:33 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=311315&action=review

> Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:41
> +    if (CodeBlock* callerCodeBlock = vm.topCallFrame ? vm.topCallFrame->callerCodeBlock() : nullptr)
> +        callerCodeBlock->ownerScriptExecutable()->incrementSkipWhenDeterminingIncumbentCounter();

Can you give some examples of when this object is instantiated and callerCodeBlock is not null? I can't seem to find any.

It's also not clear to me why you would skip the top call frame's caller rather than the top call frame.

You mentioned on IRC that all callbacks should now share the behavior that they propagate the caller that existed at the time the callback was scheduled. If that's the case, I don't understand why we need SkipWhenDeterminingIncumbentCounter.
Comment 51 Daniel Bates 2017-05-25 20:53:10 PDT
(In reply to Geoffrey Garen from comment #50)
> > Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:41
> > +    if (CodeBlock* callerCodeBlock = vm.topCallFrame ? vm.topCallFrame->callerCodeBlock() : nullptr)
> > +        callerCodeBlock->ownerScriptExecutable()->incrementSkipWhenDeterminingIncumbentCounter();
> 
> Can you give some examples of when this object is instantiated and
> callerCodeBlock is not null? I can't seem to find any.

The following test cases, included in LayoutTests/fast/dom/Window/post-message-source-srcdoc-iframe.html, are examples when a BackupIncumbentScope is instantiated and CallFrame::callerCodeBlock() is non null:

1. "Invoke window A postMessage(), bound in window A, via DOM event dispatched to window A"
    This test case is analogous to example 3 in section Incumbent in the HTML standard <https://html.spec.whatwg.org/multipage/webappapis.html#incumbent> (24 May 2017).

2. "Invoke window B postMessage() from window A function called via DOM event dispatched to window A from event handler added by window B" 

> 
> It's also not clear to me why you would skip the top call frame's caller
> rather than the top call frame.
> 

Can you please elaborate?

> You mentioned on IRC that all callbacks should now share the behavior that
> they propagate the caller that existed at the time the callback was
> scheduled. If that's the case, I don't understand why we need
> SkipWhenDeterminingIncumbentCounter.

From my understanding the existence of the skip-when-determining incumbent counter is solely to "fix up" the algorithm to work for event listeners :( See the above examples for more details.
Comment 52 Daniel Bates 2017-05-26 17:04:55 PDT
Geoffrey Garen and I spoke about this bug in-person today (05/26). Geoffrey suggested that we implement the incumbent algorithm in parts to ensure correctness and allow us to identify ways to simplify the incumbent algorithm (if possible).
Comment 53 Marian Rusnak 2018-05-03 04:07:55 PDT
Any plans to fix this issue?
Comment 54 Daniel Bates 2018-05-24 23:48:42 PDT
(In reply to Marian Rusnak from comment #53)
> Any plans to fix this issue?

Yes :) It is on my todo list. Having said that, feel free to take the bug and fix it!
Comment 55 Alexey Shvayka 2021-11-03 10:33:17 PDT Comment hidden (obsolete)
Comment 56 Alexey Shvayka 2021-11-10 14:39:35 PST Comment hidden (obsolete)
Comment 57 Alexey Shvayka 2021-11-12 09:27:53 PST
Created attachment 444074 [details]
Patch
Comment 58 Geoffrey Garen 2021-11-12 09:50:53 PST
Comment on attachment 444074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444074&action=review

> Source/JavaScriptCore/interpreter/CallFrame.cpp:-252
> -    // FIXME: We need to handle JSONP interpretation case in ProgramExecutable since it does not have vm.topCallFrame.
> -    // rdar://83691438

For this detail, I think we could consider just making a dummy call frame on the C++ stack before we do the JSONP execution.
Comment 59 Alexey Shvayka 2022-01-19 10:27:30 PST
Created attachment 449492 [details]
Patch
Comment 60 EWS Watchlist 2022-01-19 10:30:10 PST
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 61 Alexey Shvayka 2022-01-19 10:31:48 PST
(In reply to Geoffrey Garen from comment #58)
> Comment on attachment 444074 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444074&action=review
> 
> > Source/JavaScriptCore/interpreter/CallFrame.cpp:-252
> > -    // FIXME: We need to handle JSONP interpretation case in ProgramExecutable since it does not have vm.topCallFrame.
> > -    // rdar://83691438
> 
> For this detail, I think we could consider just making a dummy call frame on
> the C++ stack before we do the JSONP execution.

I've added vm.JSONPGlobalObject field instead; making a dummy call frame that works across all tiers is a bit non-trivial.
Comment 62 Geoffrey Garen 2022-01-24 16:19:08 PST
Comment on attachment 449492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449492&action=review

r=me

Seems like there's work to make EWS happy before landing.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:757
> +        SetForScope<JSGlobalObject*> changeJSONPGlobalObject(vm.JSONPGlobalObject, globalObject);

Neat simplification.

> Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:58
> +    internalField(Field::IncumbentGlobalObject).setWithoutWriteBarrier(&JSC::incumbentGlobalObject(globalObject, vm.topCallFrame));

Makes me sad, but I guess we have to do it to match the spec.

> Source/JavaScriptCore/runtime/RunCallbackScope.cpp:52
> +            if (isSameOriginDomain(topCallFrameGlobalObject, m_incumbentGlobalObject) == TriState::True)
> +                return m_incumbentGlobalObject;

I wonder if we should add an ASSERT_WITH_SECURITY_IMPLICATION here. We don't expect these to be different. Something like:

auto isSameOriginDomain = topCallFrameGlobalObject->globalObjectMethodTable()->isSameOriginDomain(topCallFrameGlobalObject, m_incumbentGlobalObject) == TriState::True;
ASSERT_WITH_SECURITY_IMPLICATION(isSameOriginDomain);
if (isSameOriginDomain)
    return m_incumbentGlobalObject;

Anyway, I like this compromise. It seems to ensure that the disappointing complexity of the incumbent global object spec can only create compatibility issues, and not security issues.

> Source/JavaScriptCore/runtime/RunCallbackScope.h:33
> +class JS_EXPORT_PRIVATE RunCallbackScope final {

I wonder if we should name this "AuthorCallbackScope" to clarify that you should deploy it any time you call into code that the spec would label "author" code. Or some other word to indicate "I'm calling out to author-specific JavaScript".

I'm not sure that "Run" adds information, so "CallbackScope" might also be OK.
Comment 63 Alexey Shvayka 2022-01-28 14:07:41 PST
Created attachment 450276 [details]
Patch

Add missing null check, add ASSERT(isSameOriginDomain), rename to AuthorCallbackScope, and patch up significant Promise jobs regression.
Comment 64 Alexey Shvayka 2022-01-28 14:14:43 PST
Created attachment 450277 [details]
Patch

svn add <missing files>
Comment 65 Alexey Shvayka 2022-01-28 15:02:39 PST
Created attachment 450281 [details]
Microbenchmark

(In reply to Geoffrey Garen from comment #62)

> r=me
> 
> Seems like there's work to make EWS happy before landing.

Thanks Geoff!

EWS was failing due to missing null check for m_incumbentGlobalObject.
What took some effort was patching up the significant performance regression of Promises.
The previous patch regressed attached microbenchmark by ~35%: from 5.5s on ToT to around 7.4s, tested via `time Tools/Scripts/run-jsc --release microbenchmark.js` (first run after build to be ignored).
This wasn't caught earlier because Tools/Scripts/run-jsc-benchmarks doesn't measure microtask loop execution.

The 35% regression DOES NOT even include execution of isSameOriginDomain() as it's missing in JSC console, so the first thing I did is introduced incumbentGlobalObjectToStore(), which skips that check and is called only if resolved _incumbent_ isn't userland-exposed (happens very often).
The second optimization was to introduce JSPromiseMicrotask for microtasks with userland code only, because adding Strong<> fields to JSMicrotask is costly.
And the last thing was to avoid call stack walk during AuthorCallbackScope and defer it until _incumbent_ resolution, saving a m_topCallFrame pointer only.

So the updated patch is only 7-8% slower in JSC console, which is IMO land-able, yet could be further improved by hand-rolling a faster version of CallFrame::globalObjectOfClosestCodeBlock().
I have a draft for that, it's a further 3-4% progression, yet the implementation that accounts for inlined DFG call frames is untrivial and it currently crashes.

> > Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:58
> > +    internalField(Field::IncumbentGlobalObject).setWithoutWriteBarrier(&JSC::incumbentGlobalObject(globalObject, vm.topCallFrame));
> 
> Makes me sad, but I guess we have to do it to match the spec.

Yeah, the spec explicitly requires it here: https://html.spec.whatwg.org/#hostenqueuefinalizationregistrycleanupjob.
This patch adds quite a few fields, yet we ensured the most of them are neutral on memory, unlike this one.
However, JSFinalizationRegistry is a very new and very low-level addition to JS, so I may presume it won't be used much on the web?

> > Source/JavaScriptCore/runtime/RunCallbackScope.cpp:52
> > +            if (isSameOriginDomain(topCallFrameGlobalObject, m_incumbentGlobalObject) == TriState::True)
> > +                return m_incumbentGlobalObject;
> 
> I wonder if we should add an ASSERT_WITH_SECURITY_IMPLICATION here. We don't
> expect these to be different. Something like:
> 
> auto isSameOriginDomain =
> topCallFrameGlobalObject->globalObjectMethodTable()-
> >isSameOriginDomain(topCallFrameGlobalObject, m_incumbentGlobalObject) ==
> TriState::True;
> ASSERT_WITH_SECURITY_IMPLICATION(isSameOriginDomain);
> if (isSameOriginDomain)
>     return m_incumbentGlobalObject;

The linter prohibits ASSERT_WITH_SECURITY_IMPLICATION now, so I went with just ASSERT to ensure we don't crash.

> > Source/JavaScriptCore/runtime/RunCallbackScope.h:33
> > +class JS_EXPORT_PRIVATE RunCallbackScope final {
> 
> I wonder if we should name this "AuthorCallbackScope" to clarify that you
> should deploy it any time you call into code that the spec would label
> "author" code. Or some other word to indicate "I'm calling out to
> author-specific JavaScript".

That's a great suggestion, "author" is heavily used in the HTML spec.
This naming makes very clear why, for example, JSMicrotask doesn't have AuthorCallbackScope: its job is never a userland code.
Comment 66 Ahmad Saleem 2022-09-03 06:46:08 PDT
Using Example test case, I get following:

** Safari Technology Preview 152 **

Got message from A: hello

** Chrome Canary 107 **

Got message from B: hello

** Firefox Nightly 106 **

Got message from B: hello

___________

STP 152 differs from other browsers. Just want to update, I know it is 2022 bug but just want to highlight difference. Thanks!