Bug 156227 - MessageEvent.source window is incorrect once window has been reified
Summary: MessageEvent.source window is incorrect once window has been reified
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 159364
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-04 21:49 PDT by Chris Dumez
Modified: 2016-07-01 13:17 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.18 KB, patch)
2016-04-04 22:12 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (19.19 KB, patch)
2016-04-04 22:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (19.25 KB, patch)
2016-04-05 11:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.47 KB, patch)
2016-04-05 16:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.81 KB, patch)
2016-04-05 16:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.81 KB, patch)
2016-04-05 17:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-04-04 21:49:57 PDT
MessageEvent.source window is incorrect once window has been reified.

If the Window has not been reified, we keep constructing new postMessage() functions when calling window.postMessage(). We pass activeDOMWindow(execState) as source Window to DOMWindow::postMessage(). activeDOMWindow() uses exec->lexicalGlobalObject() which does the right thing because we construct a new postMessage() function in the caller's context.

However, after reification, due to the way JSDOMWindow::getOwnPropertySlot() is implemented, we stop constructing new postMessage() functions when calling window.postMessage(). As a result, the source window becomes incorrect because exec->lexicalGlobalObject() returns the target Window instead.
Comment 1 Chris Dumez 2016-04-04 22:02:37 PDT
rdar://problem/25545831
Comment 2 Chris Dumez 2016-04-04 22:12:54 PDT
Created attachment 275640 [details]
Patch
Comment 3 Chris Dumez 2016-04-04 22:53:18 PDT
Created attachment 275645 [details]
Patch
Comment 4 Chris Dumez 2016-04-04 22:54:34 PDT
Comment on attachment 275645 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:592
> +    if (!callerWindow) {

This is covered by fast/dom/Window/post-message-crash.html
Comment 5 Chris Dumez 2016-04-05 11:47:03 PDT
Created attachment 275681 [details]
Patch
Comment 6 Mark Lam 2016-04-05 12:14:52 PDT
Comment on attachment 275681 [details]
Patch

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

LGTM otherwise.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:588
> +    return codeBlock ? &asJSDOMWindow(codeBlock->globalObject())->wrapped() : nullptr;

Is there ever a case where you would expect codeBlock to be null?  If not, should we have an assertion here instead?
Comment 7 Chris Dumez 2016-04-05 13:21:45 PDT
(In reply to comment #6)
> Comment on attachment 275681 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275681&action=review
> 
> LGTM otherwise.
> 
> > Source/WebCore/bindings/js/JSDOMBinding.cpp:588
> > +    return codeBlock ? &asJSDOMWindow(codeBlock->globalObject())->wrapped() : nullptr;
> 
> Is there ever a case where you would expect codeBlock to be null?  If not,
> should we have an assertion here instead?

Yes, this is covered by fast/dom/Window/post-message-crash.html as mentioned earlier.
Basically, it is null when postMessage gets called from native code, e.g.:
setTimeout(postMessage, 0) or requestAnimationFrame(postMessage)
Comment 8 Chris Dumez 2016-04-05 13:23:44 PDT
Here is that the spec says regarding what the 'source' window should be:
1. https://html.spec.whatwg.org/multipage/comms.html#dom-messageevent-source
"The source attribute must be initialised to the WindowProxy object corresponding to the global object (a Window object) specified by incumbentSettings"

incumbentSettings is defined at:
https://html.spec.whatwg.org/multipage/webappapis.html#incumbent-settings-object

which says to use JavaScript's GetActiveScriptOrModule(), which is defined at:
https://tc39.github.io/ecma262/#sec-getactivescriptormodule

I am not 100% sure how to interpret this.
Comment 9 Chris Dumez 2016-04-05 14:03:25 PDT
Will update the patch as per Mark Lam's feedback offline and add an extra layout test for a corner case that Gavin mentioned.
Comment 10 Chris Dumez 2016-04-05 16:15:00 PDT
Created attachment 275709 [details]
Patch
Comment 11 Mark Lam 2016-04-05 16:18:14 PDT
Comment on attachment 275709 [details]
Patch

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

r=me

> LayoutTests/fast/dom/Window/post-message-crash2.html:17
> +var b = window.postMessage.bind(window, "msg", "*");
> +setTimeout(b, 0);

nit: I'd prefer that you rename "b" to "boundPostMessage", but I won't insist.
Comment 12 Chris Dumez 2016-04-05 16:21:32 PDT
Created attachment 275711 [details]
Patch
Comment 13 Chris Dumez 2016-04-05 16:22:12 PDT
(In reply to comment #11)
> Comment on attachment 275709 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275709&action=review
> 
> r=me
> 
> > LayoutTests/fast/dom/Window/post-message-crash2.html:17
> > +var b = window.postMessage.bind(window, "msg", "*");
> > +setTimeout(b, 0);
> 
> nit: I'd prefer that you rename "b" to "boundPostMessage", but I won't
> insist.

Done. Thanks.
Comment 14 Chris Dumez 2016-04-05 17:00:37 PDT
I am trying to figure out why my patch does not build anymore :/
Comment 15 Chris Dumez 2016-04-05 17:09:24 PDT
Created attachment 275722 [details]
Patch
Comment 16 Chris Dumez 2016-04-05 17:10:03 PDT
(In reply to comment #14)
> I am trying to figure out why my patch does not build anymore :/

It was because Phil landed an extra 'const' in the mean time:
http://trac.webkit.org/changeset/199076
Comment 17 Chris Dumez 2016-04-05 18:18:08 PDT
Committed r199087: <http://trac.webkit.org/changeset/199087>
Comment 18 Joseph Pecoraro 2016-04-14 11:03:38 PDT
Comment on attachment 275722 [details]
Patch

Removing flag since this landed.