Fix OOM crash in gfxAlphaBoxBlur::Init on large blur surface

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

({crash})

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32 wontfix, firefox33+ fixed, firefox34+ fixed, firefox35+ fixed, firefox-esr3133+ fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed, fennec32+)

Details

Attachments

(1 attachment)

Crashed here trying to debug bug 1028802. Despite the stacks reported for bug 1028802 being a bit screwy, I don't think this is that actual crash, the stack being:

  mozalloc_abort
  mozalloc_handle_oom
  moz_xmalloc
  operator new [] (size=17683123) at ../../dist/include/mozilla/mozalloc.h:213
  gfxAlphaBoxBlur::Init 
  gfxAlphaBoxBlur::BlurRectangle
  nsContextBoxBlur::BlurRectangle
Posted patch patchSplinter Review
Attachment #8490051 - Flags: review?(bas)
Blocks: 1028802
jwatt, do you have a crash report submission on this? If not, what product and version does the crash appear in? Bug 1028802 is the largest issue we have right now in 33 Beta for Android, if this fix might affect that case, we should push for uplifts there.
Keywords: crash
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #2)
> jwatt, do you have a crash report submission on this?

No, this was in a debug build with the crash reported disabled so that I could trap crashes in gdb.

> If not, what product
> and version does the crash appear in? Bug 1028802 is the largest issue we
> have right now in 33 Beta for Android, if this fix might affect that case,
> we should push for uplifts there.

As noted in this bug's description this was while debugging bug 1028802, so I was using 33 beta branch for Android.

I intend to request approval for aurora and beta.
[Tracking Requested - why for this release]:
tracking-fennec: --- → ?
How far back does this go? Does it impact ESR31?
Flags: needinfo?(jwatt)
[Tracking Requested - why for this release]:

(In reply to Lawrence Mandel [:lmandel] from comment #5)
> How far back does this go? Does it impact ESR31?

http://hg.mozilla.org/releases/mozilla-esr31/file/07f342aeb78c/gfx/thebes/gfxBlur.cpp#l62

Seems so.
Flags: needinfo?(jwatt)
Attachment #8490051 - Flags: review?(bas) → review+
Comment on attachment 8490051 [details] [diff] [review]
patch

This has been on inbound long enough that it will be merged to central at next merge.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): long existing bug
User impact if declined: possibe fix for fennec topcrash bug 1028802, and likely a crash on other products too
Testing completed: been on inbound for a while with no backout
Risk to taking this patch (and alternatives if risky): low. Makes us take an established error path on OOM.
String or UUID changes made by this patch: none
Attachment #8490051 - Flags: approval-mozilla-esr31?
Attachment #8490051 - Flags: approval-mozilla-beta?
Attachment #8490051 - Flags: approval-mozilla-b2g32?
Attachment #8490051 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/56aa6f151467
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8490051 [details] [diff] [review]
patch

Taking this patch for aurora and beta.
"possibe fix for fennec topcrash bug 1028802" suggests that it is android only. If that is the case, why should we uplift that to esr? Thanks
Attachment #8490051 - Flags: approval-mozilla-beta?
Attachment #8490051 - Flags: approval-mozilla-beta+
Attachment #8490051 - Flags: approval-mozilla-aurora?
Attachment #8490051 - Flags: approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #10)
> Taking this patch for aurora and beta.
> "possibe fix for fennec topcrash bug 1028802" suggests that it is android
> only. If that is the case, why should we uplift that to esr? Thanks

The code is generic and used on all platforms, so the crash can happen on any platform. Bug 1028802 is specifically about fennec, and assuming that the issue there is indeed being caused by this OOM bug then it is more likely to happen on more memory constrained devices. Still, it can happen on ESR.
I haven't looked at data to know if and how much this would happen there, but remember that the updates we still ship to Android 2.2/ARMv6 come from 31 ESR.
tracking-fennec: ? → 32+
Attachment #8490051 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Comment on attachment 8490051 [details] [diff] [review]
patch

Making this exception on uplifting this to 2.0 (b2g32) as this helps us get stability and avid crashes..
Attachment #8490051 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Unable to verify this issue. 

There is no STR available for QA Team to test with.
QA Whiteboard: [QAnalyst-Triage?] [QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] [QAnalyst-verify-] → [QAnalyst-Triage+] [QAnalyst-verify-]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.