527555 - crash [@_cairo_atomic_int_dec_and_test]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Josh Aas]

The latest trunk nightly build is crashing a lot in cairo. This crash is from me:

http://crash-stats.mozilla.com/report/index/4182fd14-a347-4dd7-b4df-fd1212091109

I can reproduce this consistently by going to "Bugs filed by me" in bugzilla and clicking on the link that sorts the list by bug number.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks related to Core Text

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: avoid possible use of uninitialized pointers in gfxCoreTextFont destructor

I haven't managed to reproduce this crash locally, but inspection of the code shows that gfxCoreTextFont is unsafe: in the case where InitMetrics decides the font is not valid (if ATSFontGetHorizontalMetrics fails), several member variables are left uninitialized, with the result that the gfxCoreTextFont destructor might do bad things.

The attached patch addresses this, by ensuring the member variables are initialized to NULL, and checking in the destructor before calling Cairo or CoreFoundation cleanup functions (that are not documented to safely accept NULL).

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 411307 [details] [diff] [review]
avoid possible use of uninitialized pointers in gfxCoreTextFont destructor

Thanks!!

     cairo_font_options_t *fontOptions = cairo_font_options_create();
+        // if this fails (out of memory), the pointer is still safe to use
+        // although we're almost certainly going to fail below anyway

Fix indent

Comment 4

[Josh Aas]

Thanks for the quick patch Jonathan - it fixes the crash for me.

Comment 5

[Josh Aas]

Do you think this will also fix this (also from me):

http://crash-stats.mozilla.com/report/index/62afd196-59de-4691-8c2e-a86122091110

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to comment #5)
> Do you think this will also fix this (also from me):
> 
> http://crash-stats.mozilla.com/report/index/62afd196-59de-4691-8c2e-a86122091110

Yes, should do.

Comment 7

[Jonathan Kew [:jfkthame]]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/2716228f7ced

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 411307 [details] [diff] [review]
avoid possible use of uninitialized pointers in gfxCoreTextFont destructor

Requesting approval for 1.9.2, as this bug could potentially affect anyone who enables Core Text through about:config. IMO, the fix here is simple, localized, and safe.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 411307 [details] [diff] [review]
avoid possible use of uninitialized pointers in gfxCoreTextFont destructor

Looks to be adding mostly null checks; I do wonder if it's worth taking since it only happens with that about config change.

Jonathan: does this code only get exercised when Core Text is enabled?

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to comment #10)
> (From update of attachment 411307 [details] [diff] [review])
> Looks to be adding mostly null checks; I do wonder if it's worth taking since
> it only happens with that about config change.
> 
> Jonathan: does this code only get exercised when Core Text is enabled?

Yes, it would only apply with Core Text enabled. Hmm, now that I think about it, our official builds of FF 3.6 don't even include the CT support, do they? We're building against the 10.4 SDK on the 1.9.2 branch, which means this will be disabled at configure time.

So for our binaries, this is NPOTB - it's only an issue for people building the 1.9.2 branch on 10.5 or later. Depending on your point of view, we could argue that this means it's relatively unimportant - or that we can safely land it on the branch as it won't affect our official builds anyway.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 411307 [details] [diff] [review]
avoid possible use of uninitialized pointers in gfxCoreTextFont destructor

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.