Bug 94493 – [JSC] SerializedScriptValue::create() should throw a DataCloneError if input is an unsupported object

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 159476 [details]
Patch

[Oliver Hunt]

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.

[Oliver Hunt]

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.

[Chris Dumez]

Created attachment 159492 [details]
Patch

Take Oliver's feedback into consideration.

No regression here.

[Oliver Hunt]

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

[Chris Dumez]

Created attachment 159494 [details]
Patch

Fix typos in comment.

[Chris Dumez]

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

[WebKit Review Bot]

Comment on attachment 159494 [details]
Patch

Clearing flags on attachment: 159494

Committed r126067: <http://trac.webkit.org/changeset/126067>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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