1595431 - browsingdata.remove removes browsing data from subdomains of hostnames passed

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Vuk Mirovic]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

Go to test.vukmirovic.org.
It will print LocalStorage: Not found...
Refresh
It will print LocalStorage: Found!
Load example extension that calls
browser.browsingData.remove(
{ "hostnames": ["vukmirovic.org"] },
{ "localStorage": true },
);
every 5s from https://github.com/wooque/test-browsingdata
Refresh page after 5s
It will print LocalStorage: Not found...

Actual results:

Subdomain LocalStorage got removed when browsingData.remove got called to remove LocalStorage for base domain.

Expected results:

Subdomain LocalStorage should stay intact when when browsingData.remove got called to remove LocalStorage for base domain.

Comment 1

[Rob Wu [:robwu]]

We need to investigate this.

If "hostnames" turns out to behave as you describe (then it should have been called "domains"), then we can consider introducing the "origins" option (because that behaves in the way you requested, and is also supported by Chrome).

Comment 2

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

I took a look into this issue and it seems to only affect the browsingData API when the LSNG (nextGen LocalStorage) is disabled, but it seems to be working as expected when it is enabled (currently the defaults on Nightly):

• when LSNG is enabled we do clear the localStorage data for the given hostnames using quotaManagerService.clearStoragesForPrincipal

• on the contrary when LSNG is disabled, we do notify "extension:purge-localStorage" to the observers subscribed to it, and then the data is going to be cleared by StorageDBThread.cpp using a sqlite "DELETE FROM webappsstore2 WHERE originKey GLOB :scope" statement

LSNG is still disabled in 73 (See Bug 1608449), but it still doesn't seem worth to me to look into fixing the issue on the old localStorage implementation, and so I'm going to add Bug 1599979 (Enable LSNG on Release) as a dependency for this bug.

In the meantime I'm also going to attach to this issue a patch which adds an assertion to explicitly verify this scenario as part of our automated tests.

Comment 3

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

Attachment: Bug 1595431 - Add test case to verify that browsingData.remove clears localStorage only for the given hostname. r?mixedpuppy!

Comment 4

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

As mentioned in comment 2 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 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dde426d9e47d
Add test case to verify that browsingData.remove clears localStorage only for the given hostname. r=mixedpuppy

Comment 6

[Daniel Varga [:dvarga]]

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

Comment 7

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

The leave-open keyword is there and there is no activity for 6 months.
:rpl, maybe it's time to close this bug?

Comment 8

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

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #7)

The leave-open keyword is there and there is no activity for 6 months.
:rpl, maybe it's time to close this bug?

The leave-open keyword was added to avoid to have this bug auto-closed when we landed the additional test case attached to this issue,
I'll remove it because we don't need it to stay set at the moment.

Nevertheless, I'm not closing the issue because the bug is still not fixed, it will be fixed once LSNG is enabled on all channels (which is tracked by Bug 1599979, linked to this issue as a dependency).

Comment 9

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

As mentioned in comment 8 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 10

[Rob Wu [:robwu]]

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

LSNG is now enabled by default. The pref has been renamed to discourage its use in bug 1764696.

Can this bug be closed now? Alternatively, to clean up, we can remove some (always skipped) parts of the tests at:

• https://searchfox.org/mozilla-central/rev/59f0bf3c13dd455d9f5415b89178de701ea6b850/toolkit/components/extensions/test/xpcshell/test_ext_storage_sanitizer.js#94-101
• https://searchfox.org/mozilla-central/rev/59f0bf3c13dd455d9f5415b89178de701ea6b850/toolkit/components/extensions/test/mochitest/test_ext_browsingData_localStorage.html#293-300

The code can also trivially be simplified now:

• https://searchfox.org/mozilla-central/rev/59f0bf3c13dd455d9f5415b89178de701ea6b850/toolkit/components/extensions/parent/ext-browsingData.js#217-220,233,235
• https://searchfox.org/mozilla-central/rev/59f0bf3c13dd455d9f5415b89178de701ea6b850/toolkit/components/extensions/parent/ext-browsingData.js#316-320

Unrelated to browsingData, but in a similar vein, this code can be removed too:

• https://searchfox.org/mozilla-central/rev/59f0bf3c13dd455d9f5415b89178de701ea6b850/toolkit/components/extensions/Extension.jsm#544-556