Closed
Bug 344914
Opened 18 years ago
Closed 18 years ago
crash caused by GetImageData; buggy JSObject allocation
Categories
(Core :: Graphics: Canvas2D, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bugzilla.20.scyt, Assigned: vlad)
Details
(Keywords: crash, fixed1.8.1)
Attachments
(2 files, 1 obsolete file)
698 bytes,
patch
|
mrbkap
:
review+
vlad
:
superreview+
beltzner
:
approval1.8.1+
|
Details | Diff | Splinter Review |
1.40 KB,
patch
|
mrbkap
:
review+
beltzner
:
approval1.8.1+
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
we've changed to using localrootscopes instead of jsaddroot.
Comment 3•18 years ago
|
||
(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•18 years ago
|
||
- using nsAutoGCRoot
- no tabs
Attachment #229455 -
Attachment is obsolete: true
Comment 5•18 years ago
|
||
Comment on attachment 229480 [details] [diff] [review]
patch preventing interference by GC
Looks good, thanks.
Attachment #229480 -
Flags: review+
Assignee | ||
Comment 6•18 years ago
|
||
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•18 years ago
|
||
I don't have write access to the repository.
Updated•18 years ago
|
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 :)
Assignee | ||
Comment 9•18 years ago
|
||
This code doesn't exist on 1.8.0.x
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.5?
Assignee | ||
Comment 10•18 years ago
|
||
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?
Assignee | ||
Comment 11•18 years ago
|
||
Checked in on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 12•18 years ago
|
||
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?
Assignee | ||
Comment 15•18 years ago
|
||
Yes, yes it does. Ugh.
Attachment #229856 -
Flags: review?(mrbkap)
Updated•18 years ago
|
Attachment #229856 -
Flags: review?(mrbkap) → review+
Comment 16•18 years ago
|
||
(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).
Assignee | ||
Comment 17•18 years ago
|
||
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?
Keywords: fixed1.8.1
Comment 18•18 years ago
|
||
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+
Assignee | ||
Comment 19•18 years ago
|
||
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.
Description
•