Closed Bug 344914 Opened 18 years ago Closed 18 years ago

crash caused by GetImageData; buggy JSObject allocation

Categories

(Core :: Graphics: Canvas2D, defect)

x86
Windows XP
defect
Not set
blocker

Tracking

()

RESOLVED FIXED

People

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

Details

(Keywords: crash, fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

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
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
- 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+
I don't have write access to the repository.
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.5?
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
Closed: 18 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+
Might you need to 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.

Attachment

General

Creator:
Created:
Updated:
Size: