1213990 - Clear localStorage when a WebExtension is uninstalled

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 1

[Mihail Diordiev]

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.

Comment 2

[Andy McKay]

Can I confirm this something we'd like to do, there were some questions on irc.

Comment 3

[Kris Maglione [:kmag]]

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.

Comment 4

[Andy McKay]

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.

Comment 5

[Kris Maglione [:kmag]]

If Chrome already does this anyway, I suppose we may as well too.

Comment 6

[nkanand]

Please do prioritize this issue. We cannot use any kind of workaround as the hooks for install/uninstall events are not support for webextensions.

Comment 7

[Harsha A]

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 ?

Comment 8

[Harsha A]

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.

Comment 9

[Bob Silverberg [:bsilverberg]]

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?

Comment 10

[Nancy Grossman]

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?

Comment 11

[Andrew Swan [:aswan]]

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.

Comment 12

[Andy McKay]

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.

Comment 13

[Andrew Swan [:aswan]]

Attachment: Bug 1213990 Do immediate uninstall for test webextensions

Review commit: https://reviewboard.mozilla.org/r/63786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63786/

Comment 14

[Andrew Swan [:aswan]]

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?

Comment 15

[Andrew Swan [:aswan]]

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

Comment 16

[Kris Maglione [:kmag]]

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

Comment 17

[Kris Maglione [:kmag]]

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 18

[Andrew Swan [:aswan]]

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/

Comment 19

[Andrew Swan [:aswan]]

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)

Comment 20

[Andrew Swan [:aswan]]

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?

Comment 21

[Jan Varga [:janv]]

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.

Comment 22

[Andrew Swan [:aswan]]

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?

Comment 23

[Jan Varga [:janv]]

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.

Comment 24

[Andrew Swan [:aswan]]

Attachment: Bug 1213990 Convert getExtensionUUID to UUIDMap

Review commit: https://reviewboard.mozilla.org/r/66528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66528/

Comment 25

[Andrew Swan [:aswan]]

Attachment: Bug 1213990 Clear storage when webextension is uninstalled

Review commit: https://reviewboard.mozilla.org/r/66530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66530/

Comment 26

[Andrew Swan [:aswan]]

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/

Comment 27

[Andrew Swan [:aswan]]

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 28

[Kris Maglione [:kmag]]

Comment on attachment 8770321 [details]
Bug 1213990 Do immediate uninstall for test webextensions

https://reviewboard.mozilla.org/r/63786/#review63260

Comment 29

[Andrew Swan [: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/1-2/

Comment 30

[Kris Maglione [:kmag]]

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.

Comment 31

[Andrew Swan [:aswan]]

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.

Comment 32

[Kris Maglione [:kmag]]

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 33

[Kris Maglione [:kmag]]

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`

Comment 34

[Andrew Swan [:aswan]]

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/

Comment 35

[Andrew Swan [: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/2-3/

Comment 36

[Andrew Swan [:aswan]]

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"

Comment 37

[Andrew Swan [:aswan]]

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.

Comment 38

[Kris Maglione [:kmag]]

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.

Comment 39

[Kris Maglione [:kmag]]

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.

Comment 40

[Kris Maglione [:kmag]]

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 41

[Andrew Swan [:aswan]]

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 42

[Andrew Swan [: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/3-4/

Comment 43

[Kris Maglione [:kmag]]

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

Comment 44

[Andrew Swan [:aswan]]

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

Comment 45

[Andrew Swan [:aswan]]

The dev-doc-needed flag is to cover documenting the new preference(s?) introduced with this patch.

Comment 46

[Kris Maglione [:kmag]]

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

Comment 47

[Andrew Swan [:aswan]]

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 48

[Andrew Swan [: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/4-5/

Comment 49

[Andrew Swan [:aswan]]

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 50

[Andrew Swan [:aswan]]

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 51

[Andrew Swan [: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/5-6/

Comment 52

[Andrew Swan [:aswan]]

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.

Comment 53

[Pulsebot]

Pushed by [e-mail]:
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

Comment 54

[Andrew Swan [:aswan]]

Attachment: Bug 1213990 follow-up: remove old test of UninstallObserver uninit

Review commit: https://reviewboard.mozilla.org/r/68426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68426/

Comment 55

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/68426/#review65520

Comment 56

[Kris Maglione [:kmag]]

Comment on attachment 8776751 [details]
Bug 1213990 follow-up: remove old test of UninstallObserver uninit

https://reviewboard.mozilla.org/r/68426/#review65522

Comment 57

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out at aswan's request in https://hg.mozilla.org/integration/autoland/rev/7143e9551263 because it's apparently going to fail some mochitest.

Comment 58

[Andrew Swan [: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/

Comment 59

[Andrew Swan [: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/7-8/

Comment 60

[Jan Varga [:janv]]

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.

Comment 61

[Andrew Swan [:aswan]]

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.

Comment 62

[Andrew Swan [:aswan]]

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

Comment 63

[Pulsebot]

Pushed by [e-mail]:
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 64

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/autoland/rev/cfa39f8b7c54 for webextension test failures.

Comment 65

[Andrew Swan [:aswan]]

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 66

[Andrew Swan [:aswan]]

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 67

[Andrew Swan [: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/8-9/

Comment 68

[Luca Greco [:rpl] [:luca] [: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 69

[Andrew Swan [:aswan]]

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/

Comment 70

[Andrew Swan [: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/9-10/

Comment 71

[Matthew Wein [:mattw]]

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?

Comment 72

[Andrew Swan [:aswan]]

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.

Comment 73

[Matthew Wein [:mattw]]

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?

Comment 74

[Matthew Wein [:mattw]]

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.

Comment 75

[Andrew Swan [:aswan]]

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?

Comment 76

[Pulsebot]

Pushed by [e-mail]:
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

Comment 77

[Matthew Wein [:mattw]]

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.

Comment 78

[Andrew Swan [:aswan]]

hopefully third time is a charm...

Comment 79

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Unfortunately no.
Backed out in https://hg.mozilla.org/integration/autoland/rev/265e7ad32cbe for test failures: https://treeherder.mozilla.org/logviewer.html#?job_id=1371304&repo=autoland

Comment 80

[Andrew Swan [: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 81

[Andrew Swan [:aswan]]

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 82

[Andrew Swan [:aswan]]

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 83

[Andrew Swan [: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/10-11/

Comment 84

[Pulsebot]

Pushed by [e-mail]:
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

Comment 85

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28cd5d6f4541
https://hg.mozilla.org/mozilla-central/rev/9788d98cf100
https://hg.mozilla.org/mozilla-central/rev/b7348448a4c2

Comment 87

[Will Bamberg [:wbamberg]]

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.

Comment 88

[Andrew Swan [: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...

Comment 89

[Will Bamberg [:wbamberg]]

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