Closed
Bug 526452
Opened 15 years ago
Closed 15 years ago
imgContainer::ClearFrame() should check whether surface is null or not (Crash [@gfxContext::gfxContext(gfxASurface*) ])
Categories
(Core :: Graphics: ImageLib, defect, P1)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta4-fixed |
People
(Reporter: m_kato, Assigned: m_kato)
References
()
Details
(Keywords: crash)
Crash Data
Attachments
(2 files, 2 obsolete files)
2.05 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
2.63 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
imgContainer::ClearFrame doesn't check whether surf is null or not. We should check it. 1734 void imgContainer::ClearFrame(imgFrame *aFrame, nsIntRect &aRect) 1735 { 1736 if (!aFrame || aRect.width <= 0 || aRect.height <= 0) 1737 return; 1738 1739 aFrame->LockImageData(); 1740 1741 nsRefPtr<gfxASurface> surf; 1742 aFrame->GetSurface(getter_AddRefs(surf)); 1743 1744 // Erase the destination rectangle to transparent 1745 gfxContext ctx(surf); 1746 ctx.SetOperator(gfxContext::OPERATOR_CLEAR); 1747 ctx.Rectangle(gfxRect(aRect.x, aRect.y, aRect.width, aRect.height)); 1748 ctx.Fill(); Also, there is some crash sample in crash-state. 3.6b1 http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.6b1&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=gfxContext%3A%3AgfxContext%28gfxASurface*%29 3.7a1pre http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.7a1pre&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=gfxContext%3A%3AgfxContext%28gfxASurface*%29
Assignee | ||
Comment 1•15 years ago
|
||
Assignee: nobody → m_kato
Attachment #410204 -
Flags: review?(joe)
Comment 2•15 years ago
|
||
Comment on attachment 410204 [details] [diff] [review] patch Two things: 1. Whatever we do in this patch should also be done in the other ClearFrame, just above this one. 2. I think we should make everything conditional on LockImageData() returning success. We can then do an NS_ABORT_IF_FALSE(surf, "Null surface"). Thanks for this catch - as far as I can tell, this should only fail in OOM conditions, which is why we wouldn't run into it regularly.
Attachment #410204 -
Flags: review?(joe) → review-
Comment 3•15 years ago
|
||
This is an updated version of Makoto's patch. I saw crashes in this location in attachments on bug 289763, and so we should probably get this into 1.9.2 as well.
Attachment #410204 -
Attachment is obsolete: true
Attachment #412746 -
Flags: review?(jmuizelaar)
Attachment #412746 -
Flags: review?(bobbyholley)
Comment 4•15 years ago
|
||
Comment on attachment 412746 [details] [diff] [review] handle ClearFrame failure, v2 Why is NS_ABORT_IF_FALSE removed?
Attachment #412746 -
Flags: review?(jmuizelaar) → review-
Comment 5•15 years ago
|
||
Removing NS_ABORT_IF_FALSE there is correct (because it can come up in regular usage), but unrelated to this bug. Removed that hunk.
Attachment #412746 -
Attachment is obsolete: true
Attachment #412773 -
Flags: review?(jmuizelaar)
Attachment #412773 -
Flags: review?(bobbyholley)
Attachment #412746 -
Flags: review?(bobbyholley)
Comment 6•15 years ago
|
||
Comment on attachment 412773 [details] [diff] [review] v3 I don't really like adding the additional if (surf) checks but palleted images will return NS_OK from LockImageData(). Joe tells me that will be fixed on trunk, so given a bug number for that you have your r+
Attachment #412773 -
Flags: review?(jmuizelaar) → review+
Updated•15 years ago
|
Flags: blocking1.9.2?
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Updated•15 years ago
|
Priority: -- → P1
Comment 7•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/04216d317653 Thanks, Makoto!
status1.9.2:
--- → final-fixed
Comment 8•15 years ago
|
||
Attachment #413116 -
Flags: review?(jmuizelaar)
Attachment #413116 -
Flags: review?(bobbyholley)
Comment 9•15 years ago
|
||
Comment on attachment 413116 [details] [diff] [review] v3, mozilla-central Some tests would be nice.
Attachment #413116 -
Flags: review?(jmuizelaar) → review+
Comment 10•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5aadb09fdc70 basic crashtests also committed: http://hg.mozilla.org/mozilla-central/rev/9ee50f360dd4
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Attachment #412773 -
Flags: review?(bobbyholley)
Updated•15 years ago
|
Attachment #413116 -
Flags: review?(bobbyholley)
Comment 11•14 years ago
|
||
did it again? http://crash-stats.mozilla.com/report/index/d28ef8d9-242d-4fc4-b17d-23cfa2110103
Comment 12•14 years ago
|
||
Same here: http://crash-stats.mozilla.com/report/index/94b54949-9673-4c20-a478-09a3f2110104
Comment 13•14 years ago
|
||
This bug is fixed. I filed bug 622886 for the spike.
Updated•13 years ago
|
Crash Signature: [@gfxContext::gfxContext(gfxASurface*) ]
You need to log in
before you can comment on or make changes to this bug.
Description
•