Bug 156227 – MessageEvent.source window is incorrect once window has been reified

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

rdar://problem/25545831

[Chris Dumez]

Created attachment 275640 [details]
Patch

[Chris Dumez]

Created attachment 275645 [details]
Patch

[Chris Dumez]

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

[Chris Dumez]

Created attachment 275681 [details]
Patch

[Mark Lam]

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?

[Chris Dumez]

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

[Chris Dumez]

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.

[Chris Dumez]

Will update the patch as per Mark Lam's feedback offline and add an extra layout test for a corner case that Gavin mentioned.

[Chris Dumez]

Created attachment 275709 [details]
Patch

[Mark Lam]

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.

[Chris Dumez]

Created attachment 275711 [details]
Patch

[Chris Dumez]

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

[Chris Dumez]

I am trying to figure out why my patch does not build anymore :/

[Chris Dumez]

Created attachment 275722 [details]
Patch

[Chris Dumez]

(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

[Chris Dumez]

Committed r199087: <http://trac.webkit.org/changeset/199087>

[Joseph Pecoraro]

Comment on attachment 275722 [details]
Patch

Removing flag since this landed.