Bug 94581

Summary: window.postMessage() / MessagePort.postMessage() throw wrong exception for invalid ports argument
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, haraken, japhet, kenneth, s.choi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.w3.org/TR/html5/comms.html
Bug Depends on:    
Bug Blocks: 94310    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2012-08-21 03:14:54 PDT
1. According to the latest specification for window.postMessage() (http://www.w3.org/TR/html5/comms.html#dom-window-postmessage-2):
"Throws an INVALID_STATE_ERR if the ports array is not null and it contains either null entries or duplicate ports."

We currently throw a DATA_CLONE_ERR.

2. According to the latest specification for MessagePort.postMessage() (http://www.w3.org/TR/html5/comms.html#messageport):
"Throws an INVALID_STATE_ERR if the ports array is not null and it contains either null entries, duplicate ports, or the source or target port."

We currently throw a DATA_CLONE_ERR for those cases.

We should fix this to comply with the latest specification.
Comment 1 Chris Dumez 2012-08-21 04:21:05 PDT
Created attachment 159652 [details]
Patch
Comment 2 Chris Dumez 2012-08-21 04:38:04 PDT
Created attachment 159655 [details]
Patch
Comment 3 Chris Dumez 2012-08-21 04:41:44 PDT
Created attachment 159656 [details]
Patch
Comment 4 Kentaro Hara 2012-08-21 04:50:11 PDT
Comment on attachment 159656 [details]
Patch

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

The change looks OK. I just want to confirm cross-browser compatibility before r+ing it.

> Source/WebCore/bindings/v8/V8Utilities.cpp:117
> +            if (ports.contains(port)) {
> +                setDOMException(INVALID_STATE_ERR, isolate);
> +                return false;
> +            }

This change is trying to throw exception for the code that had not thrown exception before. What are the behaviors of other browsers (IE, Firefox and Opera)?
Comment 5 Chris Dumez 2012-08-21 05:05:46 PDT
Firefox does not seem to support MessageChannel / MessagePort at all:
https://bugzilla.mozilla.org/show_bug.cgi?id=677638
https://bugzilla.mozilla.org/show_bug.cgi?id=741618
Comment 6 Kentaro Hara 2012-08-21 05:06:25 PDT
(In reply to comment #5)
> Firefox does not seem to support MessageChannel / MessagePort at all:
> https://bugzilla.mozilla.org/show_bug.cgi?id=677638
> https://bugzilla.mozilla.org/show_bug.cgi?id=741618

Thanks, how about Opera and IE?
Comment 7 Chris Dumez 2012-08-21 05:16:08 PDT
On Opera, it throws a DATA_CLONE_ERR instead of an INVALID_STATE_ERR, but at least it throws:
PASS channel.port1.postMessage("duplicate port", [channel3.port1, channel3.port1]) threw exception DOMException: DATA_CLONE_ERR.

I'm not sure how I can check for IE.
Comment 8 Kentaro Hara 2012-08-21 05:37:23 PDT
Comment on attachment 159656 [details]
Patch

I checked it on IE. IE doesn't implement MessageChannel.

- IE and Firefox don't implement MessageChannel.

- Given that Opera throws exceptions, it makes sense for WebKit to throw some exception.

- Although Opera throws DATA_CLONE_ERR, the spec requires to throw INVALID_STATE_ERR. Since people wouldn't much care about the exception type, it sounds reasonable to change WebKit to throw INVALID_STATE_ERR according to the spec.

Let me r+ for now. But let's wait for landing the patch for one day, as someone might have a concern.
Comment 9 Chris Dumez 2012-08-22 03:09:17 PDT
Comment on attachment 159656 [details]
Patch

No one has raised any concerned and the EWS bots are all green. Setting cq flag. Could someone please commit?
Comment 10 Kentaro Hara 2012-08-22 03:10:46 PDT
Comment on attachment 159656 [details]
Patch

OK, thanks for the patch.
Comment 11 WebKit Review Bot 2012-08-22 03:36:47 PDT
Comment on attachment 159656 [details]
Patch

Clearing flags on attachment: 159656

Committed r126286: <http://trac.webkit.org/changeset/126286>
Comment 12 WebKit Review Bot 2012-08-22 03:36:51 PDT
All reviewed patches have been landed.  Closing bug.