Closed Bug 1372060 Opened 2 years ago Closed 2 years ago

Categories

(Firefox for Android :: Toolbar, defect, P1)

55 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox54 --- unaffected
firefox55 --- verified
firefox56 --- fixed

People

(Reporter: bwu, Assigned: snorp)

References

Details

(Keywords: crash, regression)

Attachments

(1 file)

Use the latest nightly 55.0a1(2017-06-11) on Google pixel XL.
Visit that website.
scroll down the page. 

It is easy to repro. 

Crash report:
https://crash-stats.mozilla.com/report/index/bp-5cb1c6fd-5c81-4766-a97f-b15320170612
It seems to be related to Gfx and layout.
Snorp,
Is it something your team can help?
Component: Audio/Video → Graphics, Panning and Zooming
Flags: needinfo?(snorp)
Keywords: crash
See Also: → 1370227
I also see some Linux crashes that have gfx/layers/opengl/TextureHostOGL.h:134 in the stack: http://bit.ly/2riNrqd is one example. Wondering if it is the same crash as this one. Happy to file a new bug if one is needed.
Yeah, looks related to my SurfaceTexture changes. I'll take.
Assignee: nobody → snorp
Flags: needinfo?(snorp)
(In reply to Marcia Knous [:marcia - use ni] from comment #2)
> I also see some Linux crashes that have
> gfx/layers/opengl/TextureHostOGL.h:134 in the stack: http://bit.ly/2riNrqd
> is one example. Wondering if it is the same crash as this one. Happy to file
> a new bug if one is needed.

The crash stack in comment #0 cannot happen on Linux, only Android, so we probably need a new bug.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3)
> Yeah, looks related to my SurfaceTexture changes. I'll take.

I assume this refers to bug 1367287, please correct if I'm mistaken.
Blocks: 1367287
Keywords: regression
Priority: -- → P1
Version: unspecified → 55 Branch
Just a wild guess:
It seems GeckoSurfaceTexture::incrementUse() is called [1] on the compositor thread in an asyn IPDL call (PImageBridge::PTexture [2], I assume), could it be that RemoteDataDecoder dispose the surface texture [3] before that?

[1] http://searchfox.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp#425
[2] http://searchfox.org/mozilla-central/source/gfx/layers/ipc/PImageBridge.ipdl#60
[3] http://searchfox.org/mozilla-central/source/dom/media/platforms/android/RemoteDataDecoder.cpp#181
Yeah, that's exactly what happens. We probably had this crash before the ref counting.

Easiest thing is to just remove the MOZ_CRASH() and handle the non-existent SurfaceTexture. Otherwise we'd need to (synchronously?) ref the ST on the content side before the transaction. Ugh.

GFX has solved this problem with other things by having a always-destroy-in-content flag. Something similar for this would be nice, because then we could also kill the idiotic ref counting in the GeckoSurfaceTexture.
The attached patch makes me feel really terrible.
Comment on attachment 8878597 [details]
Bug 1372060 - Relax assertions when missing SurfaceTexture in compositor

https://reviewboard.mozilla.org/r/149910/#review154704

Ew, but ok.
Attachment #8878597 - Flags: review?(jgilbert) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #9)
> The attached patch makes me feel really terrible.

You will feel better when you land it.
Flags: needinfo?(snorp)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/421f828a2d9d
Relax assertions when missing SurfaceTexture in compositor r=jgilbert
https://hg.mozilla.org/mozilla-central/rev/421f828a2d9d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Flags: needinfo?(snorp)
This looks like it's (by far) the top crasher for android beta, please request uplift.
Flags: needinfo?(snorp)
Comment on attachment 8878597 [details]
Bug 1372060 - Relax assertions when missing SurfaceTexture in compositor

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1322650
[User impact if declined]: Crashes
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: See comment #0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only relaxes some assertions
[String changes made/needed]: None
Flags: needinfo?(snorp)
Attachment #8878597 - Flags: approval-mozilla-beta?
Hi Blake,
Can you help check if this issue is fixed in the latest nightly?
Flags: needinfo?(bwu)
Comment on attachment 8878597 [details]
Bug 1372060 - Relax assertions when missing SurfaceTexture in compositor

relax an assertion that's firing on fennec in the wild, beta55+
Attachment #8878597 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
It looks like this bug has been fixed since I cannot hit the crash again.
Flags: needinfo?(bwu)
Duplicate of this bug: 1378822
Verified on Beta 55.0b7 with the specified STR. Didn't get the same crash signature, so this looks like it's fixed. 
But, I did get one crash, logged in Bug 1365828.
Closing this one as fixed.
Build: Beta 55.0b7 (2017-07-07)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.