crash caused by GetImageData; buggy JSObject allocation

RESOLVED FIXED

Status

()

--
blocker
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: bugzilla.20.scyt, Assigned: vlad)

Tracking

({crash, fixed1.8.1})

Trunk
x86
Windows XP
crash, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.1 ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

The following code snippet from nsCanvasRenderingContext2D::GetImageData has a problem with correctly allocating JS objects.

http://lxr.mozilla.org/mozilla/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#2846

2846     JSObject *dataArray = JS_NewArrayObject(ctx, w*h*4, jsvector.get());
2847     if (!dataArray)
2848         return NS_ERROR_OUT_OF_MEMORY;
2849 
2850     JSObject *result = JS_NewObject(ctx, NULL, NULL, NULL);
2851     if (!result)
2852         return NS_ERROR_OUT_OF_MEMORY;

What can happen here is that the array allocated in line 2846 gets deallocated by the garbage collector in line 2850. Obviously this leads to a crash later on. 


Reproducible: Always
(Reporter)

Comment 1

13 years ago
Created attachment 229455 [details] [diff] [review]
patch preventing interference by GC

Comment 2

13 years ago
we've changed to using localrootscopes instead of jsaddroot. 
Assignee: nobody → vladimir
Severity: critical → blocker
Keywords: crash
(In reply to comment #2)
> we've changed to using localrootscopes instead of jsaddroot. 

Except that there isn't a public API for that (bug 332648 seems to cover that)yet, so JS_AddRoot is about the best code in content/ can do.

My only comment on the patch itself is that in Mozilla, we have an nsAutoGCRoot class that'll avoid the fragility of having to add JS_RemoveRoot calls for every early return, and IMO, makes for prettier code. Sebasitan, would you mind making a new patch using that API (also, a nitpick: please use spaces instead of tabs in your editor, since the surrounding code already does that).
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 4

13 years ago
Created attachment 229480 [details] [diff] [review]
patch preventing interference by GC

- using nsAutoGCRoot
- no tabs
Attachment #229455 - Attachment is obsolete: true
Comment on attachment 229480 [details] [diff] [review]
patch preventing interference by GC

Looks good, thanks.
Attachment #229480 - Flags: review+
Comment on attachment 229480 [details] [diff] [review]
patch preventing interference by GC

Nice catch, thanks!  I can check this in when the tree opens again, not sure if you have commit privs or not.
Attachment #229480 - Flags: superreview+
(Reporter)

Comment 7

13 years ago
I don't have write access to the repository.

Updated

13 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.5?

Comment 8

13 years ago
sorry, i forgot :). in xpconnect we have lots of magical helpers. i should have been less specific :)
This code doesn't exist on 1.8.0.x
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.5?
Comment on attachment 229480 [details] [diff] [review]
patch preventing interference by GC

Asking for 1.8.1 approval;  potential gc crash fix in canvas code.
Attachment #229480 - Flags: approval1.8.1?
Checked in on trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Comment on attachment 229480 [details] [diff] [review]
patch preventing interference by GC

a=drivers, please land on the 181-branch
Attachment #229480 - Flags: approval1.8.1? → approval1.8.1+
Checked in on 1.8.1
Keywords: fixed1.8.1
Created attachment 229856 [details] [diff] [review]
root |result| as well

Yes, yes it does.  Ugh.
Attachment #229856 - Flags: review?(mrbkap)
Attachment #229856 - Flags: review?(mrbkap) → review+
(In reply to comment #15)
> Yes, yes it does.  Ugh.

To be clear, it does because JS_DefineProperty can cause result's slots array to grow, which can cause a GC (and we've already learned that newborn roots are not sufficient to protect against objects being collected).
Comment on attachment 229856 [details] [diff] [review]
root |result| as well

Ok, really fully fixed now, requesting 1.8.1 approval again, this time with not one but 2 nsAutoGCRoots.  (Sorry! :/)
Attachment #229856 - Flags: approval1.8.1?
Comment on attachment 229856 [details] [diff] [review]
root |result| as well

a=drivers, especially if it's now "really, fully fixed" :)
Attachment #229856 - Flags: approval1.8.1? → approval1.8.1+
Okay, it's really truly totally fixed on trunk and branch now.
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.