1372060 - Firefox crashes when visiting https://www.playstation.com/en-us/games/horizon-zero-dawn-ps4

Status: VERIFIED FIXED

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

Description

[Blake Wu [:bwu][:blakewu]]

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

Comment 1

[Blake Wu [:bwu][:blakewu]]

It seems to be related to Gfx and layout.
Snorp,
Is it something your team can help?

Comment 2

[Marcia Knous [:marcia]]

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.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, looks related to my SurfaceTexture changes. I'll take.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 6

[John Lin [:jhlin][:jolin]]

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

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1372060 - Relax assertions when missing SurfaceTexture in compositor

Review commit: https://reviewboard.mozilla.org/r/149910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149910/

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

The attached patch makes me feel really terrible.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8878597 [details]
Bug 1372060 - Relax assertions when missing SurfaceTexture in compositor

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

Ew, but ok.

Comment 11

[Mike Taylor [:miketaylr]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> The attached patch makes me feel really terrible.

You will feel better when you land it.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/421f828a2d9d
Relax assertions when missing SurfaceTexture in compositor r=jgilbert

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/421f828a2d9d

Comment 14

[Mike Taylor [:miketaylr]]

<3

Comment 15

[Julien Cristau [:jcristau]]

This looks like it's (by far) the top crasher for android beta, please request uplift.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 17

[Gerry Chang [:gchang]]

Hi Blake,
Can you help check if this issue is fixed in the latest nightly?

Comment 18

[Julien Cristau [:jcristau]]

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+

Comment 19

[Blake Wu [:bwu][:blakewu]]

It looks like this bug has been fixed since I cannot hit the crash again.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a7e869953401

Comment 22

[Oana Horvath [:ohorvath]]

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)