1609701 - Inactive GeckoSessions break when reloaded

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Arturo Mejia [:amejia]]

This issue came from originally from the Fenix issue #7636, the same steps there can apply here too. To be completely sure that this is not a bug on AC, I created a simple GeckoView app that mimics the Fenix UI for changing the tracking protection settings, and I was able to replicate the same issue with GV in isolation for more details take a look at this video.

Browser / Version
GeckoView Nightly (🦎 7 4.0.20200113093823)

Operating System
Moto G5 Galaxy S8 (Android 7)

Steps to reproduce:
0. Wait until the page is loaded.

1. Tab the "TP SETTINGS" button.
2. Tab the "STRICT" button.
3. Press the back button (Now the page loads without the bottom ad banner).
4. Tab the "TP SETTINGS" button.
5. Tab the "RECOMMENDED" button.
6. Wait at least 3 seconds and press the back button.
7. Try to interact with any web content like clicking a link

Expected Behavior:
The page is refreshed, loaded and it is responsive.

Actual Behavior:
The page is refreshed, but never fully loads and it is unresponsive.

I believe the issue is linked to change the tracking protection setting from "Strict" to "Recommend" and refresh the session while the GeckoView is partially visible.

Comment 1

[Eugen Sawin [:esawin]]

This issue is not related to Content Blocking settings, it's caused by reloading an inactive GeckoSession, here are the simplified STR:

1. Load a page in a GeckoSession.
2. Set the GeckoSession inactive.
3. Reload the inactive GeckoSession.
4. Set the GeckoSession active.

Expected: Page is reloaded and rendered once actived.
Actual: GeckoSession becomes unresponsive.

Comment 2

[Eugen Sawin [:esawin]]

This regression was introduced with bug 1578942 where we started suspending timeouts for inactive sessions.
However, our mechanics around timeout suspension seem sensible.

For Gecko devs:

1. GeckoSession.setActive(false)
   a. browser.docShellIsActive = false
   b. docShell.contentViewer.pausePainting()
   c. content.windowUtils.suspendTimeouts()
2. GeckoSession.reload()
   a. browser.reload();
3. GeckoSession.setActive(true)
   a. browser.docShellIsActive = true
   b. docShell.contentViewer.resumePainting()
   c. content.windowUtils.resumeTimeouts()

With 3.c we end up calling nsGlobalWindowInner::Resume on an inner window with mSuspendDepth == 0 which fails horribly.
It looks like resuming timeouts (nsDOMWindowUtils::ResumeTimeouts -> nsGlobalWindowInner::Resume) expects to only be called on suspended windows and I don't see how that could be enforced given that it's an unconstrained JS API.

Unless there are good reasons that the assumption needs to hold, I'm proposing to just ignore resuming on non-suspended windows.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: Bug 1609701 - [1.1] Ignore resuming of non-suspended windows.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

It is a bit surprising to do reloading on suspended content.

Comment 5

[Eugen Sawin [:esawin]]

(In reply to Olli Pettay [:smaug] from comment #4)

It is a bit surprising to do reloading on suspended content.

On mobile we have to more aggressive with throttling inactive sessions, I don't think we can completely avoid this use case.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

But why reloading suspended content? Why not resume + reload?
I don't even understand how reload + resume is suppose to work.

Comment 7

[Eugen Sawin [:esawin]]

(In reply to Olli Pettay [:smaug] from comment #6)

But why reloading suspended content? Why not resume + reload?
I don't even understand how reload + resume is suppose to work.

This is controlled by the app. In this case the app wants to reload all the inactive sessions because of changed Tracking Protection settings.
GV could resume before reloading by default without changing the active state of the session, but that could have unexpected consequences in terms of dispatched events.

I've proposed to change the behavior in Fenix to not reload inactive sessions, but set a flag on sessions to be reloaded when activated instead. This would also help with avoiding non-scalable load spikes.
However, unless we actively want to prevent this use case (by ignoring the reload or switching the session to active before the reload) we should at least not crash/break the session.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

It is just that we need to audit all the relevant code to see whether reload + resume works.

Comment 9

[Arturo Mejia [:amejia]]

(In reply to Eugen Sawin [:esawin] from comment #7)

(In reply to Olli Pettay [:smaug] from comment #6)

But why reloading suspended content? Why not resume + reload?
I don't even understand how reload + resume is suppose to work.

This is controlled by the app. In this case the app wants to reload all the inactive sessions because of changed Tracking Protection settings.
GV could resume before reloading by default without changing the active state of the session, but that could have unexpected consequences in terms of dispatched events.

I've proposed to change the behavior in Fenix to not reload inactive sessions, but set a flag on sessions to be reloaded when activated instead. This would also help with avoiding non-scalable load spikes.
However, unless we actively want to prevent this use case (by ignoring the reload or switching the session to active before the reload) we should at least not crash/break the session.

I will do the change in Fenix to only reload the session when GeckoView widget is visible.

Comment 10

[Daniel Varga [:dvarga]]

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

Comment 11

[Arturo Mejia [:amejia]]

(In reply to Daniel Varga [:dvarga] from comment #10)

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

Thanks for the patch, does this patch fix the issue? Is there still need for the workaround mentioned in comment #7?

Comment 12

[Eugen Sawin [:esawin]]

(In reply to Arturo Mejia from comment #11)

(In reply to Daniel Varga [:dvarga] from comment #10)

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

Thanks for the patch, does this patch fix the issue? Is there still need for the workaround mentioned in comment #7?

The patch should fix the issue, so you won't need a workaround. However, you might want to rethink reloading all sessions at once. It creates unnecessary strain on the device and Gecko.

Comment 13

[Arturo Mejia [:amejia]]

Thanks Eugen, we may adopt something similar as desktop button "Reload All Tabs"

Comment 14

[Arturo Mejia [:amejia]]

I opened a Fenix issue for re-thinking the use case.

Comment 15

[Chenxia Liu [:liuche]]

Eugen, could we get this uplifted to GV 73? This might be the cause of another Fenix issue we're seeing in Beta. There's already a Fenix fix for it, and we're going to need to spin another GV beta anyway.

Comment 16

[Eugen Sawin [:esawin]]

Comment on attachment 9121550 [details]
Bug 1609701 - [1.1] Ignore resuming of non-suspended windows.

Beta/Release Uplift Approval Request

• User impact if declined: GeckoView becomes unresponsive after reloading an inactive session.
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: Yes
• 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): Gecko used to assert when hitting this path before, with this change it just ignores the request by an early return.
• String changes made/needed: None

Comment 17

[Ryan VanderMeulen (PTO, back July 27)]

Comment on attachment 9121550 [details]
Bug 1609701 - [1.1] Ignore resuming of non-suspended windows.

Fixes a responsiveness issue when loading inactive sessions. Approved for GV73.

Comment 18

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e43babbd706

Comment 19

[Chris Peterson [:cpeterson]]

Move GeckoView::Tracking Protection bugs to the GeckoView::General component.