759162 - Crash in AndroidGLController::ProvideEGLSurface during CompositorParent::ResumeComposition

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ali Juma [:ajuma]]

This bug is for crashes in ProvideEGLSurface remaining after Bug 745243. The crashes that remain are (almost all) non-startup crashes, occurring during CompositorParent::ResumeComposition.

Comment 1

[Ali Juma [:ajuma]]

Attachment: Part 1: Remove unnecessary call to eglChooseConfig in GLContextProviderEGL::RenewSurface.

We're calling CreateConfig (which calls eglChooseConfig), but doing nothing with the result since GLController.provideEGLSurface on the Java side handles choosing a config for surface creation. This really shouldn't be causing crashes in provideEGLSurface, but we might as well fix it.

Comment 2

[Ali Juma [:ajuma]]

Attachment: Part 2: Make CompositorParent::ScheduleResumeOnCompositorThread block until the compositor resumes.

It's conceivable that we're crashing when we get a rapid sequence of surfaceChanged and surfaceDestroyed events, meaning that the compositor doesn't get around to creating an egl surface until after surfaceDestroyed has occurred. This patch makes us block on surfaceChanged until the compositor is actually resumed (and an egl surface is created).

(Similarly, Bug 752368 made us block on surfaceDestroyed until the compositor is actually paused (and the egl surface is released). That fixed a bunch of crashes.)

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 627786 [details] [diff] [review]
Part 2: Make CompositorParent::ScheduleResumeOnCompositorThread block until the compositor resumes.

Looks good. I don't really like how we call these functions 'Schedule' yet they perform the operation synchronously but we already have another instance of that.

Comment 4

[Ali Juma [:ajuma]]

Attachment: Part 1: Remove unnecessary call to eglChooseConfig in GLContextProviderEGL::RenewSurface, v2

This accomplishes the same thing as the previous version, but does so by entirely doing away with calls to CreateSurfaceForWindow on Native Fennec (so we no longer need to pass in a meaningless EGLConfig to this function) and making calls directly to ProvideEGLSurface instead.

Comment 5

[Ali Juma [:ajuma]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a88b2e20e15f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8df76630105d

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a88b2e20e15f
https://hg.mozilla.org/mozilla-central/rev/8df76630105d

Comment 7

[Ali Juma [:ajuma]]

Comment on attachment 627792 [details] [diff] [review]
Part 1: Remove unnecessary call to eglChooseConfig in GLContextProviderEGL::RenewSurface, v2

[Approval Request Comment]
User impact if declined: Possible crashes.
Testing completed (on m-c, etc.): On m-c since May 30th.
Risk to taking this patch (and alternatives if risky): Low-risk. Because of #ifdefs, the changes made by this patch are effectively mobile-only.
String or UUID changes made by this patch: None.

Comment 8

[Ali Juma [:ajuma]]

Comment on attachment 627786 [details] [diff] [review]
Part 2: Make CompositorParent::ScheduleResumeOnCompositorThread block until the compositor resumes.

[Approval Request Comment] 
User impact if declined: Possible crashes.
Testing completed (on m-c, etc.): On m-c since May 30th.
Risk to taking this patch (and alternatives if risky): Low-risk, affects OMTC only.
String or UUID changes made by this patch: None.

Comment 9

[Ali Juma [:ajuma]]

This appears to be the likely cause of Bug 761003 (a crash on the HTC Vision), so we shouldn't uplift until we get this sorted out.

Comment 10

[Ali Juma [:ajuma]]

(In reply to Ali Juma [:ajuma] from comment #9)
> This appears to be the likely cause of Bug 761003 (a crash on the HTC
> Vision), so we shouldn't uplift until we get this sorted out.

It's worth noting that those crashes appear to be from a single user using cyanogenmod, so it's not clear if the appearance of that crash after the landing of these patches is simply a coincidence, and even if it's not a simple coincidence, it may still be the case that the crashes potentially fixed by these patches outweigh the crashes potentially caused.

Comment 11

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

Comment on attachment 627786 [details] [diff] [review]
Part 2: Make CompositorParent::ScheduleResumeOnCompositorThread block until the compositor resumes.

[Triage Comment]

Comment 12

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

Comment on attachment 627792 [details] [diff] [review]
Part 1: Remove unnecessary call to eglChooseConfig in GLContextProviderEGL::RenewSurface, v2

[Triage Comment]

Comment 13

[Ali Juma [:ajuma]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d8d65f68ee0b
https://hg.mozilla.org/releases/mozilla-beta/rev/206e57393758

Comment 14

[Scoobidiver (away)]

There are still crashes in 14.0b6: bp-159dd1d7-4e51-436c-8f07-35e9b2120608, bp-3622b805-8285-43cf-82e5-ecb1a2120608, bp-65b51bd7-1ddf-4da6-bff1-6e75d2120608, bp-79b6a9b6-03c5-4483-8644-b21992120608.

Comment 15

[Ali Juma [:ajuma]]

Re-nomming since:

1) The crash volume no longer seems to be high enough to justify this being a release blocker. Over the past 7 days in 14.0b5 (which doesn't even include the patches from this bug), there were 118 crashes (accounting for 1.2% of all crashes) with this signature during CompositorParent::ResumeComposition.

2) Since we don't have steps to reproduce this, it's not clear how much further progress can be made here.

Comment 16

[Ali Juma [:ajuma]]

> Over the past 7 days in 14.0b5 (which doesn't even
> include the patches from this bug), there were 118 crashes (accounting for
> 1.2% of all crashes) with this signature during
> CompositorParent::ResumeComposition.

This doesn't include the roughly 100 crashes with the JNI_GetCreatedJavaVMs signature. So the combined crashes actually accounted for roughly 2.2% of all crashes in 14.0b5. However, there are no JNI_GetCreatedJavaVMs crashes yet in 14.0b6 (which includes the patches from this bug).

Comment 17

[Scoobidiver (away)]

In the first hours of 14.0b6, this bug and bug 745243 account for 3.8% of all crashes and are #2 top crasher. We need to wait more stats before unblocking.

Comment 18

[Joe Drew (not getting mail)]

OK. We think there might be something here, but we don't have enough information yet. We also want to separate this out for proper tracking.

Scoobidiver, can you file (and nom) a bug regarding this crash, so we can re-evaluate on Monday?