1534749 - Handle shutdown races between compositor thread and IPDL object creation

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Andrew Osmond [:aosmond] (he/him)]

In a number of places we assume the compositor thread is present, but shutdown may have already begun. We should ensure we actually have a message loop first.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1534749 - Handle shutdown races between the compositor thread and IPDL object setup.

When the compositor thread has begun shutdown, it will spin the event
loop for the main thread until the last CompositorThreadHolder reference
has been released. While spinning, new IPDL objects may be attempted to
be created which depend on the compositor thread; we should check to
ensure the compositor thread is still around before proceeding with
creation. These objects include CompositorManagerParent,
ImageBridgeParent, and VRManagerParent. Additionally there is a very
similar bug between the vsync thread and VsyncBridgeChild.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ef2f5740a7f3765859d90fc3f3394126c60b3d

Comment 3

[Pascal Chevrel:pascalc (PTO until April 26)]

The number of crashes on 67 Beta 3 and 4 is significantly higher than the crashes we had for 66 betas, it also seem that there are startup crashes which means lost users. I am going to track this crash for 67 for a potential uplift.

Andrew, you had a patch in progress 2 weeks ago, do you have an update on this? Thanks

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See How Do You Triage for more information

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37b25f761cae
Handle shutdown races between the compositor thread and IPDL object setup. r=rhunt

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/37b25f761cae

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Andrew, since 67 is marked as affected, should we consider uplifting this to Beta67?

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Also the same question for ESR60.7

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9050616 [details]
Bug 1534749 - Handle shutdown races between the compositor thread and IPDL object setup.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: May experience graphics related shutdown crashes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I do not consider this verified in nightly because of the low reproduction rate in nightly. It does not appear to have made things worse. I'd like to request uplift to beta given it is still early in the cycle to help verify this. If it works well, we can then uplift to ESR.

It is not risky because it is mostly just adding very simple checks for shutdown having begun and skipping new object initialization if so. These objects could never fully initialize anyways given the circumstances, so nothing can truly depend on them. These changes are well contained within their respective modules, and thus should have an impact that is easily understood.

• String changes made/needed: N/A

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9050616 [details]
Bug 1534749 - Handle shutdown races between the compositor thread and IPDL object setup.

~40 crashes a day on beta and some of them are startup crashes, let's take this patch into beta and see if it improves the situation. Uplift approved for 67 beta 7, thanks.

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/169d315489fe

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Andrew, should we also uplift to ESR60.7?

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

Attachment: esr60 uplift patch, v1

Beta has been around long enough for the crash reports to trickle in (2000+ at the time of writing) and none have been observed with the pertinent signatures, so I think uplifting is appropriate. I modified patch as there were conflicts to resolve. I will request for uplift once I see a green try run.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a0d8ce1a003642226c18792de14d809edf7e27

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9055465 [details] [diff] [review]
esr60 uplift patch, v1

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: We have begun seeing this signature on 60.6.1esr and it has been uplified/verified on 67.0b7. It brings the crash volume down to zero on beta. This should fix 3-4 crashes/day at present on ESR.
• User impact if declined: May experience graphics related shutdown crashes.
• Fix Landed on Version: 67.0b7
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It is not risky because it is mostly just adding very simple checks for shutdown having begun and skipping new object initialization if so. These objects could never fully initialize anyways given the circumstances, so nothing can truly depend on them. These changes are well contained within their respective modules, and thus should have an impact that is easily understood.
• String or UUID changes made by this patch: N/A

Comment 15

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

Comment on attachment 9055465 [details] [diff] [review]
esr60 uplift patch, v1

Crash fix, verified in beta; let's take this for 60.7 ESR.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/67298854427b