Bug 87384

Summary: postMessage and webkitPostMessage should behave the same way
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore JavaScriptAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, eoconnor, haraken, japhet, levin, mjs, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 87118    
Attachments:
Description Flags
Patch
none
Patch
abarth: review-
Patch none

Chris Dumez
Reported 2012-05-24 06:13:31 PDT
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).
Attachments
Patch (3.60 KB, patch)
2012-05-24 06:24 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (8.00 KB, patch)
2012-05-24 06:54 PDT, Chris Dumez 		abarth: review-
DetailsFormatted DiffDiff
Patch (12.02 KB, patch)
2012-05-24 10:55 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2012-05-24 06:24:15 PDT
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
Comment 2 2012-05-24 06:54:33 PDT
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 3 2012-05-24 08:43:06 PDT
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 > +2012-05-24 Christophe Dumez <christophe.dumez@intel.com> Looks like you've got an extra ChangeLog entry.
Chris Dumez
Comment 4 2012-05-24 10:55:26 PDT
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
Comment 5 2012-05-24 10:58:24 PDT
+levin because we've discussed this in the past.
Adam Barth
Comment 6 2012-05-24 11:00:18 PDT
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
Comment 7 2012-05-24 11:12:00 PDT
Fine with me. Adding Oliver just in case he had any concerns.
Alexey Proskuryakov
Comment 8 2012-05-24 12:23:56 PDT
> 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
Comment 9 2012-05-24 12:26:36 PDT
(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
Comment 10 2012-05-24 12:37:05 PDT
> 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 11 2012-05-24 16:56:14 PDT
Comment on attachment 143850 [details] Patch Clearing flags on attachment: 143850 Committed r118445: <http://trac.webkit.org/changeset/118445>
WebKit Review Bot
Comment 12 2012-05-24 16:56:26 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.