1774201 - If GPU process dies before InitRendering then subsequent ReinitRendering fails

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jamie Nicol [:jnicol]]

I discovered this while investigating bug 1772839. I commented out the DestroyEGLContext line here. This means that when you switch to a new tab, pausing the old compositor does not destroy the EGL surface. Therefore when we attempt to resume the new compositor, it fails, and we end up restarting the GPU process. This could occur in the wild if we encounter a WebRender error during initialization.

Because the GPU process has died while the content process is in the middle of RecvInitRendering, creating the WebRenderLayerManager fails, and we set mLayersConnected = Some(false) here.

Once the GPU process has been restarted, we call ReinitRendering. However, due to this line (introduced in bug 1728062) we exit early. This means the new tab forever remains in an unusable state.

If we changed the early-exit condition to if (mLayersConnected.isNothing()) that would fix this. (As the value is Some(false)). Sotaro, do you remember if doing that would still fix bug 1728062?

Comment 1

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

Set release status flags based on info from the regressing bug 1728062

Comment 2

[Jamie Nicol [:jnicol]]

Attachment: Bug 1774201 - Stop skipping ReinitRendering if previous attempt failed. r?#gfx-reviewers

In bug 1728062 we made it so that we that we skip
BrowserChild::ReinitRendering if the BrowserChild is not connected to
a compositor. This was in order to avoid initializing the compositor
for windowless browsers.

However, in cases where the GPU process dies before an initial
InitRendering has completed, then the BrowserChild will also be left
not connected to a compositor, with mLayersConnected == Some(false).
ReinitRendering will be called once the new GPU process has been
launched, but due to this condition we will exit early, and the tab
will be left in an unusable state.

To fix this, this patch changes the early return condition to only
check for mLayersConnected.isNothing(), ie we never even attempted to
initialize rendering. When it is Some(false), ie we attempted and
failed, then ReinitializeRendering is still executed.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc564899ced4
Stop skipping ReinitRendering if previous attempt failed. r=gfx-reviewers,aosmond

Comment 4

[Sotaro Ikeda [:sotaro OOO July/27 - July/31]]

(In reply to Jamie Nicol [:jnicol] from comment #0)

If we changed the early-exit condition to if (mLayersConnected.isNothing()) that would fix this. (As the value is Some(false)). Sotaro, do you remember if doing that would still fix bug 1728062?

Hmm, it does not fix bug 1728062:( The problem is that "mLayersConnected == Some(false)" has the following 2 meanings.

• [1] parent side does not request to connect layers.
• [2] parent side requested to connect layers, but the connect was failed.

We need to distinguish between [1] and [2]. And also need to take care crash before InitRendering.

Comment 5

[Sotaro Ikeda [:sotaro OOO July/27 - July/31]]

Attachment: Bug 1774201 - Make sure if layers connection is requested by parent

Comment 6

[Jamie Nicol [:jnicol]]

Thanks Sotaro. Should we back out my patch and land yours instead?

Comment 7

[Sotaro Ikeda [:sotaro OOO July/27 - July/31]]

Sorry for my slow reply. It seems not necessary. I am going to create a new bug for the patch soon.

Comment 8

[Jamie Nicol [:jnicol]]

Sounds good. thanks!

Comment 9

[Sandor Molnar[:smolnar]]

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

Comment 10

[Jamie Nicol [:jnicol]]

I think we should uplift this (and bug 1774815). I'm concerned that due to bug 1768197 users could be left with blank unusable tabs as a result.

Comment 11

[Jamie Nicol [:jnicol]]

Comment on attachment 9281797 [details]
Bug 1774201 - Stop skipping ReinitRendering if previous attempt failed. r?#gfx-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Users could be left with blank unusable tabs if they encounter a webrender error when initializing a new tab. The patch from bug 1774815 is also required as this patch alone will regress bug 1728062
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1774815
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Ensures we reinitialize the compositor in more circumstances. This was the old behaviour and worked correctly.
• String changes made/needed:
• Is Android affected?: Yes

Comment 12

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9281797 [details]
Bug 1774201 - Stop skipping ReinitRendering if previous attempt failed. r?#gfx-reviewers

We are out of the beta cycle and releng is currently merging mozilla-beta to mozilla-release before we build our RC. Morphing this into a release build uplift request.

Comment 13

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9281797 [details]
Bug 1774201 - Stop skipping ReinitRendering if previous attempt failed. r?#gfx-reviewers

The impact of the bug in comment #10 looks much more serious to me than the S3 severity set, I am taking it in our RC build, thanks!

Comment 14

[Pascal Chevrel (relman team) -> :pascalc]

https://hg.mozilla.org/releases/mozilla-release/rev/be04ddd1ac82

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr102/rev/d0658216dba8