777628 - Using postMessage to send an ImageData instance from a hiddenDOMWindow canvas crashes/throws

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Andrew Hurle [:ahurle]]

Attachment: Script that triggers crash

Run the attached script on a debug build in Scratchpad with Environment > Browser.  Firefox crashes after running the script, and prints this to the console:

> Assertion failure: obj->isTypedArray(), at c:\firefox-src\js\src\jstypedarrayinlines.h:64

On a nightly release build, no crash occurs, but an "allocation size overflow" exception is thrown.

I can reproduce on OSX, but the assertion failure is on line 103 instead.  Haven't tried Linux.

This was noticed at the same time as bug 777613, same wide regression window, possibly related?  A workaround is to create a new object, copy the properties of the ImageData instance onto it, and send that through instead.  Only happens with hiddenDOMWindow, can't reproduce in web content or by just using "document" in the scratchpad.

Comment 1

[Andrew Hurle [:ahurle]]

Attachment: Problem report after crash on OSX

Comment 2

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

We end up at 

3838   nsCOMPtr<nsIDOMImageData> imageData = do_QueryInterface(native);
3839   if (imageData) {
3840     // Prepare the ImageData internals.
3841     PRUint32 width, height;
3842     JS::Value dataArray;
3843     if (NS_FAILED(imageData->GetWidth(&width)) ||
3844         NS_FAILED(imageData->GetHeight(&height)) ||
3845         NS_FAILED(imageData->GetData(cx, &dataArray)))
3846     {
3847       return false;
3848     }
3849 
3850     // Write the internals to the stream.
3851     return JS_WriteUint32Pair(writer, SCTAG_DOM_IMAGEDATA, 0) &&
3852            JS_WriteUint32Pair(writer, width, height) &&
3853            JS_WriteTypedArray(writer, dataArray);
3854   }

This code doesn't realize that dataArray could be a cross-compartment wrapped (if imageData came from another compartment, like it does here).  Trying to JS_WriteTypedArray on a wrapped gives the js engine indigestion.

Note that if the JS_WrapValue called happened outside of GetData (say, in the wrapper) this would just work.

Comment 3

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

Also you can trigger this with content.  The hidden window isn't necessary here, just a window in a different compartment.

Comment 4

[Alex Keybl [:akeybl]]

FF14 regression, tracking for upcoming releases.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Do a Checked Unwrap in JS_WriteTypedArray. v1

Attaching a patch. Flagging jorendorff for review.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 650492 [details] [diff] [review]
Do a Checked Unwrap in JS_WriteTypedArray. v1

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

::: dom/tests/mochitest/bugs/test_bug777628.html
@@ +27,5 @@
> +canvas.width = 200;
> +canvas.height = 200;
> +doc.body.appendChild(canvas);
> +var ctx = canvas.getContext('2d');
> +ctx.fillStyle = 'rgb(';

I don't really understand this last line, but OK.

::: js/src/jsclone.cpp
@@ +401,5 @@
>  JS_WriteTypedArray(JSStructuredCloneWriter *w, jsval v)
>  {
>      JS_ASSERT(v.isObject());
>      RootedObject obj(w->context(), &v.toObject());
> +    // If the object is a security wrapper, try puncturing it. This may throw

Blank line before this comment.

@@ +402,5 @@
>  {
>      JS_ASSERT(v.isObject());
>      RootedObject obj(w->context(), &v.toObject());
> +    // If the object is a security wrapper, try puncturing it. This may throw
> +    // if the acccess is not allowed.

Too many c's in that word

Comment 7

[Bobby Holley (:bholley)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9890e2f63b9e

Comment 8

[Bobby Holley (:bholley)]

Looks green. Pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e8d066279b

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 650492 [details] [diff] [review]
Do a Checked Unwrap in JS_WriteTypedArray. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 743615
User impact if declined: Web compat, but we already shipped this in FF14, so structured cloning a cross-origin imagedata is unlikely to be very common.
Testing completed (on m-c, etc.): Just pushed to m-i, but we're running low on time for beta so I wanted to get it on the radar.
Risk to taking this patch (and alternatives if risky): Patch is very low-risk. But the alternative of just shipping the regression in FF15 might be ok too.
String or UUID changes made by this patch: none.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 650492 [details] [diff] [review]
Do a Checked Unwrap in JS_WriteTypedArray. v1

Thanks for making sure we saw this early. Since it's not on central yet, we shipped this in 14, and we're about to go to build with our second-to-last Beta we'll hold off on this for 15 as well instead of crash landing it with the other things that are more crucial to get in for this release.  I'll come back around and approve for Aurora once this has been on central for a while.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 650492 [details] [diff] [review]
Do a Checked Unwrap in JS_WriteTypedArray. v1

Been on m-c for while, I trust there are no obvious regressions in the last 5 days since, so approving for Aurora uplift.

Comment 13

[Bobby Holley (:bholley)]

landed to m-a:
https://hg.mozilla.org/releases/mozilla-aurora/rev/0112ad558432

Comment 14

[Ioana (away)]

An automated test that verifies this bug was pushed with the fix (dom/tests/mochitest/bugs/test_bug777628.html), so I will removed the verifyme keyword. Please re-add it if you need QA to do more testing around this fix.