Last Comment Bug 777628 - Using postMessage to send an ImageData instance from a hiddenDOMWindow canvas crashes/throws
: Using postMessage to send an ImageData instance from a hiddenDOMWindow canvas...
Status: RESOLVED FIXED
[qa-]
: assertion, regression
Product: Core
Classification: Components
Component: DOM: Workers (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla17
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 743615
  Show dependency treegraph
 
Reported: 2012-07-25 23:03 PDT by Andrew Hurle [:ahurle]
Modified: 2012-10-09 04:58 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
wontfix
+
fixed
+
fixed


Attachments
Script that triggers crash (469 bytes, application/x-javascript)
2012-07-25 23:03 PDT, Andrew Hurle [:ahurle]
no flags Details
Problem report after crash on OSX (85.46 KB, text/plain)
2012-07-25 23:05 PDT, Andrew Hurle [:ahurle]
no flags Details
Do a Checked Unwrap in JS_WriteTypedArray. v1 (3.00 KB, patch)
2012-08-09 03:24 PDT, Bobby Holley (:bholley) (busy with Stylo)
jorendorff: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Andrew Hurle [:ahurle] 2012-07-25 23:03:07 PDT
Created attachment 646036 [details]
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] 2012-07-25 23:05:04 PDT
Created attachment 646038 [details]
Problem report after crash on OSX
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-26 07:26:55 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-26 07:29:55 PDT
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] 2012-07-26 15:23:04 PDT
FF14 regression, tracking for upcoming releases.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-08-09 03:24:30 PDT
Created attachment 650492 [details] [diff] [review]
Do a Checked Unwrap in JS_WriteTypedArray. v1

Attaching a patch. Flagging jorendorff for review.
Comment 6 Jason Orendorff [:jorendorff] 2012-08-13 17:16:50 PDT
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) (busy with Stylo) 2012-08-13 17:30:30 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9890e2f63b9e
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 08:32:10 PDT
Looks green. Pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e8d066279b
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-08-14 08:38:04 PDT
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 2012-08-14 09:04:09 PDT
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] 2012-08-14 17:51:55 PDT
https://hg.mozilla.org/mozilla-central/rev/e2e8d066279b
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-20 15:30:39 PDT
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) (busy with Stylo) 2012-08-21 11:21:00 PDT
landed to m-a:
https://hg.mozilla.org/releases/mozilla-aurora/rev/0112ad558432
Comment 14 Ioana (away) 2012-10-09 04:58:00 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.