391243 - Various crashes [@ _moz_cairo_status] [@ _moz_cairo_surface_get_type] [@ _cairo_array_num_elements]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

We have many reported Breakpad crashes with _moz_cairo* on the stack,
a number of these seems to be a null surface or possibly a surface
that we failed to setup properly (CairoStatus() != 0).

https://crash-reports.mozilla.com/reports/?do_query=1&query_search=stack&query_type=contains&query=_moz_cairo&date=&range_value=3&range_unit=months
(only some of these are due to this bug of course)

The patch adds some error handling that avoids crashing in this situation.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 275622 [details] [diff] [review]
Patch rev. 1

I have a patch for this in bug 390668, though I like your bulletproofing more than mine.  Mind if I just integrate the two when I get review from stuart later today?

Comment 2

[Mats Palmgren (inactive)]

No problem at all, merge them as you see fit.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Should be fixed by bug 390668.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch 2

Sorry to bother you again Vlad, but there are a couple of places that
still can fail which I think we should handle:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/thebes/src/gfxWindowsFonts.cpp&rev=1.137&root=/cvsroot&mark=83,85#80

1. CurrentSurface() can return nsnull (which has been seen in other bugs).
2. surface->GetDC() can return NULL;  it just returns mDC, which is assigned
from ::GetDC() or ::CreateCompatibleDC() which return NULL when they fail.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 276166 [details] [diff] [review]
Patch 2

Could've sworn I just commented on this, but apparently not.

Yeah, this looks fine; in theory aSurface should never be null -- can you add a NS_ASSERTION to that effect in DCFromContext?  Also, i'd much rather see something like:
dc = nsnull;
if (... Win32) {
  dc = blah;
  needsRelease = PR_FALSE;
}

if (!dc) {
  dc = GetDC(NULL);
  needsRelease = PR_TRUE;
}

instead of the early return and the nested if.  But r+ me with that.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch 3

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 276226 [details] [diff] [review]
Patch 3

Looks great, thanks!

Comment 8

[Steve England [:stevee]]

This (or bug 390898) has caused bug 392170.

Comment 9

[Mats Palmgren (inactive)]

I accidentally checked in the wrong patch at first, it should be fixed now.
mozilla/gfx/thebes/src/gfxWindowsFonts.cpp 	1.139