Closed Bug 391243 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

Attached patch Patch rev. 1 (obsolete) — Splinter Review
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.
Flags: blocking1.9?
Attachment #275622 - Flags: superreview?(vladimir)
Attachment #275622 - Flags: review?(vladimir)
Blocks: 390720
Assignee: nobody → mats.palmgren
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?
Flags: blocking1.9? → blocking1.9+
No problem at all, merge them as you see fit.
Should be fixed by bug 390668.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #275622 - Flags: superreview?(vladimir)
Attachment #275622 - Flags: review?(vladimir)
Attached patch Patch 2 (obsolete) — Splinter Review
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.
Attachment #275622 - Attachment is obsolete: true
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.
Attachment #276166 - Flags: review+
Attached patch Patch 3Splinter Review
Attachment #276166 - Attachment is obsolete: true
Attachment #276226 - Flags: superreview?(vladimir)
Attachment #276226 - Flags: review?(vladimir)
Comment on attachment 276226 [details] [diff] [review]
Patch 3

Looks great, thanks!
Attachment #276226 - Flags: superreview?(vladimir)
Attachment #276226 - Flags: superreview+
Attachment #276226 - Flags: review?(vladimir)
Attachment #276226 - Flags: review+
Attachment #276226 - Flags: approval1.9+
This (or bug 390898) has caused bug 392170.
I accidentally checked in the wrong patch at first, it should be fixed now.
mozilla/gfx/thebes/src/gfxWindowsFonts.cpp 	1.139 
Depends on: 392170
Crash Signature: [@ _moz_cairo_status] [@ _moz_cairo_surface_get_type] [@ _cairo_array_num_elements]
You need to log in before you can comment on or make changes to this bug.