Bug 87384 – postMessage and webkitPostMessage should behave the same way

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Chris Dumez]

In Bug 73589, the V8 implementation has been updated in order to support transfer of MessagePorts. This follows the latest "implementation-ready" spec.
However, the JSC implementation has not been updated accordingly.

The following test case is supposed to test it:
  fast/dom/Window/window-postmessage-args.html

In particular, this check:
  tryPostMessageFunction(window.postMessage, channel3.port1, '*', [channel3.port1, channel3.port2]);

However, with JSC, this will not throw because of Bug 87118 (SerializedScriptValue.create() succeeds even if MessagePort object cannot be found in transferred ports).

[Chris Dumez]

Created attachment 143812 [details]
Patch

The new expected global result for fast/dom/Window/window-postmessage-args.html is now closer to the one for Chromium. The remaining diff is:
3,8c3,8
< PASS: Posting message ('1', 1): threw exception TypeError: Type error
< PASS: Posting message ('1', 1): threw exception TypeError: Type error
< PASS: Posting message ('2', c): threw exception TypeError: Type error
< PASS: Posting message ('2', c): threw exception TypeError: Type error
< PASS: Posting message ('3', [object Object]): threw exception TypeError: Type error
< PASS: Posting message ('3', [object Object]): threw exception TypeError: Type error
---
> PASS: Posting message ('1', 1): threw exception TypeError: TransferArray argument must be an object
> PASS: Posting message ('1', 1): threw exception TypeError: TransferArray argument must be an object
> PASS: Posting message ('2', c): threw exception TypeError: TransferArray argument must be an object
> PASS: Posting message ('2', c): threw exception TypeError: TransferArray argument must be an object
> PASS: Posting message ('3', [object Object]): threw exception TypeError: TransferArray argument has no length attribute
> PASS: Posting message ('3', [object Object]): threw exception TypeError: TransferArray argument has no length attribute

[Chris Dumez]

Created attachment 143820 [details]
Patch

Update JSC implementation of postMessage for MessagePort, DedicatedWorkerContext and Worker as well. This now matches the V8 implementation.

[Adam Barth]

Comment on attachment 143820 [details]
Patch

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

Rather than introduce a difference between postMessage and webkitPotMessage, I'd rather change both to be the same and match the spec.  I believe that means always passing "true" for this value, including in V8.

> Source/WebCore/ChangeLog:28
> [phone]  Christophe Dumez  <[e-mail]>

Looks like you've got an extra ChangeLog entry.

[Chris Dumez]

Created attachment 143850 [details]
Patch

Make postMessage behave the same way as webkitPostMessage, meaning that it supports transfer of MessagePorts and ArrayBuffers. Both JSC and V8 implementations have been updated, as per Adam Barth's feedback.

Instead of passing 'true' always for extendedTransfer argument, I simply removed it. It is never used without extended transfer so I thought I would clean up.

[Adam Barth]

+levin because we've discussed this in the past.

[Adam Barth]

Comment on attachment 143850 [details]
Patch

Thanks.  There's some compatibility risk with this change, but it seems like the right thing to do.

[David Levin]

Fine with me. Adding Oliver just in case he had any concerns.

[Alexey Proskuryakov]

> Rather than introduce a difference between postMessage and webkitPotMessage, I'd rather change both to be the same and match the spec.  

I do not have a lot of insight in this particular issue, but in general, this doesn't sounds like a great approach. Why break existing pages that use a webkit-prefixed version when new pages will use unprefixed one anyway?

There is some risk of a page doing something like "if (!window.postMessage && window.webkitPostMessage) window.postMessage = window.webkitPostMessage" and not testing in old browser versions. But breaking old browser versions seems like a much lesser evil than breaking pages.

[David Levin]

(In reply to comment #8)
> > Rather than introduce a difference between postMessage and webkitPotMessage, I'd rather change both to be the same and match the spec.  
> 
> I do not have a lot of insight in this particular issue, but in general, this doesn't sounds like a great approach. Why break existing pages that use a webkit-prefixed version when new pages will use unprefixed one anyway?
> 
> There is some risk of a page doing something like "if (!window.postMessage && window.webkitPostMessage) window.postMessage = window.webkitPostMessage" and not testing in old browser versions. But breaking old browser versions seems like a much lesser evil than breaking pages.

The behavior of postMessage has been augmented. webkitPostMessage should be unaffected and the change to postMessage is backward compatible.

[Adam Barth]

> There is some risk of a page doing something like "if (!window.postMessage && window.webkitPostMessage)

This conditional will never return true because there never existed a browser that contained webkitPostMessage but not postMessage.

[WebKit Review Bot]

Comment on attachment 143850 [details]
Patch

Clearing flags on attachment: 143850

Committed r118445: <http://trac.webkit.org/changeset/118445>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.