All users were logged out of Bugzilla on October 13th, 2018

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

VERIFIED FIXED in Firefox 34, Firefox OS v2.1

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: khuey, Assigned: milan)

Tracking

({assertion})

unspecified
mozilla35
assertion
Points:
---

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Clicking on songs to try to play them in the music app asserts about 1/3rd of the time for me.
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
Assignee: nobody → bas
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.
Flags: needinfo?(milan)
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.
Flags: needinfo?(milan)
(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.
blocking-b2g: 2.1? → 2.1+
Assignee: nobody → milan
Created attachment 8488774 [details] [diff] [review]
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.
Attachment #8488774 - Flags: review?(bas)
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.
Flags: needinfo?(khuey)
I don't have a stack handy, and I haven't seen this in a while.  I keep hitting bug 1061954 instead.
Flags: needinfo?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Attachment #8488774 - Attachment is obsolete: true
Attachment #8488774 - Flags: review?(bas)
Created attachment 8490046 [details] [diff] [review]
Part 1: Be consistent to where we report tiles lost. r=nsilva
Attachment #8490046 - Flags: review?(nical.bugzilla)
Created attachment 8490047 [details] [diff] [review]
Part 2: Shrink to maximum, rather than minimum size, and keep mOutstandingClients consistent at all times. r=nsilva
Attachment #8490047 - Flags: review?(nical.bugzilla)

Updated

4 years ago
Attachment #8490046 - Flags: review?(nical.bugzilla) → review+
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!
Attachment #8490047 - Flags: review?(nical.bugzilla) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e6ae62618df
https://hg.mozilla.org/mozilla-central/rev/9a997d743553
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Please request Aurora approval on these patches when you get a chance.
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → fixed
Flags: needinfo?(milan)
Attachment #8490046 - Flags: approval-mozilla-aurora?
Flags: needinfo?(milan)
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]:
Attachment #8490047 - Flags: approval-mozilla-aurora?
The same uplift reasoning for both patches - both need uplift.
Comment on attachment 8490046 [details] [diff] [review]
Part 1: Be consistent to where we report tiles lost. r=nsilva

Aurora+
Attachment #8490046 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8490047 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.