1749745 - If compositor is initialized without valid Surface on Android then nothing gets rendered subsequently

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Jamie Nicol [:jnicol]]

In bug 1747116 we were seeing crashes caused by the compositor being reinitialized while the android Surface is invalid.

D135117 attempted to prevent this situation occurring. It was uplifted to 96, but doesn't seem to have reduced crash numbers.

D135118 guarded against the crash if we find ourselves in this situation, by simply not resuming the compositor if we detect the surface. This does indeed fix the crash, but I believe may cause nothing to be rendered after the user has resumed the compositor properly. (Until the user subsequently resizes the window, eg opens the keyboard).

The patch makes it so that instead of calling ANativeWindow_getWidth/Height() for every AndroidCompositorWidget::GetClientSize() call, we instead only do so in AndroidCompositorWidget::OnResumeComposition() and cache the result. When this gets called with an invalid Surface, the size therefore remains at the default of 0x0. Webrender subsequently receives a display list and queries the widget size, and therefore sets the document view rect to an empty rect here. This means when the compositor eventually does get resumed properly, webrender believes there is nothing to render. I think this also makes APZ not work, meaning scrolling around does not cause a new display list to be sent, so we get stuck in this state. The only way out of it I've found is the force the window size to change, eg by opening the keyboard.

We can avoid this by initializing the AndroidCompositorWidget with a default size.

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: Bug 1749745 - Initialize AndroidCompositorWidget with initial size. r?aosmond

Since bug 1747116 landed, if the compositor is reinitialized whilst
the Android Surface is invalid, we avoid crashing when querying the
window size and instead keep the compositor in a paused state.
However, in this case we will believe the widget size is 0x0 until the
compositor is eventually resumed. If webrender receives a display list
during this time, it will set an empty view rect. This means when the
compositor is subsequently resumed webrender believes it has nothing
to render, and we get stuck in a state where nothing is ever rendered
to the screen.

This patch initializes the AndroidCompositorWidget with an initial
size, which avoids the problem.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac3d2fbba1fb
Initialize AndroidCompositorWidget with initial size. r=aosmond,geckoview-reviewers,agi

Comment 3

[Atila Butkovits]

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

Comment 4

[Jamie Nicol [:jnicol]]

We should uplift this to beta, as it was regressed by bug 1747116 D135118. Once that's had a few days to settle we should uplift it to release too along with D135118. (The other patch from bug 1747116 - D135117 - was already uplifted to release, but didn't seem to reduce the number of crashes)

Comment 5

[Jamie Nicol [:jnicol]]

Comment on attachment 9258681 [details]
Bug 1749745 - Initialize AndroidCompositorWidget with initial size. r?aosmond

Beta/Release Uplift Approval Request

• User impact if declined: Users who were previously hitting the crash from bug 1747116 will no have their browser freeze and not render anything
• 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): Minor change - initializes the window size earlier to a known good value
• String changes made/needed:

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9258681 [details]
Bug 1749745 - Initialize AndroidCompositorWidget with initial size. r?aosmond

Approved for Fenix 97.0.0-beta.2.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8edd0d6b641b

Comment 8

[Jamie Nicol [:jnicol]]

Comment on attachment 9258681 [details]
Bug 1749745 - Initialize AndroidCompositorWidget with initial size. r?aosmond

Beta/Release Uplift Approval Request

• User impact if declined: Crashes/freezes for android users
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1747116
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minor change - initializes the window size earlier to a known good value
• String changes made/needed:

Comment 9

[Dianna Smith [:diannaS]]

Comment on attachment 9258681 [details]
Bug 1749745 - Initialize AndroidCompositorWidget with initial size. r?aosmond

Approved for 96.0.2

Comment 10

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-release/rev/4950158ef362