Bug 163412 - REGRESSION: MessageEvent.source for bound postMessage() is wrong for same-origin iframe
Summary: REGRESSION: MessageEvent.source for bound postMessage() is wrong for same-ori...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari 10
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on: 171145
Blocks: 162075
  Show dependency treegraph
 
Reported: 2016-10-13 15:34 PDT by lee.wang.zhong+wk
Modified: 2018-05-24 23:48 PDT (History)
14 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

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
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.
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
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.
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
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.
Comment 12 Build Bot 2017-04-19 22:29:41 PDT
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
Comment 13 Build Bot 2017-04-19 22:29:42 PDT
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
Comment 14 Build Bot 2017-04-19 23:27:16 PDT
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
Comment 15 Build Bot 2017-04-19 23:27:18 PDT
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
Comment 16 Build Bot 2017-04-20 00:15:57 PDT
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
Comment 17 Build Bot 2017-04-20 00:15:59 PDT
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
Comment 18 Build Bot 2017-04-20 03:02:36 PDT
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
Comment 19 Build Bot 2017-04-20 03:02:38 PDT
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
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
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.
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
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.
Comment 28 Build Bot 2017-05-08 19:44:42 PDT
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.
Comment 29 Build Bot 2017-05-08 19:44:43 PDT
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
Comment 30 Build Bot 2017-05-08 19:46:58 PDT
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.
Comment 31 Build Bot 2017-05-08 19:46:59 PDT
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
Comment 32 Build Bot 2017-05-08 19:56:44 PDT
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.
Comment 33 Build Bot 2017-05-08 19:56:45 PDT
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
Comment 34 Build Bot 2017-05-08 20:05:40 PDT
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.
Comment 35 Build Bot 2017-05-08 20:05:42 PDT
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
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
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.
Comment 38 Build Bot 2017-05-12 13:22:24 PDT
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
Comment 39 Build Bot 2017-05-12 13:22:25 PDT
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
Comment 40 Build Bot 2017-05-12 13:28:35 PDT
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
Comment 41 Build Bot 2017-05-12 13:28:37 PDT
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
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
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.
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
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.
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!