a Symbol passed to postMessage should throw DataCloneException

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mek, Assigned: baku)

Tracking

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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
Flags: needinfo?(amarchesini)
(Assignee)

Comment 1

2 years ago
Created attachment 8754792 [details] [diff] [review]
bc3.patch
Assignee: nobody → amarchesini
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(amarchesini)
Attachment #8754792 - Flags: review?(bugs)

Updated

2 years ago
Attachment #8754792 - Flags: review?(bugs) → review+
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
Attachment #8754792 - Flags: review-

Comment 4

2 years ago
Which tests?
Per spec we're pretty much supposed swallow exceptions, at least most of them.
Flags: needinfo?(khuey)

Comment 5

2 years ago
s/swallow/swallow and throw DataCloneException/
At least webmessaging/without-ports/026.htm.
Flags: needinfo?(khuey)
(Assignee)

Comment 7

2 years ago
Created attachment 8755219 [details] [diff] [review]
bc3.patch
Attachment #8754792 - Attachment is obsolete: true
Attachment #8755219 - Flags: review?(sphink)
Whiteboard: btpp-active
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.
Attachment #8755219 - Flags: review?(sphink)
(Assignee)

Comment 9

2 years ago
> 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.
Flags: needinfo?(sphink)
(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?
Flags: needinfo?(sphink) → needinfo?(amarchesini)
(Assignee)

Updated

2 years ago
Flags: needinfo?(amarchesini) → needinfo?(annevk)

Comment 11

2 years ago
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.
Flags: needinfo?(annevk)
(Assignee)

Comment 12

2 years ago
sfink, what about if we land this patch (with comments if you have it) and we discuss about other exceptions in a follow up?
Flags: needinfo?(sphink)
(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.
Flags: needinfo?(sphink)
(Assignee)

Comment 14

2 years ago
Created attachment 8759610 [details] [diff] [review]
bc3.patch
Attachment #8755219 - Attachment is obsolete: true
Attachment #8759610 - Flags: review?(sphink)
Attachment #8759610 - Flags: review?(sphink) → review+

Comment 15

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dac26bb716f9
a Symbol passed to postMessage should throw DataCloneException, r=sfink

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dac26bb716f9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.