1634493 - WebExtension.allowedInPrivateBrowsing is not persisted after the app process gets killed.

Status: RESOLVED FIXED

(GeckoView :: Extensions, defect, P1)

Description

[Simon Chae]

Related fenix issue: https://github.com/mozilla-mobile/fenix/issues/10290

STR:

• Set WebExtension.allowedInPrivateBrowsing to true (false by default)
• Close the app & restart

It appears that if WebExtension.enabled state is changed with allowedInPrivateBrowsing, then the allowedInPrivateBrowsing is persisted.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1634493 - Wait for extension policy to be ready.

When exporting the addon object, the extension policy sometimes is not ready
yet (very common when list() is called at startup), so we need to wait until
it's ready or the values in it will not be correct.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1634493 - Wait for extension policy to be ready.

When exporting the addon object, the extension policy sometimes is not ready
yet (very common when list() is called at startup), so we need to wait until
it's ready or the values in it will not be correct.

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1634493 - Add GVE setting for extensions in private browsing.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65e4e70c6851
Wait for extension policy to be ready. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/4d31941a0eb4
Add GVE setting for extensions in private browsing. r=aklotz

Comment 5

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/65e4e70c6851
https://hg.mozilla.org/mozilla-central/rev/4d31941a0eb4

Comment 6

[Simon Chae]

I'm seeing this bug intermittently now. Here's a reproducible workflow:

1. Set ublock allowPrivateBrowsing to false (from true) and confirm success callback
2. Restart app and check metadata for ublock when runtime.webExtensionController.list() - confirm false
3. Set ublock allowPrivateBrowsing to true (from false) and confirm success callback
4. Restart app and check metadata for ublock when runtime.webExtensionController.list() - still false

2020-05-08 12:14:38.096 14135-14135/org.mozilla.samples.browser D/allowPrivateBrowsing: success: false
2020-05-08 12:14:47.810 14594-14594/org.mozilla.samples.browser D/allowPrivateBrowsing: listInstalledWebExtensions: [e-mail]/false
2020-05-08 12:14:54.268 14594-14594/org.mozilla.samples.browser D/allowPrivateBrowsing: success: true
2020-05-08 12:15:02.545 14858-14858/org.mozilla.samples.browser D/allowPrivateBrowsing: listInstalledWebExtensions: [e-mail]/false

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1634493 - Re-read the policy object when exporting extensions.

WebExtensionPolicy.getByID(id) will return a stub when called earlier during
startup. Once the startup process is complete, this object is sawpped with the
real extension policy.

Before this patch, we used to hold onto the stub object, which makes it so that
we read incorrect values even though we are waiting for the policy to be ready.

To fix this we just read the result value of the readyPromise promise.

Comment 8

[[ex-Mozilla] Agi Sferro | :agi]

Thanks Simon! There was a bug on our side, next nightly should have the fix :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1d0588ee5c5
Re-read the policy object when exporting extensions. r=snorp

Comment 10

[Cosmin Sabou [:CosminS]]

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

Comment 11

[Simon Chae]

I'm still seeing intermittent issue with nightly version (78.0.20200510092917):

log with id is from runtime.webExtensionController.list() and just the boolean value is from runtime.webExtensionController.setAllowedInPrivateBrowsing() success callback

2020-05-11 09:53:41.878 5759-5759/org.mozilla.samples.browser D/++private-browsing++: false
2020-05-11 09:53:43.242 5759-5759/org.mozilla.samples.browser D/++private-browsing++: true
2020-05-11 09:53:53.963 6817-6817/org.mozilla.samples.browser D/++private-browsing++: id: [e-mail] / true
2020-05-11 09:54:03.541 6817-6817/org.mozilla.samples.browser D/++private-browsing++: false
2020-05-11 09:54:16.406 7137-7137/org.mozilla.samples.browser D/++private-browsing++: id: [e-mail] / true
2020-05-11 09:54:25.290 7137-7137/org.mozilla.samples.browser D/++private-browsing++: false
2020-05-11 09:54:36.985 7401-7401/org.mozilla.samples.browser D/++private-browsing++: id: [e-mail] / false
2020-05-11 09:54:43.557 7401-7401/org.mozilla.samples.browser D/++private-browsing++: true
2020-05-11 09:54:54.302 7714-7714/org.mozilla.samples.browser D/++private-browsing++: id: [e-mail] / true
2020-05-11 09:55:05.207 7714-7714/org.mozilla.samples.browser D/++private-browsing++: false
2020-05-11 09:55:14.388 7983-7983/org.mozilla.samples.browser D/++private-browsing++: id: [e-mail] / false
2020-05-11 09:55:20.700 7983-7983/org.mozilla.samples.browser D/++private-browsing++: true
2020-05-11 09:55:28.649 8265-8265/org.mozilla.samples.browser D/++private-browsing++: id: [e-mail] / false```

Comment 12

[[ex-Mozilla] Agi Sferro | :agi]

Simon: do you kill the app quickly after setting the value? The value has to be written to disk so it takes a little bit to persist. We could potentially change this behavior but I think it would be a separate bug.

Comment 13

[Simon Chae]

(In reply to Agi Sferro | :agi | ⏰ PST | he/him from comment #12)

Simon: do you kill the app quickly after setting the value? The value has to be written to disk so it takes a little bit to persist. We could potentially change this behavior but I think it would be a separate bug.

Hmm, how quickly is too quickly? I kill the app after runtime.webExtensionController.setAllowedInPrivateBrowsing() success callback (https://github.com/mozilla-mobile/android-components/blob/master/components/browser/engine-gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngine.kt#L437).

Comment 14

[[ex-Mozilla] Agi Sferro | :agi]

About a few seconds, can you still reproduce if you wait like 10 seconds before restarting the app?

Comment 15

[Simon Chae]

It does appear to be more consistent if I wait x amount of time before restarting the app. Enable/disable works with minimal wait time, is there a difference between how these two values are written to the disk?

Comment 16

[[ex-Mozilla] Agi Sferro | :agi]

Yeah we do have different code paths and some flush earlier than others, I'm addressing that in Bug 1637680, please follow that bug. I'm gonna resolve this in the meanwhile since the problem outlined in this bug is fixed.