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

RESOLVED FIXED in Firefox 16

Status

()

Core
DOM: Workers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ahurle, Assigned: bholley)

Tracking

({assertion, regression})

Trunk
mozilla17
x86_64
All
assertion, regression
Points:
---

Firefox Tracking Flags

(firefox14 wontfix, firefox15+ wontfix, firefox16+ fixed, firefox17+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 646038 [details]
Problem report after crash on OSX
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.
Blocks: 743615
Keywords: regressionwindow-wanted
Also you can trigger this with content.  The hidden window isn't necessary here, just a window in a different compartment.
status-firefox14: --- → wontfix
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?

Comment 4

5 years ago
FF14 regression, tracking for upcoming releases.
tracking-firefox15: ? → +
tracking-firefox16: ? → +
tracking-firefox17: ? → +

Updated

5 years ago
Assignee: nobody → bobbyholley+bmo
Created attachment 650492 [details] [diff] [review]
Do a Checked Unwrap in JS_WriteTypedArray. v1

Attaching a patch. Flagging jorendorff for review.
Attachment #650492 - Flags: review?(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
Attachment #650492 - Flags: review?(jorendorff) → review+
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9890e2f63b9e
Looks green. Pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e8d066279b
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.
Attachment #650492 - Flags: approval-mozilla-beta?
Attachment #650492 - Flags: approval-mozilla-aurora?
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.
Attachment #650492 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox15: affected → wontfix
https://hg.mozilla.org/mozilla-central/rev/e2e8d066279b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
status-firefox17: affected → fixed
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.
Attachment #650492 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
landed to m-a:
https://hg.mozilla.org/releases/mozilla-aurora/rev/0112ad558432
status-firefox16: affected → fixed
Keywords: verifyme

Comment 14

5 years ago
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.
Keywords: verifyme
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.