Closed
Bug 777628
Opened 12 years ago
Closed 12 years ago
Using postMessage to send an ImageData instance from a hiddenDOMWindow canvas crashes/throws
Categories
(Core :: DOM: Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: ahurle, Assigned: bholley)
References
Details
(Keywords: assertion, regression, Whiteboard: [qa-])
Attachments
(3 files)
469 bytes,
application/x-javascript
|
Details | |
85.46 KB,
text/plain
|
Details | |
3.00 KB,
patch
|
jorendorff
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
||
FF14 regression, tracking for upcoming releases.
Updated•12 years ago
|
Assignee: nobody → bobbyholley+bmo
Assignee | ||
Comment 5•12 years ago
|
||
Attaching a patch. Flagging jorendorff for review.
Attachment #650492 -
Flags: review?(jorendorff)
Comment 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9890e2f63b9e
Assignee | ||
Comment 8•12 years ago
|
||
Looks green. Pushed to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e8d066279b
Assignee | ||
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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-
Updated•12 years ago
|
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e2e8d066279b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Updated•12 years ago
|
Comment 12•12 years ago
|
||
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+
Assignee | ||
Comment 13•12 years ago
|
||
landed to m-a: https://hg.mozilla.org/releases/mozilla-aurora/rev/0112ad558432
Comment 14•12 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.
Description
•