Bug 163412 – Implement backup incumbent settings object stack and fix _entry_ realm so it's always correct

• Component: DOM
• Status: NEW

On iOS 10, a `postMessage` from 

This is a change from iOS 9.

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

[Radar WebKit Bug Importer]

<rdar://problem/28776024>

[Chris Dumez]

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

[Daniel Bates]

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

[Daniel Bates]

Created attachment 304301 [details]
Patch and layout tests

[WebKit Commit Bot]

Attachment 304301 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:70:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 304309 [details]
Patch and layout tests

Add forwarding header file for JSBoundFunction.h.

[WebKit Commit Bot]

Attachment 304309 [details] did not pass style-queue:


ERROR: Source/WebCore/ForwardingHeaders/runtime/JSBoundFunction.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:70:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

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.

[Daniel Bates]

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.

[Build Bot]

Attachment 307546 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:70:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 307546 [details]
Patch and layout tests

Attachment 307546 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3567686

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html

[Build Bot]

Created attachment 307556 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307546 [details]
Patch and layout tests

Attachment 307546 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3567933

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html

[Build Bot]

Created attachment 307563 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307546 [details]
Patch and layout tests

Attachment 307546 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3568219

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html

[Build Bot]

Created attachment 307570 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307546 [details]
Patch and layout tests

Attachment 307546 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3568930

New failing tests:
imported/w3c/web-platform-tests/html/webappapis/scripting/events/compile-event-handler-settings-objects.html

[Build Bot]

Created attachment 307583 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Daniel Bates]

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

[Build Bot]

Attachment 307623 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:70:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

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.

[Saam Barati]

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.

[Saam Barati]

I think it's also worth ensuring this works for Promises, requestAnimationFrame, etc. (I spoke to Dan in person).

[Daniel Bates]

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

[Daniel Bates]

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.

[Build Bot]

Attachment 309455 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSLazyEventListener.cpp:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/runtime/BackupIncumbentScope.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:69:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSEventListener.cpp:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 5 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 309455 [details]
Patch and layout tests

Attachment 309455 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3702628

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 309458 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309455 [details]
Patch and layout tests

Attachment 309455 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3702631

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 309459 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309455 [details]
Patch and layout tests

Attachment 309455 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3702639

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 309460 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309455 [details]
Patch and layout tests

Attachment 309455 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3702644

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 309461 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Daniel Bates]

Created attachment 309922 [details]
Patch and layout tests

[Build Bot]

Attachment 309922 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSLazyEventListener.cpp:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/runtime/BackupIncumbentScope.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/bindings/js/JSMutationCallback.cpp:46:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:69:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSEventListener.cpp:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 309922 [details]
Patch and layout tests

Attachment 309922 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3727746

New failing tests:
fast/workers/worker-document-leak.html

[Build Bot]

Created attachment 309934 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309922 [details]
Patch and layout tests

Attachment 309922 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3727772

New failing tests:
fast/workers/worker-document-leak.html

[Build Bot]

Created attachment 309935 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Daniel Bates]

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.

[Build Bot]

Attachment 309943 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSLazyEventListener.cpp:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/runtime/BackupIncumbentScope.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/bindings/js/JSMutationCallback.cpp:46:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:69:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSEventListener.cpp:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

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.

[Daniel Bates]

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

[Daniel Bates]

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

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 311319 [details]
Patch and layout tests

Rebase patch after <https://trac.webkit.org/changeset/217451> (bug #172601).

[Build Bot]

Attachment 311319 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSLazyEventListener.cpp:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/runtime/BackupIncumbentScope.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/bindings/js/JSMutationCallback.cpp:46:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/ScheduledAction.cpp:69:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSEventListener.cpp:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/BackupIncumbentScope.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

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.

[Daniel Bates]

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

[Daniel Bates]

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

[Marian Rusnak]

Any plans to fix this issue?

[Daniel Bates]

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

[Alexey Shvayka]

Created attachment 443209 [details]
WIP

[Alexey Shvayka]

Created attachment 443865 [details]
WIP

[Alexey Shvayka]

Created attachment 444074 [details]
Patch

[Geoffrey Garen]

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.

[Alexey Shvayka]

Created attachment 449492 [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

[Alexey Shvayka]

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

[Geoffrey Garen]

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.

[Alexey Shvayka]

Created attachment 450276 [details]
Patch

Add missing null check, add ASSERT(isSameOriginDomain), rename to AuthorCallbackScope, and patch up significant Promise jobs regression.

[Alexey Shvayka]

Created attachment 450277 [details]
Patch

svn add <missing files>

[Alexey Shvayka]

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.

[Ahmad Saleem]

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!