1525846 - “Run in Private Windows” not switching correctly after restarting the browser with the detailed page of the extension open

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[CosminB]

Attachment: BugRadioButtons.gif

[Affected versions]:

• Firefox 67.0a1 (20190206215551)

[Affected platforms]:

• Win 7 64-bit
• Mac OS X 10.14.1

[Steps to reproduce]:

1.Install:https://addons.mozilla.org/en-US/firefox/addon/sidebar_plus/?src=search
2.Make sure ‘extensions.allowPrivateBrowsingByDefault’ is set to false.
3.In about:addons navigate to the detailed page of the extension.
4.Check if “Don’t Allow” radio buttons is checked.
5.Restart the browser with the detailed page open.
6.Switch to “Allow” and observe the radio buttons.

[Expected results]:

• The “Allow” buttons is checked successfully.

[Actual results]:

• After the “Allow” button is checked it switches back to “Don’t Allow” at the first attempt.

[Notes]

• Only from the second attempt, it switches to “Don’t Allow”.
• Browser Console: “Console was cleared. Background.js:2:1”
• The same rule applies if you restart the browser with “Allow” checked and you try to switch to “Don’t Allow”.
• Disabled extensions are not affected by this behavior.

Please see the attached video.

Comment 1

[Shane Caraveo (:mixedpuppy)]

What I've noticed is:

• I can only reproduce if the addon manager is reopened on startup (session restore) to the extension being tested
• The extension must be enabled (prior to restart)
• It intermittently fails, but fails more than not.
• It fails only the first time the permission is toggled
• When it fails, _lazyInit in ExtensionPermissions.jsm has NOT been called

If I add:

await ExtensionPermissions._get(id)

prior to attempting an add/remove, it never fails. "_get" bypasses the StartupCache used by get, forcing a call to _lazyInit.

@aswan, seems like some kind of race condition between EP and StartupCache. Any thoughts here?

Comment 2

[Shane Caraveo (:mixedpuppy)]

Figured out a solution after chatting with aswan.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1525846 ensure ExtensionPermissions is fully initialized prior to changing

Comment 4

[Rob Wu [:robwu]]

This is happening:

1. ExtensionPermission.add retrieves a reference to an object from the StartupCache (via ._getCached).
2. Then it adds permissions to it, and calls _saveSoon.
3. ExtensionPermissions.add returns and the extension is reloaded.
   Part of reloading is shutting down, which clears StartupCache.

Step 2 and 3 happens in parallel. During step 2, the following async task is initiated:

4. _saveSoon calls lazyInit to read the JSON file.
5. _saveSoon retrieves the latest value of the preferences, via _getCached.
6. _saveSoon updates the JSON file with the data.

The assumption is that the permissions from step 5 includes those at the time of the _saveSoon call. However, since the StartupCache is wiped immediately thereafter, the permissions are lost.

After reloading, it seems to work, but only accidentally: StartupCache.permissions starts off empty, so _getCached at step 1 will return a direct reference to a member of the JSONFile data. So even if the shutdown operation clears the prefs, the next _getCached call (when the add-on detail page is loaded) will immediately read from the JSONFile, which does include the added pref.

The currently proposed patch calls lazyInit earlier, which causes step 4 to run very soon, and significantly increase the probability that step 5 executes before the extension is reloaded. This doesn't look like a fix for the root cause though.
I think that a better fix is to maintain a transaction log that is independent of the StartupCache. This could simply be implemented as a chain of promises.

Comment 5

[Rob Wu [:robwu]]

ExtensionPermissions.jsm assumes that StartupCache.permissions contains at least the same value as the contents of the JSON file. This bug is a consequence of that invariant being broken.

The following fix will fix the problem by updating the JSON file data before the StartupCache (similarly for remove):

  async add(extensionId, perms, emitter) {
-   let {permissions, origins} = await this._getCached(extensionId);
+   let perms = await this._get(extensionId);
+   let {permissions, origins} = perms;

    ...
    if (added.permissions.length > 0 || added.origins.length > 0) {
-     this._saveSoon(extensionId);
+     prefs.saveSoon();
+     await StartupCache.permissions.set(extensionId, perms);
      if (emitter) {
        emitter.emit("add-permissions", added);

Another nice effect of this refactor is that the StartupCache update is explicit. Previously, the updated StartupCache was only saved because the "add-permissions" event triggers an update of StartupCache.manifests, which in its turn saves the whole StartupCache (including the "permissions" store of the StartupCache).

Comment 6

[Shane Caraveo (:mixedpuppy)]

Hrm, it seems StartupCache.permissions is not currently used, it is never set. So ExtensionPermissions only ever gets permissions from extension-preferences.json.

Either we should remove StartupCache.permissions entirely, or use it for the storage and get rid of extension-preferences.json. The latter would get rid of a file, but the former would require much more work (we currently clear the cache for an addon in a couple scenarios, some of which we may need to retain ExtensionPermissions data).

Comment 7

[Rob Wu [:robwu]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)

Hrm, it seems StartupCache.permissions is not currently used, it is never set. So ExtensionPermissions only ever gets permissions from extension-preferences.json.

StartupCache.permissions is set (though lazily): _getCached calls CacheStore.get, which sets the permissions if not cached.

If the permissions have already been cached, then it is used (and extension-preferences.json is never read). Upon updating the permissions, the JSON file is read (because of _saveSoon that calls lazyInit) and written (because _saveSoon calls saveSoon).

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)

Hrm, it seems StartupCache.permissions is not currently used, it is never set. So ExtensionPermissions only ever gets permissions from extension-preferences.json.

To clarify, it is set in the permissions.get, but it's only a shortcut, it's not set elsewhere, and isn't used elsewhere. I don't see it being any faster than just using the prefs object in ExtensionPermissions.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Rob Wu [:robwu] from comment #7)

If the permissions have already been cached, then it is used (and extension-preferences.json is never read).

Hm, it's not what I am seeing on startup, but I'll look again.

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)

(In reply to Rob Wu [:robwu] from comment #7)

If the permissions have already been cached, then it is used (and extension-preferences.json is never read).

Hm, it's not what I am seeing on startup, but I'll look again.

This was because I was running "mach build" which apparently kills startupcache.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eac52dd0d152
ensure ExtensionPermissions is fully initialized prior to changing r=aswan

Comment 12

[Rob Wu [:robwu]]

You also need to add an explicit prefs.data[extensionId] = {origins, permissions}; before calling prefs.saveSoon, because this is no longer automatically done by _get (it's removed in https://hg.mozilla.org/integration/autoland/rev/5542b48b6dfe).

Comment 13

[Cristian Brindusan [:cbrindusan]]

Backed out changeset eac52dd0d152 (Bug 1525846) for:

• mochitest failures at test_chrome_ext_permissions.html and
• bc failures at browser_webext_incognito.js and browser_ext_menus_replace_menu_permissions.js
• xpcshell failure at test_ext_permissions_uninstall.js

Backout: https://hg.mozilla.org/integration/autoland/rev/69a26dbaa3bbab1b6b0c18d8263584fd306b7a5e

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=eac52dd0d1520aaad58b91492b613f45aeb1e4c3&selectedJob=228293854

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228293854&repo=autoland&lineNumber=3373
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228293413&repo=autoland&lineNumber=13197
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228293940&repo=autoland&lineNumber=10050
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228301826&repo=autoland&lineNumber=12412

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/181ae83c0c05
ensure ExtensionPermissions is fully initialized prior to changing r=aswan

Comment 15

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/181ae83c0c05

Comment 16

[CosminB]

Attachment: Bug1525846.gif

This issue is verified as fixed on Firefox 67.0a1 (20190225102402) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached video.