1238541 - crash in mozilla::gl::SharedSurface_EGLImage::ProducerReleaseImpl

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

This bug was filed from the Socorro interface and is 
report bp-fd8b28be-dd2b-475f-aae2-58ebc2160111.
=============================================================

No particular STR, seems to happen randomly.

Here is another report: https://crash-stats.mozilla.com/report/index/98fc06c9-f11a-447f-ba7a-cabc82160111

Comment 1

[Szmozsánszky István [:flaki]]

Easily reproducible with today's Fennec Nightly at
http://2016.render-conf.com/
Sometimes the page doesn't crash right away, but after some scrolling around it's a safe bet it eventually will.
(on Lollipop Z3Compact & Marshmallow/Nexus6 either)

Comment 2

[Dietrich Ayala (:dietrich)]

I see this every time when loading: https://www.donmccurdy.com/aframe-gamepad-controls/

Same as Flaki, it sometimes takes a few seconds, but eventually (within 30 sec) will crash.

Comment 3

[Dietrich Ayala (:dietrich)]

Looks like 100% reproducibility now. Any ETA on someone looking into fixing this?

Comment 4

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

Jim, is this also fallout from the recent fennec widget changes? If not we should move it to graphics.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

I don't know for sure, but it doesn't look like it.

Comment 6

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

It's #3 on the Fennec 46 top crasher list over the last 7 days. snorp, do you have cycles to look into this?

Comment 7

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

^

Comment 8

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

Seems to have started with the Jan 7 nightly, which would put the regression range at https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d6ffc7a08b6b47056eefe1e652710a3849adbf7&tochange=e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c

Comment 9

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

Also cc'ing jgilbert since the MOZ_RELEASE_ASSERT it's dying on was added by him in bug 1144906 back in last June.

Comment 10

[Ludovic Hirlimann [:Usul]]

Attachment: Logcat when the crash happens

Crash when loading or scrolling horizontally https://torflow.uncharted.software/

Comment 11

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

Jeff, it looks like readback is kinda busted. We acquire the producer lock to get at the FB, but when we release we may have already had a fence created, hitting this assertion. The comment here[0] says the consumer lock is the best one to use, probably for this reason, but the code does not match.

[0] https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContext.cpp?from=GLContext.cpp#2780

Comment 12

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

Milan, this is a gfx regression

Comment 13

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

Bas, can :jgilbert and you sort this out?  It may be related to pushlayer/poplayer, it's in the range mentioned in comment 8.

Comment 14

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

[Tracking Requested - why for this release]: We don't want new regressions, and this is a frequent crash.

Comment 15

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #13)
> Bas, can :jgilbert and you sort this out?  It may be related to
> pushlayer/poplayer, it's in the range mentioned in comment 8.

This work is preffed off on Android.

Comment 16

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

Since there's a bunch of large and possibly related changes that landed on the day this started, it would help if somebody who can reproduce this can narrow down the range using mozregression.

Comment 17

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #11)
> Jeff, it looks like readback is kinda busted. We acquire the producer lock
> to get at the FB, but when we release we may have already had a fence
> created, hitting this assertion. The comment here[0] says the consumer lock
> is the best one to use, probably for this reason, but the code does not
> match.
> 
> [0]
> https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContext.
> cpp?from=GLContext.cpp#2780

ProducerReadAcquire is the right choice here. (the comment is out-of-date)(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)

> Seems to have started with the Jan 7 nightly, which would put the regression
> range at
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=9d6ffc7a08b6b47056eefe1e652710a3849adbf7&tochange=e0bc
> d16e1d4b99ba3e542149d0d41e0f60c54b5c

It might have to do with bug 1236762.

Comment 18

[Teodora Vermesan (:TeoVermesan)]

I can reproduce this crash using Nexus 6 (Android 6.0) on Firefox for Android 46.0a2 (2016-01-27) after loading get.webgl.org

Comment 19

[Josh Carpenter [:jcarpenter]]

We're getting reports of all demos at mozvr.com (mixture of WebVR demos from around the web) crashing on Firefox Android nightly. Looking into it now. Reporter indicated they believed this bug was the cause.

Comment 20

[Casey Yee]

FWIW, This crash happens fairly often for me (1+One device).   Though, I am unable to reproduce these crashes consistently -- even with the examples posted in the comments above.    These crashes seem to happen seemingly randomly sometimes, and the only common thread from my experience is that it's due to webGL content on the page.

Comment 21

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

Attachment: Don't die in SharedSurface_EGLImage::ProducerReadReleaseImpl() if there is an existing fence

It seems to me the assertion is just wrong in the ProducerReadReleaseImpl() case, which right now is
treated the same as ProducerReleaseImpl(). This patch provides a separate path, and waits on
any existing fence in the ProducerReadAcquireImpl().

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

I see this crash showing up in Fx47 and Fx46

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a85f477cd68f

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Benjamin Kerensa [:bkerensa]]

Jeff, do you want to uplift this to 46?

Comment 26

[Benjamin Kerensa [:bkerensa]]

Tracking because topcrash/regression

Comment 27

[Kelsey Gilbert [:jgilbert]]

(In reply to Benjamin Kerensa [:bkerensa] from comment #25)
> Jeff, do you want to uplift this to 46?

It seems like we should.

Comment 28

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8713686 [details] [diff] [review]
Don't die in SharedSurface_EGLImage::ProducerReadReleaseImpl() if there is an existing fence

Approval Request Comment
[Feature/regressing bug #]: unknown
[User impact if declined]: top-crash
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 29

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

Yup, thanks.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8713686 [details] [diff] [review]
Don't die in SharedSurface_EGLImage::ProducerReadReleaseImpl() if there is an existing fence

Crash fix, please uplift to aurora.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2796ce6aeeaf

Comment 32

[Adina Petridean QA [:apetridean]]

Verified as fixed on the latest Nightly 109.0a1 from 22/11 with Sony Xperia (Android 6.0.1).

Comment 33

[Chris Peterson [:cpeterson]]

Removing regressionwindow-wanted keyword because this bug has been resolved.

Comment 34

[Chris Peterson [:cpeterson]]

Removing regressionwindow-wanted keyword because this bug has been resolved.

Comment 35

[Chris Peterson [:cpeterson]]

Removing regressionwindow-wanted keyword because this bug has been resolved.