Closed Bug 1525846 Opened 7 months ago Closed 6 months ago

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

Categories

(WebExtensions :: General, defect, P1)

67 Branch
defect

Tracking

(firefox65 disabled, firefox66 disabled, firefox67 verified)

VERIFIED FIXED
mozilla67
Tracking Status
firefox65 --- disabled
firefox66 --- disabled
firefox67 --- verified

People

(Reporter: cbadescu, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Attached image 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.

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?

Flags: needinfo?(aswan)

Figured out a solution after chatting with aswan.

Flags: needinfo?(aswan)
Assignee: nobody → mixedpuppy
Priority: -- → P1

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:

  1. _saveSoon calls lazyInit to read the JSON file.
  2. _saveSoon retrieves the latest value of the preferences, via _getCached.
  3. _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.

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).

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).

Flags: needinfo?(aswan)

(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).

(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.

(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.

(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.

Flags: needinfo?(aswan)
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eac52dd0d152
ensure ExtensionPermissions is fully initialized prior to changing r=aswan

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).

Depends on: 1522918
Flags: needinfo?(mixedpuppy)
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/181ae83c0c05
ensure ExtensionPermissions is fully initialized prior to changing r=aswan
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1529826
Attached image 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.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.