Closed Bug 1213990 Opened 9 years ago Closed 8 years ago

Clear localStorage when a WebExtension is uninstalled

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: billm, Assigned: aswan, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [storage]triaged)

Attachments

(3 files, 1 obsolete file)

For privacy reasons, WebExtensions would like to ensure that their localStorage data is cleared when the extension is uninstalled. I don't think this should be too hard once bug 1208874 is fixed. We should probably fix them together.
Another related bug is when we have objects in objects stored. After removing and installing the addon again we get DeadObject from the storage, which throws internally in resource://gre/modules/ExtensionUtils.jsm.
Flags: blocking-webextensions-
Can I confirm this something we'd like to do, there were some questions on irc.
Assignee: nobody → aswan
Flags: needinfo?(kmaglione+bmo)
Whiteboard: [storage] → [storage]triaged
I'm not sure. We've talked about doing this for XUL extensions several times in the past, and every time there have been objections, from both users and developers, that they'd rather their data and settings stick around, in case they reinstall the extension later.
Flags: needinfo?(kmaglione+bmo)
It doesn't seem great leaving that data around forever without it ever being cleaned up. There are bugs on how the marketplace filled localStorage with several megs of data, I'd hate for that to happen on Android.

If an add-on wants the data to be persisted they could use chrome.storage.sync.
If Chrome already does this anyway, I suppose we may as well too.
Mentor: kmaglione+bmo
Depends on: 1250784
See Also: → 627432
Please do prioritize this issue. We cannot use any kind of workaround as the hooks for install/uninstall events are not support for webextensions.
Hello there, 
We would like this defect to be prioritized. This is a blocker for us at Symantec. Can someone please take a look at it ?
We (Symantec) would like the Firefox to either delete its storage container automatically when the web extension is uninstalled or provide an event that the extension can listen for and cleanup its storage. 
 
Like I mentioned earlier, this is a blocker for us and would be great if it can be fixed soon.
There is some discussion of implementing management.onUninstalled this quarter, and it sounds like that might give you what you need. Kris, does that sound like a reasonable way to address this issue?
Flags: needinfo?(kmaglione+bmo)
See Also: → 1282984
You don't get a `management.onUninstalled` event if you uninstall yourself, do you? And if you do, what happens if you use a method with a callback or a promise? Is uninstall blocked until they resolve, or is the event handler swept into the abyss, or what?
For a number of reasons, I don't think management.onUninstalled is the answer here.  (Nancy makes a good point, even if we did some quirky workaround, there's also the case of uninstalling a disabled extension.  Moreover, if extensions can perform asynchronous actions at uninstall time they could create all sorts of havoc, either intentionally or unintentionally).
This is slowly working its way to the top of my queue, hopefully I'll find some time next week.
Flags: needinfo?(kmaglione+bmo)
Is it worth filing a bug about an uninstall method that can block? I know we want to avoid blocking APIs as much as possible, but I'm wondering about other things that need cleaning up when an add-on is uninstalled. Adding anything that does block should be added with much caution though.
I've tested the attached patch manually, I don't see any way to test this without actually using the AddonManager to install/uninstall an extension, which I think will really work best as a chrome mochitest.  While I'm doing that, I figure I may as well move the existing local storage over to the new test file, just to keep all the local storage tests together and the existing test will work fine from chrome.
Am I overlooking something simpler or does this sound right?
Flags: needinfo?(kmaglione+bmo)
(In reply to Harsha A from comment #8)
> Like I mentioned earlier, this is a blocker for us and would be great if it
> can be fixed soon.

This is getting close to landing and I hope to have it in 50 but I'm curious how an issue like this rises to the level of a blocker?  The consequence is simply orphaned unreachable data when an extension is uninstalled.
Flags: needinfo?(sriharsha.angara)
(In reply to Andrew Swan [:aswan] from comment #14)
> I've tested the attached patch manually, I don't see any way to test this
> without actually using the AddonManager to install/uninstall an extension,
> which I think will really work best as a chrome mochitest.  While I'm doing
> that, I figure I may as well move the existing local storage over to the new
> test file, just to keep all the local storage tests together and the
> existing test will work fine from chrome.
> Am I overlooking something simpler or does this sound right?

No need for a chrome mochitest. Just add `useAddonManager` to the extension options.
Flags: needinfo?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/63786/#review61036

::: toolkit/components/extensions/Extension.jsm:520
(Diff revision 1)
> +
> +function clearExtensionUUID(id) {
> +  let map = readUUIDMap();
> +  delete map[id];
> +  writeUUIDMap(map);
> +}

This is getting a bit messy. Can we create a `UUIDMap` singleton, or something, with all of these methods?

::: toolkit/components/extensions/Extension.jsm:537
(Diff revision 1)
>    },
>  
>    onUninstalling: function(addon) {
>      let extension = GlobalManager.extensionMap.get(addon.id);
>      if (extension) {
> +      // Clear any local storage owned by the extension

We should only clear the local storage and UUID entry on `onUninstalled`, not `onUninstalling`. The latter can be canceled, which would leave us in a bad state.
Comment on attachment 8770321 [details]
Bug 1213990 Do immediate uninstall for test webextensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63786/diff/1-2/
The attached patch now represents my best understanding about how this should work, but it isn't working.  The new test case added here (https://reviewboard.mozilla.org/r/63786/diff/2#1) creates an extension that writes to localStorage, and the test is able to see the written data using the DOM storage manager.
The cleanup code is at line 539 of Extension.jsm (https://reviewboard.mozilla.org/r/63786/diff/2#0) and when the test is run, I do see that dump command, something like:

clear storage for moz-extension://347ab5c2-a0af-f748-a4db-00654bfdde36^addonId=localstorage%40tests.mozilla.org

But when the test proceeds to re-read that storage, the data is still there.  The info message looks like the principal matches:
29 INFO looking up storage for moz-extension://347ab5c2-a0af-f748-a4db-00654bfdde36/_blank.html principal moz-extension://347ab5c2-a0af-f748-a4db-00654bfdde36^addonId=localstorage%40tests.mozilla.org

I added an extra delay between uninstalling the extension and checking storage and it was still present.  And poking around at webappsstore.sqlite in the profile directory confirms the data is still there.

I'm not sure what I'm doing wrong here, Jan can you help me out?  (I hope there's enough context here to understand the problem, please ask questions if not)
Flags: needinfo?(jvarga)
https://reviewboard.mozilla.org/r/63786/#review61036

> This is getting a bit messy. Can we create a `UUIDMap` singleton, or something, with all of these methods?

Yep, I was planning the same thing.  But I want to get the actual storage cleanup working first, then I'll take another pass at some code cleanup before setting r?
I think there's some confusion about what "local storage" means here. It seems you replaced domStorageManager.getLocalStorageForPrincipal() with qms.clearStoragesForPrincipal()
The problem is that Local Storage API is not yet plugged into quota manager, so clearStoragesForPrincipal() won't clear local storage data. It will only clear IndexedDB, DOM cache and AsmJS cache.
Flags: needinfo?(jvarga)
Hm, I started with getLocalStorageForPrincipal() but that requires a document URI and I don't know all the URIs that might be associated with the given principal, I want to remove all the local storage associated with the given principal.  I was looking at https://dxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/ForgetAboutSite.jsm which uses clearStoragesForPrincipal(), that module has a test that asserts that local storage is cleared but I don't see any code that uses getLocalStorageForPrincipal().
Can you help me understand how it works and/or how I can clear all local storage associated with a given principal?
Flags: needinfo?(jvarga)
I think it's |Services.obs.notifyObservers(null, "browser:purge-domain-data", aDomain);| that broadcast the notification and DOMStorageObserver.cpp handles it to clear local storage data.
Flags: needinfo?(jvarga)
Review commit: https://reviewboard.mozilla.org/r/66528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66528/
Attachment #8770321 - Attachment description: Bug 1213990 clear local storage on uninstall → Bug 1213990 Do immediate uninstall for test webextensions
Attachment #8773873 - Flags: review?(kmaglione+bmo)
Attachment #8773874 - Flags: review?(kmaglione+bmo)
Attachment #8770321 - Flags: review?(kmaglione+bmo)
Comment on attachment 8770321 [details]
Bug 1213990 Do immediate uninstall for test webextensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63786/diff/2-3/
https://reviewboard.mozilla.org/r/63786/#review61036

> We should only clear the local storage and UUID entry on `onUninstalled`, not `onUninstalling`. The latter can be canceled, which would leave us in a bad state.

Whoops, forgot to address this before pushing the updated patch.
The problem is that by the time onUinstalled fires, the extension is gone from the map in the GlobalManager.
I guess this object could keep its own map of recently uninstalled extensions where it creates entries in onUninstalling and clears them in onUninstalled?
Comment on attachment 8770321 [details]
Bug 1213990 Do immediate uninstall for test webextensions

https://reviewboard.mozilla.org/r/63786/#review63260
Attachment #8770321 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/1-2/
https://reviewboard.mozilla.org/r/66530/#review63262

Apparently I made these comments on a previous version and forgot to submit.

::: toolkit/components/extensions/Extension.jsm:59
(Diff revision 1)
>                                    "resource://gre/modules/AppConstants.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "MessageChannel",
>                                    "resource://gre/modules/MessageChannel.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
>                                    "resource://gre/modules/AddonManager.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionStorage",

I know these aren't really sorted as is, but please insert this in sorted order.

::: toolkit/components/extensions/Extension.jsm:110
(Diff revision 1)
>    flushJarCache,
>  } = ExtensionUtils;
>  
>  const LOGGER_ID_BASE = "addons.webextension.";
>  const UUID_MAP_PREF = "extensions.webextensions.uuids";
> +const LEAVE_STORAGE_PREF = "extensions.webextensions.leaveStorage";

`retainStorageOnUninstall`?

::: toolkit/components/extensions/Extension.jsm:535
(Diff revision 1)
>    initialized: false,
>  
>    init: function() {
>      if (!this.initialized) {
>        AddonManager.addAddonListener(this);
> +      XPCOMUtils.defineLazyPreferenceGetter(this, "leaveStorage", LEAVE_STORAGE_PREF);

Please add a default value argument, and also add a default value for this preferences to `all.js` so that developers can easily find it in about:config.
https://reviewboard.mozilla.org/r/66530/#review63262

> Please add a default value argument, and also add a default value for this preferences to `all.js` so that developers can easily find it in about:config.

I had intended this to be a test-only thing but if you think its worth making public I can do that...
Agreed that if its going to be public, it needs a more descriptive name.
Attachment #8773874 - Flags: review?(kmaglione+bmo)
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

https://reviewboard.mozilla.org/r/66530/#review64306

::: toolkit/components/extensions/Extension.jsm:564
(Diff revision 2)
> +  onOperationCancelled(addon) {
> +    this.pending.delete(addon.id);
> +  },
> +
> +  onUninstalled(addon) {
> +    let extension = this.pending.get(addon.id);

There are too many things that might go wrong with this map. Can we just create a principal from the add-on ID rather than trying to use an Extension object?

::: toolkit/components/extensions/test/mochitest/chrome.ini:32
(Diff revision 2)
>  [test_ext_cookies_permissions.html]
>  skip-if = buildapp == 'b2g'
>  [test_ext_jsversion.html]
>  skip-if = buildapp == 'b2g'
>  [test_ext_schema.html]
> +[test_chrome_storage_cleanup.html]

`test_chrome_ext_storage_cleanup.html`

::: toolkit/components/extensions/test/mochitest/test_chrome_storage_cleanup.html:23
(Diff revision 2)
> +// Test that storage used by a webextension (through localStorage,
> +// indexedDB, and browser.storage.local) gets cleaned up when the
> +// extension is uninstalled.
> +add_task(function* test_uninstall() {
> +  function writeData() {
> +    let [, uuid] = window.location.href.match(/^moz-extension:\/\/([-a-z0-9]+)\//);

`window.location.hostname`

::: toolkit/components/extensions/test/mochitest/test_chrome_storage_cleanup.html:61
(Diff revision 2)
> +      browser.test.sendMessage("finished");
> +    });
> +  }
> +
> +  function readData() {
> +    let matchLocalStorage = (localStorage.getItem("hello") == "world");

Unnecessary parens.

::: toolkit/components/extensions/test/mochitest/test_chrome_storage_cleanup.html:82
(Diff revision 2)
> +        let addreq = transaction.objectStore("store").get("hello");
> +        addreq.onerror = e => {
> +          reject(new Error(`read from indexedDB failed with ${e.errorCode}`));
> +        };
> +        addreq.onsuccess = e => {
> +          let match = (addreq.result.value == "world");

Unnecessary parens (also unnecessary variable).

::: toolkit/components/extensions/test/mochitest/test_chrome_storage_cleanup.html:89
(Diff revision 2)
> +        };
> +      };
> +    });
> +
> +    let browserStoragePromise = browser.storage.local.get("hello").then(result => {
> +      return (Object.keys(result).length == 1 && result.hello == "world");

Unnecessary parens.

::: toolkit/components/extensions/test/mochitest/test_chrome_storage_cleanup.html:95
(Diff revision 2)
> +    });
> +
> +    Promise.all([idbPromise, browserStoragePromise])
> +           .then(([matchIDB, matchBrowserStorage]) => {
> +             let result = {
> +               matchLocalStorage, matchIDB, matchBrowserStorage,

Either the object literal should be a single line, or every property should be on its own line.

::: toolkit/components/extensions/test/mochitest/test_chrome_storage_cleanup.html:97
(Diff revision 2)
> +    Promise.all([idbPromise, browserStoragePromise])
> +           .then(([matchIDB, matchBrowserStorage]) => {
> +             let result = {
> +               matchLocalStorage, matchIDB, matchBrowserStorage,
> +             };
> +             dump(`results ${JSON.stringify(result)}\n`);

No dump calls in tests.
Comment on attachment 8773873 [details]
Bug 1213990 Convert getExtensionUUID to UUIDMap

https://reviewboard.mozilla.org/r/66528/#review64312

::: toolkit/components/extensions/Extension.jsm:488
(Diff revision 1)
> +// installed (by trying to load a moz-extension URI referring to a
> +// web_accessible_resource from the extension). UUIDMap.get()
> +// returns the UUID for a given add-on ID.
> +this.UUIDMap = {
> +  _read() {
> +    let pref = Preferences.get(UUID_MAP_PREF, "{}");

We should use `defineLazyPreferenceGetter` for this.

::: toolkit/components/extensions/Extension.jsm:517
(Diff revision 1)
> +    map[id] = uuid;
> +    this._write(map);
> +    return uuid;
> +  },
> +
> +  clear(id) {

`clear` generally means "empty the entire map". This should be `delete`
Attachment #8773873 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8773873 [details]
Bug 1213990 Convert getExtensionUUID to UUIDMap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66528/diff/1-2/
Attachment #8773874 - Flags: review?(kmaglione+bmo)
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/2-3/
https://reviewboard.mozilla.org/r/66530/#review64306

> There are too many things that might go wrong with this map. Can we just create a principal from the add-on ID rather than trying to use an Extension object?

I'm not opposed to making the change but please elaborate on the many things that might go wrong?

> Unnecessary parens.

I realize they don't change the meaning of the code but I prefer this when reading quickly to make the intent clear that this is of the form "a = b == c" and not "a = b = c"
https://reviewboard.mozilla.org/r/66528/#review64312

> We should use `defineLazyPreferenceGetter` for this.

Why?  Since there's no analogue for setters, I'd prefer to keep the read/write code symmetric, the alternative seems like a pain for folks reading the code to absort, and I don't think there's any plausible efficiency issue here.
https://reviewboard.mozilla.org/r/66528/#review64312

> Why?  Since there's no analogue for setters, I'd prefer to keep the read/write code symmetric, the alternative seems like a pain for folks reading the code to absort, and I don't think there's any plausible efficiency issue here.

It's a pervasive best practice in toolkit code to use cached getters for preferences that are accessed repeatedly, and we're going to be reading this preference much more often than we'll be writing it.
https://reviewboard.mozilla.org/r/66530/#review64306

> I'm not opposed to making the change but please elaborate on the many things that might go wrong?

For one thing, it's possible to uninstall an extension that's never been enabled in the current session (e.g., was disabled in a previous session), in which case its storage won't be cleared.

That issue aside, keeping extension instances around after they've been shut down has too many ways it might go wrong. We might wind up accidentally calling a method that expects the instance to be alive. We might accidentally keep around resources that should be freed when the instance is GCed. We might let the map get out of sync because of some circumstance we didn't expect. We can try to account for all of those problems, but I think it's just adding a lot of complexity for very little gain.
https://reviewboard.mozilla.org/r/66530/#review64584

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_storage_cleanup.html:127
(Diff revision 3)
> +    let map = UUIDMap._read();
> +    is(map[ID], undefined, "Test extension was removed from UUID map");
> +
> +    map[ID] = uuid;
> +    UUIDMap._write(map);
> +  }

Can we just add a preference to keep the UUID map entry, like the one we have to keep the storage data?
Comment on attachment 8773873 [details]
Bug 1213990 Convert getExtensionUUID to UUIDMap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66528/diff/2-3/
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/3-4/
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

https://reviewboard.mozilla.org/r/66530/#review65130

Looks good. Thanks

::: toolkit/components/extensions/Extension.jsm:111
(Diff revision 4)
>  } = ExtensionUtils;
>  
>  const LOGGER_ID_BASE = "addons.webextension.";
>  const UUID_MAP_PREF = "extensions.webextensions.uuids";
> +const LEAVE_STORAGE_PREF = "extensions.webextensions.keepStorageOnUninstall";
> +const LEAVE_UUID_PREF = "extensions.webextensions.keepUuidOnUninstall";

This should also be in all.js
Attachment #8773874 - Flags: review?(kmaglione+bmo) → review+
https://reviewboard.mozilla.org/r/66530/#review65130

> This should also be in all.js

I mean to add a comment about this with the new patch but this one doesn't seem useful outside of testing scenarios so I meant to keep it non-public.  (To put it another way, I can't imagine how it would be documented to users in a comprehensible way...)
The dev-doc-needed flag is to cover documenting the new preference(s?) introduced with this patch.
Keywords: dev-doc-needed
https://reviewboard.mozilla.org/r/66530/#review65130

> I mean to add a comment about this with the new patch but this one doesn't seem useful outside of testing scenarios so I meant to keep it non-public.  (To put it another way, I can't imagine how it would be documented to users in a comprehensible way...)

I'd imagine it would be fairly useful to some testers, especially along with `keepStorageOnUninstall`.
https://reviewboard.mozilla.org/r/66530/#review65130

> I'd imagine it would be fairly useful to some testers, especially along with `keepStorageOnUninstall`.

True I guess keepStorageOnUninstall by itself is pretty useless without keepUuidOnUninstall, I'll add it.
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/4-5/
Comment on attachment 8770321 [details]
Bug 1213990 Do immediate uninstall for test webextensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63786/diff/3-4/
Comment on attachment 8773873 [details]
Bug 1213990 Convert getExtensionUUID to UUIDMap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66528/diff/3-4/
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/5-6/
This had a bunch of conflicts with recent changes, the latest revision is rebased and has conflicts resolved.
I had to remove the code that tries to un-initialize the UninstallObserver when we go from 1->0 active webextensions since that code ran before the onUninstalled event was fired.  Thinking this through, I realized that we have another problem though, if you have exactly 1 webextension installed and it uses any of the storage types handled by this patch, then you disable the extension, restart the browser, then uninstall the extension, the UninstallObserver will not have been initialized so the storage won't get cleaned up.  I think that's enough of a corner case that I'd like to land this as-is for 50 and then have a separate follow-up bug for that scenario.
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12d7193f1310
Do immediate uninstall for test webextensions r=kmag
https://hg.mozilla.org/integration/autoland/rev/26bab9c6a89c
Convert getExtensionUUID to UUIDMap r=kmag
https://hg.mozilla.org/integration/autoland/rev/846ac5ddba37
Clear storage when webextension is uninstalled r=kmag
Attachment #8776751 - Flags: review?(kmaglione+bmo)
Comment on attachment 8776751 [details]
Bug 1213990 follow-up: remove old test of UninstallObserver uninit

https://reviewboard.mozilla.org/r/68426/#review65522
Attachment #8776751 - Flags: review?(kmaglione+bmo) → review+
Backed out at aswan's request in https://hg.mozilla.org/integration/autoland/rev/7143e9551263 because it's apparently going to fail some mochitest.
Flags: needinfo?(aswan)
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/6-7/
Attachment #8776751 - Attachment is obsolete: true
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/7-8/
https://reviewboard.mozilla.org/r/66530/#review65556

::: toolkit/components/extensions/Extension.jsm:547
(Diff revision 8)
> +      // Clear any IndexedDB storage created by the extension
> +      let baseURI = NetUtil.newURI(`moz-extension://${uuid}/`);
> +      let principal = Services.scriptSecurityManager.createCodebasePrincipal(
> +        baseURI, {addonId: addon.id}
> +      );
> +      Services.qms.clearStoragesForPrincipal(principal);

Do we actually need to call clearStoragesForPrincipal() here ?
Quota Manager listens for the "clear-origin-data" notification and clears origins based on the passed pattern.
https://reviewboard.mozilla.org/r/66530/#review65556

> Do we actually need to call clearStoragesForPrincipal() here ?
> Quota Manager listens for the "clear-origin-data" notification and clears origins based on the passed pattern.

Well, when I comment out that line, then the individual test of IndexedDB fails.
https://reviewboard.mozilla.org/r/66530/#review65556

> Well, when I comment out that line, then the individual test of IndexedDB fails.

I'm going to land this as is.  Jan, if you think this really shouldn't be necessary can you open a separate bug?  I think I could relatively easily produce a simplified test case for you and can remove a few lines of code if/when it gets fixed...
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea870f4c5a61
Do immediate uninstall for test webextensions r=kmag
https://hg.mozilla.org/integration/autoland/rev/157efae8dd8a
Convert getExtensionUUID to UUIDMap r=kmag
https://hg.mozilla.org/integration/autoland/rev/2b1fdeff506d
Clear storage when webextension is uninstalled r=kmag
Comment on attachment 8770321 [details]
Bug 1213990 Do immediate uninstall for test webextensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63786/diff/4-5/
Comment on attachment 8773873 [details]
Bug 1213990 Convert getExtensionUUID to UUIDMap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66528/diff/4-5/
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/8-9/
Attachment #8773873 - Flags: review?(lgreco)
https://reviewboard.mozilla.org/r/66528/#review65720

::: toolkit/components/extensions/Extension.jsm:509
(Diff revision 5)
> +function getExtensionUUID(id) {
> +  return UUIDMap.get(id, false);
> +}

as briefly discussed over irc, the previous `getExtensionUUID` creates the extension uuid if it is not yet defined (like in `UUIDMap.get(id, true)`).

For the test case that is currently using `getExtensionUUID` it doesn't make any difference, but testpilot seems to create the webextension channels (which uses this helper) at `onInstallEnded`, and the different behavior could make a difference.
Comment on attachment 8773873 [details]
Bug 1213990 Convert getExtensionUUID to UUIDMap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66528/diff/5-6/
Attachment #8773873 - Flags: review?(lgreco)
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/9-10/
https://reviewboard.mozilla.org/r/66528/#review66052

::: toolkit/components/extensions/Extension.jsm:464
(Diff revision 6)
> +// All moz-extension URIs use a machine-specific UUID rather than the
> +// extension's own ID in the host component. This makes it more
> +// difficult for web pages to detect whether a user has a given add-on
> +// installed (by trying to load a moz-extension URI referring to a
> +// web_accessible_resource from the extension). UUIDMap.get()
> +// returns the UUID for a given add-on ID.

Could you move this comment above UUIDMap.get before landing?
https://reviewboard.mozilla.org/r/66528/#review66052

> Could you move this comment above UUIDMap.get before landing?

The comment is meant to document what the UUIDMap is, not just what UUIDMap.get() does.
https://reviewboard.mozilla.org/r/63786/#review66060

::: toolkit/components/extensions/Extension.jsm:1275
(Diff revision 5)
>        throw Error("installType must be one of: temporary, permanent");
>      }
>    },
>  
>    shutdown() {
> -    this.addon.uninstall(true);
> +    this.addon.uninstall();

Unless I'm looking at the wrong function signature, https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7453, it looks like this method is supposed to take a boolean called `alwaysAllowUndo`.  What's the reason for removing this and why not pass in `false`?  Also, how does passing in `alwaysAllowUndo` set to `false` make the uninstall immediate for test webextensions?
https://reviewboard.mozilla.org/r/66528/#review66052

> The comment is meant to document what the UUIDMap is, not just what UUIDMap.get() does.

Oh, sorry, I parsed this incorrectly. I didn't realize that `returns the UUID for a given add-on ID.` continued off of the line above it.
https://reviewboard.mozilla.org/r/63786/#review66060

> Unless I'm looking at the wrong function signature, https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#7453, it looks like this method is supposed to take a boolean called `alwaysAllowUndo`.  What's the reason for removing this and why not pass in `false`?  Also, how does passing in `alwaysAllowUndo` set to `false` make the uninstall immediate for test webextensions?

I'm not sure why true was being passed before, but the convention elsewhere in code that uses AddonManager is to leave the argument unspecified to get an immediate uninstall.  I don't understand your last question, are you asking how the add-ons manager implements undoable versus immediate uninstalls?
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3223d0970b9a
Do immediate uninstall for test webextensions r=kmag
https://hg.mozilla.org/integration/autoland/rev/ffc2455a9135
Convert getExtensionUUID to UUIDMap r=kmag
https://hg.mozilla.org/integration/autoland/rev/c9b70a1998fc
Clear storage when webextension is uninstalled r=kmag
https://reviewboard.mozilla.org/r/63786/#review66060

> I'm not sure why true was being passed before, but the convention elsewhere in code that uses AddonManager is to leave the argument unspecified to get an immediate uninstall.  I don't understand your last question, are you asking how the add-ons manager implements undoable versus immediate uninstalls?

No, it's just the lack of documentation in `XPIProvider.jsm` that got me confused.  Aside from it being the convention elsewhere, it's not very clear to me that this change will make the uninstall immediate, but that's not your fault.
hopefully third time is a charm...
Flags: needinfo?(aswan)
Depends on: 1292239
Flags: needinfo?(aswan)
Try run with patches for this bug stacked onto mconley's proposed patch for bug 1292239:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f40f628e45af
Comment on attachment 8770321 [details]
Bug 1213990 Do immediate uninstall for test webextensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63786/diff/5-6/
Comment on attachment 8773873 [details]
Bug 1213990 Convert getExtensionUUID to UUIDMap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66528/diff/6-7/
Comment on attachment 8773874 [details]
Bug 1213990 Clear storage when webextension is uninstalled

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66530/diff/10-11/
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/28cd5d6f4541
Do immediate uninstall for test webextensions r=kmag
https://hg.mozilla.org/integration/autoland/rev/9788d98cf100
Convert getExtensionUUID to UUIDMap r=kmag
https://hg.mozilla.org/integration/autoland/rev/b7348448a4c2
Clear storage when webextension is uninstalled r=kmag
I've added a note on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/local. Let me know if this covers it.
Flags: needinfo?(aswan)
(In reply to Will Bamberg [:wbamberg] from comment #87)
> I've added a note on this:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/local.
> Let me know if this covers it.

The note looks good, just one quibble about "This feature is provided to help developers test their add-on, and is not for WebExtensions to use in production." -- I read that as saying that extensions should not override those prefs, but WebExtensions don't have the ability to modify preferences...
Flags: needinfo?(aswan)
Flags: needinfo?(sriharsha.angara)
(In reply to Andrew Swan [:aswan] from comment #88)
> (In reply to Will Bamberg [:wbamberg] from comment #87)
> > I've added a note on this:
> > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/local.
> > Let me know if this covers it.
> 
> The note looks good, just one quibble about "This feature is provided to
> help developers test their add-on, and is not for WebExtensions to use in
> production." -- I read that as saying that extensions should not override
> those prefs, but WebExtensions don't have the ability to modify
> preferences...

Yes, I struggled a bit with this. I know WebExtensions can't change prefs, but other add-ons can, and people sometimes expect that they might be able to. So I want to make it clear that this is *not* a feature for WebExtensions to use. I've reworded it to be more clear, hopefully. Marking as dev-doc-complete, but let me know if you want any other changes.
Blocks: 1313422
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: