imgContainer::ClearFrame() should check whether surface is null or not (Crash [@gfxContext::gfxContext(gfxASurface*) ])

RESOLVED FIXED

Status

()

P1
critical
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

({crash})

Trunk
x86
Windows Vista
crash
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta4-fixed)

Details

(crash signature, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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

9 years ago
Created attachment 410204 [details] [diff] [review]
patch
Assignee: nobody → m_kato
Attachment #410204 - Flags: review?(joe)
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-

Updated

9 years ago
Severity: normal → critical
Keywords: crash
Created attachment 412746 [details] [diff] [review]
handle ClearFrame failure, v2

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 on attachment 412746 [details] [diff] [review]
handle ClearFrame failure, v2

Why is NS_ABORT_IF_FALSE removed?
Attachment #412746 - Flags: review?(jmuizelaar) → review-
Created attachment 412773 [details] [diff] [review]
v3

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 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+
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/04216d317653

Thanks, Makoto!
status1.9.2: --- → final-fixed
Created attachment 413116 [details] [diff] [review]
v3, mozilla-central
Attachment #413116 - Flags: review?(jmuizelaar)
Attachment #413116 - Flags: review?(bobbyholley)
Comment on attachment 413116 [details] [diff] [review]
v3, mozilla-central

Some tests would be nice.
Attachment #413116 - Flags: review?(jmuizelaar) → review+
http://hg.mozilla.org/mozilla-central/rev/5aadb09fdc70

basic crashtests also committed: http://hg.mozilla.org/mozilla-central/rev/9ee50f360dd4
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Attachment #412773 - Flags: review?(bobbyholley)
Attachment #413116 - Flags: review?(bobbyholley)

Comment 13

8 years ago
This bug is fixed. I filed bug 622886 for the spike.
Crash Signature: [@gfxContext::gfxContext(gfxASurface*) ]
You need to log in before you can comment on or make changes to this bug.