Closed Bug 1341277 Opened 7 years ago Closed 7 years ago

Update privacy API to allow for disabling/enabling settings over disable/enable and updates of extensions

Categories

(WebExtensions :: Compatibility, defect, P3)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [privacy]triaged)

Attachments

(3 files)

Update the privacy API, as well as the PreferencesManager and SettingsStore to support the following:

1. When an extension is disabled, mark any settings currently being requested by the extension as disabled. If the extension was currently in control of a setting that will also cause the underlying prefs for that setting to be updated as the extension will no longer be in control.

2. When an extension is enabled, mark any settings currently being requested by the extension as enabled. If the extension is now in control of a setting that will also cause the underlying prefs for that setting to be updated as per the extension's request.

3. When an upgrade/downgrade occurs, disable all settings currently being requested by the extension during the "uninstall" phase of the upgrade/downgrade. 

4. When the upgrade/downgrade completes, with the new version of the extension installed, re-enable all of the extensions setting requests.

Note that this differs from the current behaviour in the following ways:

- Currently settings are untouched during upgrade/downgrade which could result in settings remaining in place for an uninstalled extension if the upgrade/downgrade does not complete successfully.
- Currently settings are removed completely during a disable and are therefore unable to be reinstated upon a re-enable of the extension.
Looking at SettingsStore and PreferencesManager, I think maybe the simplest way to do this would be to change the current removal process to a soft delete. I.e., where we currently physically remove an extension setting from the precedence chain, we instead just mark it as disabled, and treat that as a soft delete. This does mean we could eventually end up with a large number of disabled settings in the SettingsStore, but I don't think the cost of that is very high.

The alternative seems to be to duplicate all of the `remove` methods into `disable` methods, so that different code paths can choose to either truly remove a setting, or just disable it. This will avoid the problem of old settings hanging around after a true uninstall, but will require more methods and code paths.

What do you think, Andrew?
Flags: needinfo?(aswan)
Actually, I don't think we can have only soft deletes and not removals, because we need to support `clear` methods, which need to permanently remove items. Nevermind.
Flags: needinfo?(aswan)
Comment on attachment 8840440 [details]
Bug 1341277 - Part 3: Update ext-privacy.js to support disabling and re-enabling settings.

Note that the tests for this are still a work in progress, but the code for ext-privacy.js as well as the code and tests for parts 1 and 2 should be considered to be complete. I wanted to submit this for review to make sure I am on the right track with all of this.
Comment on attachment 8840438 [details]
Bug 1341277 - Part 1: Update ExtensionSettingsStore to support disabled settings.

https://reviewboard.mozilla.org/r/114942/#review117288

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:28
(Diff revision 1)
>   *       precedenceList: [
>   *         {
>   *           id, // The id of the extension requesting the setting.
>   *           installDate, // The install date of the extension.
> - *           value // The value of the setting requested by the extension.
> + *           value, // The value of the setting requested by the extension.
> + *           disabled // Whether the setting is currently disabled.

nit: I think this would overall be easier to read if the flag is "enabled" instead of "disabled", avoiding double-negatives like `!disabled` to check if a particular setting is enabled.  As we discussed in person, I don't think we need to be concerned about compatibility here.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:187
(Diff revision 1)
> +   * @returns {object | null}
> +   *          Either an object with properties for key and value, which
> +   *          corresponds to the current top precedent setting, or null if
> +   *          the current top precedent setting has not changed.
>     */
> -  async removeSetting(extension, type, key) {
> +  async removeSetting(extension, type, key, permanent = true) {

As discussed in person, I don't like the idea of overloading remove, but I do think that a single function `setEnabled()` that takes the value for the enabled flag would work nicely.  That function and this one would still be similar, whether you want to factor out a shared helper is up to you (I would probably not bother)
Attachment #8840438 - Flags: review?(aswan) → review+
Comment on attachment 8840438 [details]
Bug 1341277 - Part 1: Update ExtensionSettingsStore to support disabled settings.

https://reviewboard.mozilla.org/r/114942/#review117288

> As discussed in person, I don't like the idea of overloading remove, but I do think that a single function `setEnabled()` that takes the value for the enabled flag would work nicely.  That function and this one would still be similar, whether you want to factor out a shared helper is up to you (I would probably not bother)

I did factor out a shared helper, and that led me to have an `enable` and a `disable` method in the API, which I think is a bit friendlier than `setEnabled(true)` etc.

I have also updated the PreferencesManager to use the new API of the SettingsStore, although I haven't updated the code for the privacy API yet.
Comment on attachment 8840439 [details]
Bug 1341277 - Part 2: Update ExtensionPreferencesManager to support disabled settings.

https://reviewboard.mozilla.org/r/114944/#review117488

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:144
(Diff revision 2)
>      if (item) {
>        let prefs = item.initialValue || await settingsMap.get(name).setCallback(item.value);
>        setPrefs(prefs);
>      }

this is repeated enough times that i think it would be worth factoring out

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:159
(Diff revision 2)
> -   * @param {Extension} extension The extension for which all settings are being unset.
> +   * @param {Extension} extension
> +   *        The extension for which a preference setting is being removed.
> +   * @param {string} name
> +   *        The unique id of the setting.
> +   */
> +  async disableSetting(extension, name) {

nit: the order of these methods doesn't make much sense to me.  I think it might be easier to navigate if you group the single-setting methods together and then the `*All` methods together after that.
Attachment #8840439 - Flags: review?(aswan) → review+
Comment on attachment 8840440 [details]
Bug 1341277 - Part 3: Update ext-privacy.js to support disabling and re-enabling settings.

https://reviewboard.mozilla.org/r/114946/#review117492

clearing review flag until this is updated
Attachment #8840440 - Flags: review?(aswan)
This is ready for another review. The changes to ext-privacy.js should be final, and the tests are good, except I'm still having that issue with having to wait for prefs to be updated when disabling an extension. For now I've just added waits into the test for that, so I can verify that everything is working and my asserts are correct, but that needs to be fixed.
I just pushed a change to the disable/enable test which uses a Preferences observer as we discussed in our meeting today. I believe that all of this is now ready to be reviewed.
Blocks: 1341458
Comment on attachment 8840440 [details]
Bug 1341277 - Part 3: Update ext-privacy.js to support disabling and re-enabling settings.

https://reviewboard.mozilla.org/r/114946/#review118068

::: toolkit/components/extensions/ext-privacy.js:24
(Diff revision 4)
> +  if (["ADDON_DISABLE", "ADDON_UPGRADE", "ADDON_DOWNGRADE"]
> +      .includes(extension.shutdownReason)) {
> +    await ExtensionPreferencesManager.disableAll(extension);
> +  } else if (extension.shutdownReason === "ADDON_UNINSTALL") {
> +    await ExtensionPreferencesManager.removeAll(extension);
> +  }

nit: I would write this as a switch statement, but if you prefer this that's fine.

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy_disable.js:57
(Diff revision 4)
> +  const OLD_ID = "old_id@tests.mozilla.org";
> +  const NEW_ID = "new_id@tests.mozilla.org";
> +
> +  const PREF_TO_WATCH = "network.http.speculative-parallel-limit";
> +
> +  // Create a object to hold the values to which we will initialize the prefs.

nit: "an" object

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy_disable.js:80
(Diff revision 4)
> +      if (pref === "network.http.speculative-parallel-limit") {
> +        equal(Preferences.get(pref),
> +              expected ? ExtensionPreferencesManager.getDefaultValue(pref) : 0,
> +              msg);
> +      } else {
> +        equal(Preferences.get(pref), expected ? PREFS[pref] : !PREFS[pref], msg);
> +      }

nit: you could make this less repetitive by storing the expected value in a local with a branch to special case the speculative-parallel-limit pref, then just have a single `equal(Preferences.get(pref), expectedValue, msg);` call

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy_disable.js:92
(Diff revision 4)
> +    }
> +  }
> +
> +  async function toggleExtension(addonId, enabled) {
> +    let extensionChangePromise = enabled ? awaitEvent("ready") : awaitPrefChange(PREF_TO_WATCH);
> +    let addon = await AddonManager.getAddonByID(addonId);

The addon object won't change across disable/enable, so you can just do this once in the main test.
At that point, I think it would be clearer to just inline the two lines that set userDisabled and then await an appropriate event...

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy_disable.js:143
(Diff revision 4)
> +  }
> +
> +  // Set the value to true for the older extension.
> +  testExtensions[0].sendMessage("set", {value: true});
> +  let data = await testExtensions[0].awaitMessage("privacyData");
> +  ok(data.value, "Value to set true for the older extension.");

I don't understand the message.  Were "to" and "set" just meant to be swapped?

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy_disable.js:163
(Diff revision 4)
> +  for (let pref in PREFS) {
> +    equal(Preferences.get(pref), PREFS[pref], `${pref} reset correctly.`);
> +  }

why isn't this just calling `checkPrefs()`?

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy_update.js:129
(Diff revision 4)
> +  let webExtensionFile = createTempWebExtensionFile({
> +    manifest: {
> +      version: "2.0",
> +      applications: {
> +        gecko: {
> +          id: EXTENSION_ID,
> +        },
> +      },
> +      permissions: ["privacy"],
> +    },
> +    background,
> +  });
> +
> +  testServer.registerFile("/addons/test_privacy-2.0.xpi", webExtensionFile);

can you put this together with the http server setup code above?

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy_update.js:162
(Diff revision 4)
> +  let install = update.updateAvailable;
> +
> +  let promiseInstalled = promiseAddonEvent("onInstalled");
> +  await promiseCompleteAllInstalls([install]);
> +
> +  await extension.awaitMessage("reloading");

does this serve any purpose?  (likewise with the corresponding code in the background script)

::: toolkit/components/extensions/test/xpcshell/test_ext_privacy_update.js:179
(Diff revision 4)
> +    if (pref === "network.http.speculative-parallel-limit") {
> +      equal(Preferences.get(pref), 0, msg);
> +    } else {
> +      equal(Preferences.get(pref), !PREFS[pref], msg);

same comment as above
Attachment #8840440 - Flags: review?(aswan) → review+
Comment on attachment 8840440 [details]
Bug 1341277 - Part 3: Update ext-privacy.js to support disabling and re-enabling settings.

https://reviewboard.mozilla.org/r/114946/#review118068

> why isn't this just calling `checkPrefs()`?

The difference is that in this case we do not need to special case the check for "network.http.speculative-parallel-limit" because we expect it to be the same value that we set it to during initialization, which is different from both the value expected when setting `network.networkPredictionEnabled` to `true` and when setting `network.networkPredictionEnabled` to `false`. This tests that things are returned to their initial values when all controlling extensions are disabled.

I could reuse `checkPrefs` if I added an argument to indicate that I don't need to special case the check for "network.http.speculative-parallel-limit", but I fear that would over-complicate `checkPrefs`, and as this is just a simple loop with a single line of code in it, I figured it was better to just leave it as separate from `checkPrefs`.
Try looks good. Requesting check in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/68d9a7c54c00
Part 1: Update ExtensionSettingsStore to support disabled settings. r=aswan
https://hg.mozilla.org/integration/autoland/rev/02d52f2c1d55
Part 2: Update ExtensionPreferencesManager to support disabled settings. r=aswan
https://hg.mozilla.org/integration/autoland/rev/ce4a71e11833
Part 3: Update ext-privacy.js to support disabling and re-enabling settings. r=aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/68d9a7c54c00
https://hg.mozilla.org/mozilla-central/rev/02d52f2c1d55
https://hg.mozilla.org/mozilla-central/rev/ce4a71e11833
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.