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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

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

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Kaiser]

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.

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Robert Kaiser (:[e-mail]) 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.

Comment 4

[Jonathan Watt [:jwatt]]

[Tracking Requested - why for this release]:

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

How far back does this go? Does it impact ESR31?

Comment 6

[Jonathan Watt [: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.

Comment 7

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56aa6f151467

Comment 8

[Jonathan Watt [:jwatt]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56aa6f151467

Comment 10

[Sylvestre Ledru [:Sylvestre]]

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

Comment 11

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-beta/rev/023a362fab21
https://hg.mozilla.org/releases/mozilla-aurora/rev/f61e00dd4325

Comment 12

[Jonathan Watt [:jwatt]]

(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.

Comment 13

[Robert Kaiser]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/246f5fd728b3

Comment 15

[bhavana bajaj [:bajaj]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/d81f4f069976

Comment 17

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/d81f4f069976

Comment 18

[Duane Dixon [:ddixon]]

Unable to verify this issue. 

There is no STR available for QA Team to test with.