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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Makoto Kato [:m_kato]]

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

Comment 1

[Makoto Kato [:m_kato]]

Attachment: patch

Comment 2

[Joe Drew (not getting mail)]

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.

Comment 3

[Joe Drew (not getting mail)]

Attachment: 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.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 412746 [details] [diff] [review]
handle ClearFrame failure, v2

Why is NS_ABORT_IF_FALSE removed?

Comment 5

[Joe Drew (not getting mail)]

Attachment: 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.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

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+

Comment 7

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/04216d317653

Thanks, Makoto!

Comment 8

[Joe Drew (not getting mail)]

Attachment: v3, mozilla-central

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 413116 [details] [diff] [review]
v3, mozilla-central

Some tests would be nice.

Comment 10

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/5aadb09fdc70

basic crashtests also committed: http://hg.mozilla.org/mozilla-central/rev/9ee50f360dd4

Comment 11

[Igor Velkov]

did it again? http://crash-stats.mozilla.com/report/index/d28ef8d9-242d-4fc4-b17d-23cfa2110103

Comment 12

[u331436]

Same here: 
http://crash-stats.mozilla.com/report/index/94b54949-9673-4c20-a478-09a3f2110104

Comment 13

[Scoobidiver (away)]

This bug is fixed. I filed bug 622886 for the spike.