1274362 - a Symbol passed to postMessage should throw DataCloneException

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marijn]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.36 Safari/537.36

Steps to reproduce:

Run the following javascript code:

let c = new BroadcastChannel("foo");
c.postMessage(Symbol());


Actual results:

Throws a "TypeError: unsupported type for structured data"


Expected results:

According to the structured clone spec[1], this should throw a "DataCloneError" DOMException instead. The same problem exists for other postMessage methods such as the one on MessagePort.

[1]: https://html.spec.whatwg.org/multipage/infrastructure.html#structuredclone

Comment 1

[Andrea Marchesini [:baku]]

Attachment: bc3.patch

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6963224026cd

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8754792 [details] [diff] [review]
bc3.patch

Just swallowing the pending exception here no matter what is wrong and it makes us fail tests.  Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa9ce57309ab138647d728e1321363f3b48096c

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Which tests?
Per spec we're pretty much supposed swallow exceptions, at least most of them.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

s/swallow/swallow and throw DataCloneException/

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

At least webmessaging/without-ports/026.htm.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: bc3.patch

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8755219 [details] [diff] [review]
bc3.patch

Review of attachment 8755219 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/StructuredClone.cpp
@@ +1234,5 @@
> +        callbacks->reportError(context(), JSMSG_SC_UNSUPPORTED_TYPE);
> +    } else {
> +        JS_ReportErrorNumber(context(), GetErrorMessage, nullptr, JSMSG_SC_UNSUPPORTED_TYPE);
> +    }
> +

I skimmed through StructuredClone.cpp and honestly didn't really understand the error handling, or specifically whether more of the error reporting needs to go through callbacks->reportError. I'm fine with making this specific change without looking further into it, but I'd like you to do it by adding a JS_SCERR_UNSUPPORTED_TYPE, adding it to ReportErrorTransferable, and then returning reportErrorTransferable() here.

Comment 9

[Andrea Marchesini [:baku]]

> I skimmed through StructuredClone.cpp and honestly didn't really understand
> the error handling, or specifically whether more of the error reporting

When something goes wrong, JS StructuredClone code, calls ReportError() callback and then it returns 'false'.
Following the spec, StructuredCloneHolder ignored all the error reported by the JS engine and return DataCloneError here:

https://mxr.mozilla.org/mozilla-central/source/dom/base/StructuredCloneHolder.cpp#278

The current setup works fine. The only issue here is that reportError() is not called and an exception is added in the JSContext.

Comment 10

[Steve Fink [:sfink] [:s:]]

(In reply to Andrea Marchesini (:baku) from comment #9)
> > I skimmed through StructuredClone.cpp and honestly didn't really understand
> > the error handling, or specifically whether more of the error reporting
> 
> When something goes wrong, JS StructuredClone code, calls ReportError()
> callback and then it returns 'false'.
> Following the spec, StructuredCloneHolder ignored all the error reported by
> the JS engine and return DataCloneError here:
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/base/
> StructuredCloneHolder.cpp#278
> 
> The current setup works fine. The only issue here is that reportError() is
> not called and an exception is added in the JSContext.

Sorry, I should have been more explicit. All that is fine. What I'm unsure of is whether the remaining calls to JS_ReportErrorNumber should be invoking the error callback as well. They are for when the object graph is too large, an unrecognized NaN value is seen, a string is too long, an invalid typed array or Date or RegExp or back-reference is found, and various other things. Currently, all these will return false and set an exception, but will not invoke the reportError callback. I don't understand the logic for which is which. Maybe the intention is that the reportError callback is only invoked for things that should report a DataCloneError according to the HTML5 spec? If so, then I think my comment 8 is correct, because that is the chokepoint for things that invoke the reportError callback. Except that then it should be renamed, since it's being used to trigger a DataCloneError that has nothing to do with transferables. Probably just reportCloneError.

In looking at this, I notice that the HTML5 spec section on structured cloning has been completely rewritten, and I doubt we fully follow the new spec. For example:

16. Otherwise, if input has a [[Clone]] internal method, then let output be ? input.[[Clone]](targetRealm, memory).

17. Otherwise, if IsCallable(input)}} is true, then throw a "DataCloneError" DOMException.

18. Otherwise, if input has any internal slot other than [[Prototype]] or [[Extensible]], then throw a "DataCloneError" DOMException.

I see no IsCallable checks currently. Maybe that doesn't matter, since anytime we create a new sort of object without special internal slots, we'll update the html5 spec for it?

Anyway, does the above make sense, or am I just confused about things somehow?

Comment 11

[Anne (:annevk)]

Note that symbol throws because of step 4 of https://html.spec.whatwg.org/multipage/infrastructure.html#structuredclone.

The idea is that each time ECMAScript gains a new object or primitive, we need to update the HTML Standard StructuredClone algorithms. It's really an extension to ECMAScript that's too hot a potato for TC39. New platform objects (defined through IDL) do not require changes to this algorithm. They simply use the [[Clone]] and [[Transfer]] internal slot mechanism.

I think the IsCallable() check is there to make postMessage(function() { }, ...) throw since there is nothing that ensures that otherwise.

Comment 12

[Andrea Marchesini [:baku]]

sfink, what about if we land this patch (with comments if you have it) and we discuss about other exceptions in a follow up?

Comment 13

[Steve Fink [:sfink] [:s:]]

(In reply to Andrea Marchesini (:baku) from comment #12)
> sfink, what about if we land this patch (with comments if you have it) and
> we discuss about other exceptions in a follow up?

As I said above: I'm fine with landing what this patch does, but I'd like it to be done slightly differently: by adding JS_SCERR_UNSUPPORTED_TYPE to reportErrorTransferable, renaming reportErrorTransferable to reportDataCloneError, and calling reportDataCloneError here instead of explicitly doing the |if (callbacks && callbacks->reportError)| thing inline. It's just a refactoring of what the patch does.

And I agree, the other exceptions can be deferred to a follow up. I'm not even sure if anything needs to be done; I'd need to talk to you more to figure that out. But that can be in a different bug.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: bc3.patch

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dac26bb716f9
a Symbol passed to postMessage should throw DataCloneException, r=sfink

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dac26bb716f9