1313401 - Background Page LocalStorage is cleared along with site storage

Status: RESOLVED FIXED

(WebExtensions :: Storage, defect, P3)

Description

[M8R-p7]

If a user has their Privacy preferences set to clear cookies/site data on exiting the browser, WebExtensions also lose their background page's localStorage data. I understand from IRC #webextensions that this is undesired behavior.

STR:
0. Create a new profile.
1. Install a WebExtension that writes to localStorage from a background script.
2. Check that data written to localStorage persists across restarts.
3. Go to the Privacy Preferences page (about:preferences#privacy)
4. Choose 'Use custom settings for History' and select 'Keep until: I close (browser)'
5. Check that data written to localStorage persists across restarts.

Actual Results:
The addon data persists only when websites are allowed to persist data.

Expected Results:
The addon data should persist even if website data is discarded.

Comment 1

[M8R-p7]

Attachment: bug1313401_localstoragetest.zip

Testcase WebExtension. It reads any data currently stored in its LocalStorage and prints it to the console, then updates it with the current time.

Comment 2

[:shell escalante]

per triage check if happens in indexdb as well.  at very least need doc depending upon action.

Comment 3

[Kris Maglione [:kmag]]

Note that the problem here is that the setting in question puts us into permanent private browsing mode, which also applies to all extension contexts.

If/when we implement split incognito mode, this probably becomes a non-issue. In the mean time, we could force background page docshells to be non-private, but I'm not sure whether or not that's actually desirable.

Comment 4

[Andy McKay]

I think probably the best thing we can do here is provide some documentation for extension authors. 

I couldn't spot anything in MDN that suggested to extension authors when they should use which kind of storage, currently there's storage.local, storage.sync and the web APIs that could all be used. Would it be a good idea to detail that out and the implications for things like private browsing?

Comment 5

[Will Bamberg [:wbamberg]]

Yes, this sounds like a good idea. I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1317797.

Comment 7

[Will Bamberg [:wbamberg]]

> I think probably the best thing we can do here is provide some documentation for extension authors. 

Although I do think this should be documented, FWIW I don't agree that this is "the best thing we can do here".

I think private browsing, clearing history &c is generally understood to be about clearing data stored by websites, and I don't think add-ons are like websites, they are more like a part of the browser. It just happens that they use many of the same APIs as websites, because those APIs are familiar.

Put another way, it seems inconsistent to clear data for localStorage but not for storage.local.

By clearing this data, we make the APIs effectively unreliable for add-on authors - some percentage of their users will just think their add-on is broken. This isn't so bad for localStorage (although it is a footgun), because they can use the storage API instead. But it seems worse for IndexedDB, that doesn't have an equivalent WebExtension API.

Comment 8

[Kris Maglione [:kmag]]

(In reply to Will Bamberg [:wbamberg] from comment #7)
> I think private browsing, clearing history &c is generally understood to be
> about clearing data stored by websites, and I don't think add-ons are like
> websites, they are more like a part of the browser. It just happens that
> they use many of the same APIs as websites, because those APIs are familiar.
> 
> Put another way, it seems inconsistent to clear data for localStorage but
> not for storage.local.

The difference is that storage.local is shared between private and non-private windows. localStorage is a web API, and is different between private and non-private windows the same way that it is for other web content.

> By clearing this data, we make the APIs effectively unreliable for add-on
> authors - some percentage of their users will just think their add-on is
> broken. This isn't so bad for localStorage (although it is a footgun),
> because they can use the storage API instead. But it seems worse for
> IndexedDB, that doesn't have an equivalent WebExtension API.

Yes, that's particularly unfortunate for permanent private browsing, since there's no way around it. For other cases, extensions need to make sure IndexedDB code is running in a non-private window when the data needs to be permanent.

Comment 9

[Allan Gardner (:Mathnerd314)]

> localStorage is a web API, and is different between private and non-private windows the same way that it is for other web content.

Going by Bug 1277686 and https://developer.chrome.com/extensions/manifest/incognito, the incognito mode for all Webextensions is "spanning", and so *all* storage for WebExtensions (including localstorage and IndexedDB) should be shared across private and non-private windows. If it isn't, that's a separate bug.

> the setting in question puts us into permanent private browsing mode

Permanent private browsing mode is activated by the separate "Always use private browsing mode" checkbox / browser.privatebrowsing.autostart pref. 'Keep until browser close' sets network.cookie.lifetimePolicy to 2, hence is completely separate. 

Looking at 
https://dxr.mozilla.org/mozilla-central/rev/3e166b6838931b3933ca274331f9e0e115af5cc0/dom/base/nsContentUtils.cpp#9048
it seems easy enough to add an exception for "moz-extension", similar to "about" and the system (chrome) principal.

Comment 10

[Tom S [:evilpie]]

This is listed by the uBlock dev as an issue for them. https://github.com/gorhill/uBlock/issues/2795

Comment 12

[Tom S [:evilpie]]

Hey Bob! I think Kris indicated on IRC that you could maybe take a look at this instead?

Comment 13

[voltron]

(In reply to Allan Gardner (:Mathnerd314) from comment #9)
> Going by Bug 1277686 and
> https://developer.chrome.com/extensions/manifest/incognito, the incognito
> mode for all Webextensions is "spanning", and so *all* storage for
> WebExtensions (including localstorage and IndexedDB) should be shared across
> private and non-private windows. 

This is exactly how it should work. 

Otherwise IndexedDB is not available for extensions in private browsing mode which makes absolutely no sense at all since extensions need that API the most.

Comment 14

[Bob Silverberg [:bsilverberg]]

TBH I'm not sure where this bug stands and what, if anything needs to be done about it. I'll summarize what I think has been said thus far:

1. Kris has said that the setting in question puts us into permanent private browsing mode, but Allan (in comment 9) suggests that is not the case. Which is true?

2. Kris suggested a possible solution to the problem would be to force background page docshells to be non-private, but was not sure if that's actually desirable. Can we make a definitive call on that?

3. Both Allan and JV state that "spanning" mode should not have the displayed behaviour. I.e., "spanning" mode should allow localStorage and IndexedDB to be shared across private and non-private windows, and suggest that this is a bug. I'm not sure if that's actually the case. The Chrome docs for "spanning" [1] talk about a shared process to which an incognito tab can send events or messages, but also states that "incognito tabs cannot use this shared process" which could be interpreted to mean they would not have access to information stored there (e.g., localStorage and IndexedDB). We need some clarification about whether this is in fact a bug or is expected behaviour.

4. There was a suggestion by Allan that we could add an exception for "moz-extension", similar to "about" and the system (chrome) principal, based on https://dxr.mozilla.org/mozilla-central/rev/3e166b6838931b3933ca274331f9e0e115af5cc0/dom/base/nsContentUtils.cpp#9048. Is that a viable solution to this problem, and if so, does it solve all the issues?

[1] https://developer.chrome.com/extensions/manifest/incognito#spanning

Comment 16

[Tom S [:evilpie]]

Can we implement solution 4? Would we need to still clean this data when uninstalling the extension, I guess we should follow whatever browser.storage.local is doing? I just ran across another extension author who ran into this: https://www.reddit.com/r/imagus/comments/722al4/small_bug_in_latest_firefox_extension_update/dnfimb9/

Comment 17

[Denis]

We ran into this issue as well(FVD Speed Dial extension). When the history setting is "Never Remember History" IndexedDB fails on the .open call with the .onerror listener triggered. We really need to use IndexedDB, because our legacy addon used sqlite, so IndexedDB is the only option for us. For the users with such privacy settings our addon will be just unexpectedly broken in firefox 57.

Comment 18

[Tom S [:evilpie]]

I think this bug is very important and should be fixed as soon as possible. So I looked into solution 4 from comment 14 again. And by reading the code it looks like we still respect private-browsing mode even for about:// URLs. I am not even sure we can fix this by returning StorageAccess::eAllow in nsContentUtils::InternalStorageAllowedForPrincipal.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

In general I deal with our storage stuff very rarely, so perhaps better to ask Jan's opinion here.
And ehsan might have some opinion on private browsing mode.

I would expect that we need to add some exception in case the principal is something from a webextension, and not clear in such case. But I'm not familiar with what kinds of principals webextensions use.

Comment 20

[Alex]

Run into this issue today. We also use indexedDB heavily in the extension's popup and setting "Use custom settings for history" and unchecking "Accept cookies from websites" blocks popups from accessing indexedDB.

Is the fix decided for this?

Comment 21

[voltron]

I think the whole bunch of problems with IndexedDB and localStorage related to WebExtensions could be summarized into a simple specification:

"WebExtension *Background page* can use IndexedDB and window.localStorage always. Contents of these storages are deleted only when the user removes the extension or when the extension code itself decides to clear them by calling related APIs."

In other words, IndexedDB and window.localStorage for the WebExtension _background page_ (<-- this is the key) should work regardless of any cookie settings, remember history settings, "clear history when Firefox closes" settings or Private Browsing mode. Those should not limit what the background page can do.

Comment 22

[Bob Silverberg [:bsilverberg]]

Kris, I apologize for not being able to deal with this myself, but much of this discussion around option 4 from comment #14 is over my head. I'm being asked questions about things I don't understand, so I think it might be best if you tried to jump in and address some of the things that are being asked.

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

I think this bug is likely going to be addressed by 1406675 which is taking the solution 4 approach proposed in comment 14.  In https://bugzilla.mozilla.org/show_bug.cgi?id=1406675#c15 I agree with :evilpie's comment 18 here that private browsing mode is a separate implementation issue.

I think it probably makes sense to dupe this bug to bug 1406675, but there's a lot of discussion here and a lot of CC's and I don't want to spuriously dupe.

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

As I described in Bug 1406675 comment 21, I confirm that the issue reported in Comment 0 is going to be fixed
by the patched attached on Bug 1406675 (and Bug 1406675 attachment 8922884 [details] contains a new test for this scenario)

The user preference described in comment 0:

- Choose 'Firefox will Use custom settings for History' and select 'Keep until: I close (browser)'

does not enable the "permanent private browsing mode" (that on the contrary is enabled by choosing "Firefox will Never remember history", or "Firefox will Use custom settings for History" and then select "Always use private browsing mode")
but set "network.cookie.lifetimePolicy" to 2, which makes localStorage to switch into session-mode (which is primed by the stored data, but the changes to the data are not persisted and the cached dropped as soon as we exit Firefox).

There is also another preference that has a similar outcome:

"Firefox will use custom settings for history", and then check “Clear history when Firefox closes”

This is a separate issue (related to the underlying "privacy.sanitize.sanitizeOnShutdown" about:config preference), and I filed it as Bug 1416219 (I've also added some additional details in the bug summary).

Comment 26

[Jan Varga [:janv]]

Bug 1406675 has been fixed.
Reporter, can we close this now ?

Comment 27

[vtol]

bug #1416219 has not been patched and WX local storage still gets purged in FF 61.0.1 (64-bit) on W10. Unless that dependency is not relevant to this bug and thus should be removed then?

Comment 28

[Jan Varga [:janv]]

Oh, I overlooked comment 24.

Comment 29

[u608312]

In case it is useful, I'd like to highlight a specific use case in support of getting this bug resolved:

The Privacy Pass extension uses local storage to persist 'passes' - to avoid CAPTCHAs (served by CloudFlare & others under certain circumstances, e.g. by using Tor or a VPN). The passes use blind signature cryptography to ensure they are fully anonymous for privacy concious users. This data thus seems an ideal example of something which it ought to be possible to retain, whilst purging all fingerprintable/trackable content resulting from web page visits.  Users of the extension in Chrome can keep passes whilst clearing all browsing data on exit and the difference in behaviour is confusing some: https://github.com/privacypass/challenge-bypass-extension/issues/8

IMHO, a best case scenario would be the option for the Firefox user to be able to specifically request extension-related data be cleared, but the default behaviour to be oherwise.

Comment 30

[Luca Greco [:rpl] [:luca] [:lgreco]]

This bug may be actually fixed when LSNG is enabled by default, if that is the case we should reverse the dependency and make this bug to be blocked on enabling LSNG by default.

Added needinfo to double-check that.

Comment 31

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1313401 - Add test to ensure run offlineApps Sanitizer does not affect extensions. r?mixedpuppy!

Comment 32

[Luca Greco [:rpl] [:luca] [:lgreco]]

The attached patch adds a new test case to verify that running the offlineApps Sanitizer doesn't clear the extension's localStorage and IndexedDB data. The test passes with LSNG enabled (as expected) but fails with the previous localStorage implementation (and so that test case is currently skipped in the patch if LSNG is disabled using its related pref).

Changed issue dependencies to reflect that this issue is going to be fixed on release once LSNG is being enabled by default on all channels (tracked by Bug 1599979).

Comment 34

[Luca Greco [:rpl] [:luca] [:lgreco]]

As mentioned in comment 32 this issue is going to be fixed once LSNG becomes the default on the release channels.

In the meantime I'm going to land the test case attached to this issue, to prevent the behavior from regressing on LSNG, but I'm adding the leave-open keyword to keep this issue open until Bug 1599979 is being marked as fixed.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd068e4a0ea2
Add test to ensure run offlineApps Sanitizer does not affect extensions. r=mixedpuppy

Comment 36

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/bd068e4a0ea2

Comment 37

[Alfredo]

Hi,
any update about this bug?

Comment 38

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Nedo from comment #37)

Hi,
any update about this bug?

This bug depends on Bug 1599979, which is about LSNG (nextgen localStorage) being enabled by default on release, the patch attached to this bug is making sure that when LSNG is enabled the offlineApps Sanitizer doesn't clear the localStorage data for the extensions.

Comment 40

[Luca Greco [:rpl] [:luca] [:lgreco]]

As mentioned in comment 38 this bug is meant to be closed once LSNG has been enabled on all channels (Bug 1599979).

Bug 1599979 doesn't have a priority set, but Bug 1711922 is tracking a staged rollout and its priority is set to P3.

In the meantime, I'm clearing the assignment (to reflect that none of us is actively working on this anymore, and it is just waiting for the fix to reach release) and changing the priority to P3.

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:mixedpuppy, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Comment 43

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 7 duplicates, 16 votes and 51 CCs.
:mixedpuppy, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 44

[Luca Greco [:rpl] [:luca] [:lgreco]]

I'm marking this issue as fixed, LSNG has been enabled on release channel and we landed an automated test (attached to this bug) which make sure that this issue isn't reproducible when NextGen LocalStorage is in use.