1781936 - Important settings are no longer carried over across navigations in GV apps

Status: RESOLVED FIXED

(GeckoView :: Core, defect, P1)

Description

[Thomas Wisniewski [:twisniewski]]

Steps to reproduce:

1. Open Fenix (or GeckoViewExample)
2. Enable strict antitracking/ETP (to enable the content blocker).
3. Navigate to rottentomatoes.com
4. Confirm that tracking content is being blocked. In Fenix this can be easily seen in the lock icon in the URL bar in the details, whereas in GeckoViewExample, a small gap at the top of the page (where an ad will otherwise appear) will remain empty when blocking is working.
5. Navigate on the same tab to twitter.com
6. On Fenix, confirm that tracking content is NOT being blocked as it should be (on GVE there is no simple UI/way to tell).
7. Navigate on the same tab to rottentomatoes.com
8. Confirm that tracking content is still NOT being blocked (see step 4 for details).

Very curiously, not all websites seem to trigger this bug. Twitter.com and Facebook.com do, but others may not. However, once it is broken, the user must toggle a setting in the UI like strict mode (more info on this shortly), or open a new tab to get it working again.

This was originally reported in https://github.com/mozilla-mobile/fenix/issues/26059, and
several of us have been digging around to discover what's happening. We're not sure when the bug regressed, or what regressed it; it looks like it may have been a surprisingly long time ago.

Note that this affects more than just the content blocker. These features are also broken:

• the anti-tracking content blocker used in Strict mode (useTrackingProtection flag)
• PWA display mode support (displayMode flag)
• display desktop page mode (viewportMode flag)
• toggle page javascript on/off (allowJavascript flag)
• custom user-agent string (customUserAgent flag)

Evidently browsingContexts need to have these flags set by GeckoView to keep these features working. The only place where this seems to be done is in GeckoViewSettings.onSettingsUpdate(), but this is not called during navigations, only when a tab is first created, or when a setting is changed in the UI.

I've made a trivial patch which also triggers onSettingsUpdate when onLocationChange begins to be handled, which fixes this. I'm not at all sure if it's the best fix here, but it's a start.

As for why this broke, I suspect that one of these things had to be true at some point, otherwise the current code would never have worked:

• onSettingsUpdate was once called each time a navigation happened, and so was able to update the browsingContext.
• The flags were once carried over to new browsingContexts across navigations in some lower-level code, and so did not have to be re-set.
• browsingContexts somehow did not change before, so the flags did not need to be re-set in the first place.

(Special thanks also to owlish, tihuang, and amoya for their help with the investigations here as well!)

Comment 1

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1781936 - ensure that GeckoViewSettings updates browsingContexts onLocationChange, so key features work across navigations

Comment 2

[Arturo Mejia [:amejia]]

I was doing some debugging and I think I found a possible point in time were the functionality broke, AC: 75.0.20210409143104 that contains GV: 89.0a1-20210409092020 this first working version and next AC: 75.0.20210410143112 that contains GV: 89.0a1-20210410091448 is the first bad version after being working. Hopefully this we could help us to track down what was added between 2021-04-09 to 2021-04-10.

From looking at the AC commits there is not much going apart of the GV update. Mozilla central commits

Bad https://hg.mozilla.org/integration/autoland/rev/05337140272cc5fe59e58b9d302fb84eda9acbf3

1.0.2231 (Build #1)
AC: 75.0.20210410143112, 4bfb91577
GV: 89.0a1-20210410091448
AS: 74.0.1
f6e466175f59fef29128e8d7350e6ed0b2c5f71a

Good https://hg.mozilla.org/integration/autoland/rev/7bc2dd06085f53993037b4fc78ee651c5afc7671

1.0.2231 (Build #1)
AC: 75.0.20210409143104, bfaf51479
GV: 89.0a1-20210409092020
AS: 74.0.1
b17b99eae858f123234d01deef0c1cd31fb660ea

Comment 3

[Chris Peterson [:cpeterson]]

Patch looked good to Cathy, but let's get Irene's review next week, too.

We'll want to uplift to Beta 104, but we probably don't need to (or will have time to) uplift to a 103.0.x dot release.

Comment 4

[Arturo Mejia [:amejia]]

I was trying to run the commit range on mozregression but unfortunately Task Cluster doesn't keep builds for more than a year. Also I was try to compile each commit locally but looks like I'm unable to build due to some configurations. But I went manually and review each commit on the range. There is one that fits the exact behaviour that we are seeing after the regression Bug 1703291 - Update root bc state when the root bc changes in the session history. r=smaug I believe this is the one that most likely caused the regression.

Comment 5

[Arturo Mejia [:amejia]]

Hi Emilio,
If you don't mind we would like to loop you in to have more context around your patch. And check if the propose fix doesn't collide/cause issues with your changes.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I don't see how my patch could regress this. It was fixing a window.close() fission issue related to session-history handling (to this check in particular).

It should be easy to confirm whether it's the culprit? The patch should pretty much revert cleanly-ish, and is very small.

I don't think the current GeckoView patch is the right way to go, I'm pretty sure it'd run too late for some things like tracking protection or UA string stuff. The main issue is that these settings are stored on BrowsingContext (link, and BrowsingContext can be replaced due to BFCacheInParent / SHIP.

The right fix is to propagate these field changes when the BrowsingContext is replaced in https://searchfox.org/mozilla-central/rev/1061fae5e225a99ef5e43dbdf560a91a0c0d00d1/docshell/base/CanonicalBrowsingContext.cpp#316

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1781936 - Propagate GeckoView settings on BrowsingContext replacement. r=nika,smaug

This should be a better fix. GeckoView assumes these don't change on .top
unless they are explicitly set.

I don't have an Android build environment handy, but I'll try to confirm.

Also, remove an unused flag while at it.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Can you confirm whether comment 7 fixes the issue? It should but I won't have an android build handy until tomorrow, so if you can confirm earlier it'd be great.

Comment 9

[Arturo Mejia [:amejia]]

Thanks for the patch Emilio. I was able to verified on GeckoView example app and Fenix, that your patch address the issue!

It should be easy to confirm whether it's the culprit? The patch should pretty much revert cleanly-ish, and is very small.

Unfortunately, when I try to run ./mach boostrap on your patch or any commit on that commit range I the following error. For that reason I couldn't verify by running it.

The details of the failure are as follows:

NotImplementedError: Cannot bootstrap GeckoView/Firefox for Android: mozboot.base does not yet implement install_mobile_android_packages()

  File "/Users/arturomejia/Desktop/gecko/python/mozboot/mozboot/mach_commands.py", line 50, in bootstrap
    bootstrapper.bootstrap()
  File "/Users/arturomejia/Desktop/gecko/python/mozboot/mozboot/bootstrap.py", line 447, in bootstrap
    getattr(self.instance, "install_%s_packages" % application)(mozconfig_builder)
  File "/Users/arturomejia/Desktop/gecko/python/mozboot/mozboot/base.py", line 244, in install_mobile_android_packages
    raise NotImplementedError(

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Arturo Mejia from comment #9)

Thanks for the patch Emilio. I was able to verified on GeckoView example app and Fenix, that your patch address the issue!

It should be easy to confirm whether it's the culprit? The patch should pretty much revert cleanly-ish, and is very small.

Unfortunately, when I try to run ./mach boostrap on your patch or any commit on that commit range I the following error. For that reason I couldn't verify by running it.

I meant manually reversing my patch on top of current central. But anyways glad to hear it!

Comment 11

[Kevin Brosnan [Ex-Mozilla]]

The other major bit of work in that range is Arron turned on 2 content processes. Has anyone checked if this behavior does not reproduce in 1 content process?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

That is more likely to cause the regression I think. Browsing context swap happens on process switch IIRC.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b1bd37bbdd4
Propagate GeckoView settings on BrowsingContext replacement. r=nika

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/1b1bd37bbdd4

Comment 15

[Thomas Wisniewski [:twisniewski]]

@Emilio, does this patch also somehow handle the viewportMode variable? If not, we should take care to update it as well, or it will break the toggle for viewing pages like a desktop browser. (And we'll have to take care to do this as well: https://searchfox.org/mozilla-central/source/mobile/android/actors/GeckoViewSettingsChild.jsm#32).

Comment 16

[Thomas Wisniewski [:twisniewski]]

Emilio mentioned to me on Slack that we should make viewportMode a synced flag as well, so I've filed bug 1782751 as a follow-up to do that.

Comment 17

[Chris Peterson [:cpeterson]]

Seems too late to uplift to Beta 104. This fix can ride the trains with 105.