1278437 - Crash in mozilla::layers::TileClient::GetBackBuffer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ting-Yu Chou [:ting] (away)]

This bug was filed from the Socorro interface and is 
report bp-5d65ceef-9230-4d97-9b73-e1cb82160605.
=============================================================

#4 of Nightly Android 20160605030215, 9 crashes from 5 installations. Occurred also on Mac OS X.

The first report was from build 20160603030242.

Comment 1

[Peter Chang[:pchang]]

This crash was caused by the following gfx critical log.

|[0][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (t=4257.17)

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

It certainly doesn't make sense to call:
mInvalidBack = IntRect(0, 0, mBackBuffer->GetSize().width, mBackBuffer->GetSize().height);
before:
if (!mBackBuffer) {
we're guarding too late.

Same for the mBackBufferOnWhite below.

Speaking of which, should we be using mBackBufferOnWhite instead of mBackBuffer here https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.cpp#687 (together with the null check beforehand of course.)

Bug 1272600 Part 3 seems to have played a part here.

Let's make sure the fix gets uplifted to 49.

Comment 3

[Markus Stange [:mstange]]

I just hit this on OS X.

Comment 4

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] (June 10 PTO) from comment #2)
> It certainly doesn't make sense to call:
> mInvalidBack = IntRect(0, 0, mBackBuffer->GetSize().width,
> mBackBuffer->GetSize().height);
> before:
> if (!mBackBuffer) {
> we're guarding too late.

Indeed, doesn't make sense. mInvalidBack should be set after the nullcheck.

It is quite sad that this is #4 on android, because this can only happen if we fail to allocate the texture which is already very bad in itself.

> 
> Same for the mBackBufferOnWhite below.
> 
> Speaking of which, should we be using mBackBufferOnWhite instead of
> mBackBuffer here
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/
> TiledContentClient.cpp#687

This doesn't really matter, the tile size is the same for the buffers on white and on black (and all other buffers really.

> 
> Bug 1272600 Part 3 seems to have played a part here.

Yes, definitely.

Peter you are assigned to the bug, let me know if you want me to steal it from you. It's a simple fix either way.

Comment 5

[Peter Chang[:pchang]]

Nical, I just assigned this bug to you.

Comment 6

[Nicolas Silva [:nical]]

Attachment: Access mBackBuffer after we have null-checked it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eec36edf2899
Null-check after allocating a tile's texture, before accessing it. r=jnicol

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eec36edf2899

Comment 9

[Nicolas Silva [:nical]]

Comment on attachment 8762686 [details] [diff] [review]
Access mBackBuffer after we have null-checked it.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Crashes when running low and memory.
[Describe test coverage new/current, TreeHerder]: none.
[Risks and why]: none, trivial patch, nul-check.
[String/UUID change made/needed]: none.

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8762686 [details] [diff] [review]
Access mBackBuffer after we have null-checked it.

Crash fix, aurora is affected, let's take it.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6fae46fe5fee

Comment 12

[u279076]

The numbers are dropping but making a note that this was the #8 topcrash in Aurora on Android with 2.04% of the volume.