Closed Bug 777628 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Workers, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox14 --- wontfix
firefox15 + wontfix
firefox16 + fixed
firefox17 + fixed

People

(Reporter: ahurle, Assigned: bholley)

References

Details

(Keywords: assertion, regression, Whiteboard: [qa-])

Attachments

(3 files)

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.
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.
Also you can trigger this with content. The hidden window isn't necessary here, just a window in a different compartment.
FF14 regression, tracking for upcoming releases.
Assignee: nobody → bobbyholley+bmo
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+
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: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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+
Keywords: verifyme
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.

Attachment

General

Created:
Updated:
Size: