Bug 94493

Summary: [JSC] SerializedScriptValue::create() should throw a DataCloneError if input is an unsupported object
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: kenneth, oliver, ossy, s.choi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.w3.org/TR/html5/common-dom-interfaces.html#structured-clone
Bug Depends on: 94949    
Bug Blocks: 65292, 94310    
Attachments:
Description Flags
Patch
oliver: review-, oliver: commit-queue-
Patch
oliver: review+, oliver: commit-queue-
Patch none

Description Chris Dumez 2012-08-20 09:59:23 PDT
As per the structured clone specification, the JSC implementation SerializedScriptValue::create() should throw a DataCloneError if input is an unsupported native object (Function and Error objects).
We currently don't throw for those input values.

The V8 implementation already handles this properly.
Comment 1 Chris Dumez 2012-08-20 11:09:27 PDT
Created attachment 159476 [details]
Patch
Comment 2 Oliver Hunt 2012-08-20 11:14:10 PDT
Comment on attachment 159476 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:688
>              if (getCallData(value, unusedData) == CallTypeNone)

This is the function check here -- if the call type is not CallTypeNone then whatever object we are dealing with is a functino of some kind.
Comment 3 Oliver Hunt 2012-08-20 11:16:19 PDT
The better approach is just to do a classInfo check directly against JSFinalObject::s_info in objectStartState just before inputObjectStack.append(inObject)

That will also allow dumpIfTerminal to drop the stupid navigator check.
Comment 4 Chris Dumez 2012-08-20 12:46:29 PDT
Created attachment 159492 [details]
Patch

Take Oliver's feedback into consideration.

No regression here.
Comment 5 Oliver Hunt 2012-08-20 12:53:01 PDT
Comment on attachment 159492 [details]
Patch

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

r+, but cq- as you need to add the new results file for message-port-multi-expected

> LayoutTests/platform/chromium/fast/events/message-port-multi-expected.txt:-23
> -This test checks the various use cases around sending multiple ports through MessagePort.postMessage
> -
> -On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> -
> -
> -PASS channel.port1.postMessage("same port", [channel.port1]) threw exception Error: DATA_CLONE_ERR: DOM Exception 25.
> -PASS channel.port1.postMessage("entangled port", [channel.port2]) threw exception Error: DATA_CLONE_ERR: DOM Exception 25.
> -PASS channel.port1.postMessage("null port", [channel3.port1, null, channel3.port2]) threw exception Error: DATA_CLONE_ERR: DOM Exception 25.
> -PASS channel.port1.postMessage("notAPort", [channel3.port1, {}, channel3.port2]) threw exception TypeError: Type error.
> -PASS channel.port1.postMessage("notAnArray", channel3.port1) threw exception TypeError: Type error.
> -PASS channel.port1.postMessage("notASequence", [{length: 3}]) threw exception TypeError: Type error.
> -PASS channel.port1.postMessage("largeSequence", largePortArray) threw exception Error: DATA_CLONE_ERR: DOM Exception 25.
> -PASS event.ports is non-null and zero length when no port sent
> -PASS event.ports is non-null and zero length when empty array sent
> -PASS event.ports contains two ports when two ports sent
> -PASS event.ports contains two ports when two ports re-sent after error
> -PASS Sending host object has thrown Error: DATA_CLONE_ERR: DOM Exception 25
> -PASS send-port: transferred one port
> -PASS send-port-twice: transferred one port twice
> -PASS send-two-ports: transferred two ports
> -
> -TEST COMPLETE
> -

You've removed this -- presumably as the results are now identical, but in that case you need to add the new results file in the right place :D
Comment 6 Chris Dumez 2012-08-20 12:53:16 PDT
Created attachment 159494 [details]
Patch

Fix typos in comment.
Comment 7 Chris Dumez 2012-08-20 12:57:33 PDT
(In reply to comment #5)
> (From update of attachment 159492 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159492&action=review
> 
> r+, but cq- as you need to add the new results file for message-port-multi-expected
> 
> > LayoutTests/platform/chromium/fast/events/message-port-multi-expected.txt:-23
> > -This test checks the various use cases around sending multiple ports through MessagePort.postMessage
> > -
> > -On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > -
> > -
> > -PASS channel.port1.postMessage("same port", [channel.port1]) threw exception Error: DATA_CLONE_ERR: DOM Exception 25.
> > -PASS channel.port1.postMessage("entangled port", [channel.port2]) threw exception Error: DATA_CLONE_ERR: DOM Exception 25.
> > -PASS channel.port1.postMessage("null port", [channel3.port1, null, channel3.port2]) threw exception Error: DATA_CLONE_ERR: DOM Exception 25.
> > -PASS channel.port1.postMessage("notAPort", [channel3.port1, {}, channel3.port2]) threw exception TypeError: Type error.
> > -PASS channel.port1.postMessage("notAnArray", channel3.port1) threw exception TypeError: Type error.
> > -PASS channel.port1.postMessage("notASequence", [{length: 3}]) threw exception TypeError: Type error.
> > -PASS channel.port1.postMessage("largeSequence", largePortArray) threw exception Error: DATA_CLONE_ERR: DOM Exception 25.
> > -PASS event.ports is non-null and zero length when no port sent
> > -PASS event.ports is non-null and zero length when empty array sent
> > -PASS event.ports contains two ports when two ports sent
> > -PASS event.ports contains two ports when two ports re-sent after error
> > -PASS Sending host object has thrown Error: DATA_CLONE_ERR: DOM Exception 25
> > -PASS send-port: transferred one port
> > -PASS send-port-twice: transferred one port twice
> > -PASS send-two-ports: transferred two ports
> > -
> > -TEST COMPLETE
> > -
> 
> You've removed this -- presumably as the results are now identical, but in that case you need to add the new results file in the right place :D

I have updated the expected result at LayoutTests/fast/events/message-port-multi-expected.txt. Isn't it enough?
Comment 8 WebKit Review Bot 2012-08-20 14:13:10 PDT
Comment on attachment 159494 [details]
Patch

Clearing flags on attachment: 159494

Committed r126067: <http://trac.webkit.org/changeset/126067>
Comment 9 WebKit Review Bot 2012-08-20 14:13:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Csaba Osztrogonác 2012-08-24 10:17:59 PDT
(In reply to comment #8)
> (From update of attachment 159494 [details])
> Clearing flags on attachment: 159494
> 
> Committed r126067: <http://trac.webkit.org/changeset/126067>

It caused an assertion on Qt-WK2 platform. Could you check it, please?
Here is the new bug report for it: https://bugs.webkit.org/show_bug.cgi?id=94949