1048984 - Music app crashes frequently by asserting mOutstandingClients >0 at TextureClientPool.cpp:143

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Clicking on songs to try to play them in the music app asserts about 1/3rd of the time for me.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

[Blocking Requested - why for this release]:

Comment 2

[bhavana bajaj [:bajaj]]

Ni :milan, since this has nominated over a month for 2.1. Not sure we would block a release on this assertion issue but could take a fix before FC.

Comment 3

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

If it's just in debug, I wouldn't block on it, but I would like to keep considering this and if the fix ends up being appropriate, take an uplift.

Comment 4

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

(In reply to Milan Sreckovic [:milan] from comment #3)
> If it's just in debug, I wouldn't block on it, but I would like to keep
> considering this and if the fix ends up being appropriate, take an uplift.

I take that back, pays to look at the code. The value is unsigned int and this assertion precedes --, so we'd end up with a very large number after this assertion and it'd probably just be a question of time before we crash somewhere.

Comment 5

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

One problem at least is calling ShrinkToMaximumSize(http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClientPool.cpp#57) after prematurely increasing mOutstandingClients http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClientPool.cpp#45

Comment 6

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

Attachment: Keep the mOutstandingClient value consistent.

The actual logic change is in TextureClientPool::GetTextureClient - the rest of the code are asserts and comments.

I'm not sure this actually fixes the original problem, as I couldn't reproduce it myself, but it is at least a potential bug.

Comment 7

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

Pretty sure the above wouldn't fix the problem, given that we'd run into this assertion when mOutstandingClients is 0, but mTextureClientsDeferred size is not, I assume in a call to ReturnTextureClientDeferred().  That likely means that ReportClientLost() was called earlier, to get us out of sync in the first place.

Kyle, you don't have a stack when this asserts?  Just want to double check that it is ReturnTextureClientDeferred() that's the second in the stack, although I'm not sure that's very relevant, the trouble is likely caused earlier.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't have a stack handy, and I haven't seen this in a while.  I keep hitting bug 1061954 instead.

Comment 9

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #8)
> I don't have a stack handy, and I haven't seen this in a while.  I keep
> hitting bug 1061954 instead.

Thanks. I'm going to assume it's the combination of ReportClientLost and changing gfxContentType that conspire to mess up the count of outstanding tiles and post a patch that deals with it.

Comment 10

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

Attachment: Part 1: Be consistent to where we report tiles lost. r=nsilva

Comment 11

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

Attachment: Part 2: Shrink to maximum, rather than minimum size, and keep mOutstandingClients consistent at all times. r=nsilva

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8490047 [details] [diff] [review]
Part 2: Shrink to maximum, rather than minimum size, and keep mOutstandingClients consistent at all times. r=nsilva

Review of attachment 8490047 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the added documentation!

Comment 13

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

Try was run on a combined patch, otherwise only trivial changes, so I'd like to "reuse" it:
https://tbpl.mozilla.org/?tree=Try&rev=b462806339e3.

The patches should apply cleanly to Aurora, but let me know if there are problems.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6ae62618df
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a997d743553

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7e6ae62618df
https://hg.mozilla.org/mozilla-central/rev/9a997d743553

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on these patches when you get a chance.

Comment 17

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

Comment on attachment 8490047 [details] [diff] [review]
Part 2: Shrink to maximum, rather than minimum size, and keep mOutstandingClients consistent at all times. r=nsilva

Approval Request Comment
[Feature/regressing bug #]: 1046025
[User impact if declined]: Scenarios where we switch from transparent content to opaque, or vice versa, could cause problems with the tiling system.
[Describe test coverage new/current, TBPL]:
[Risks and why]: Low
[String/UUID change made/needed]:

Comment 18

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

The same uplift reasoning for both patches - both need uplift.

Comment 19

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

Comment on attachment 8490046 [details] [diff] [review]
Part 1: Be consistent to where we report tiles lost. r=nsilva

Aurora+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/51f29a050e63
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba00e8f04fa6

Comment 21

[Cody Roesch [:croesch]]

This bug is verified fixed in Flame 2.2 KK and 2.1 KK. Tested with Fullflash and 319mb.

No assert occurs and the Music app does not crash out when playing many different songs while accessing them from different screens within the Music app. Reading the logcat also shows no signs of asserts.

Device: Flame 2.2 KK
BuildID: 20141013040202
Gaia: 3b81896f04a02697e615fa5390086bd5ecfed84f
Gecko: f547cf19d104
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
-----------------------------------------------------------------
Device: Flame 2.1 KK
BuildID: 20141013001201
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0