1630763 - localstorage is different for each tab when dynamic FPI is active

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, defect, P1)

Description

[Steven Englehardt [:englehardt]]

STR: In a fresh nightly profile with network.cookie.cookieBehavior = 5, manually open 3 tabs and visit https://senglehardt.com/test/dfpi/first_and_third.html in each tab.

ER: englehardt-tracker.com's localStorage is the same across tabs.

AR: englehardt-tracker.com's localStorage is different in each tab. It's behaving similar to sessionStorage.

This does not reproduce with the default cookie policy, nor with privacy.firstparty.isolate=true.

Comment 1

[Gary Chen [:xeonchen]]

It is designed to share the cache:
https://searchfox.org/mozilla-central/rev/567b68b8ff4b6d607ba34a6f1926873d21a7b4d7/dom/base/nsGlobalWindowInner.cpp#4617

But

1. the storage (PartitionedLocalStorage) being used here is in-memory-only
2. the tabs here are in different process, SessionStorageCache is newly created in each process.

So the caches are not shared correctly.

Comment 2

[Gary Chen [:xeonchen]]

(In reply to Gary Chen [:xeonchen] from comment #1)

1. the tabs here are in different process, SessionStorageCache is newly created in each process.

Enabling Fission will fix his issue, and I can't find proper synchronization mechanism on SessionStorageManager without Fission.
To make it synchronized might not be easy, I think we need more discussion on this bug.

Comment 3

[Steven Englehardt [:englehardt]]

Thanks Gary! From what I can tell this is a bug introduced by Bug 1558420. The comments in https://searchfox.org/mozilla-central/rev/567b68b8ff4b6d607ba34a6f1926873d21a7b4d7/dom/base/nsGlobalWindowInner.cpp#4489-4492 seem to imply that we only intended to expose the in-memory localStorage object to origins in pref: privacy.restrict3rdpartystorage.partitionedHosts. This seems to align with Ehsan's comments in Bug 1558420 Comment 17.

The StoragePrincipal should just take care of things for us, and indeed we check for a mismatch here. I'll attach a quick and dirty patch that seems to fix things: cross-tab localStorage is the same before isolation, and switches after a storage access permission is granted. I haven't thought through all of the logic for our various cookie policies nor have I run any tests, but I'd like baku to verify that this is indeed a bug.

Comment 4

[Steven Englehardt [:englehardt]]

Attachment: bug1630763.patch

Comment 5

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1630763-ensure-dFPI-uses-correct-storage-type.patch

After reading Steven's comment, I made some further changes to ensure PartitionedLocalStorage is used only when isolated is true. Please take a look and see if the test case works for this bug.

Comment 6

[Andrea Marchesini [:baku]]

Yes, this is reasonable. I haven't checked the patch, because I see Gary is working on it.
PartitionLocalStorage was introduced because, before StoragePrincipal and dFPI, some websites did not work as expected with a blocked localStorage. We had to expose a "in-memory-only" localStorage object. But this is not needed for dFPI.

Comment 7

[Steven Englehardt [:englehardt]]

Comment on attachment 9141480 [details] [diff] [review]
0001-Bug-1630763-ensure-dFPI-uses-correct-storage-type.patch

Review of attachment 9141480 [details] [diff] [review]:
-----------------------------------------------------------------

This change makes the logic much clearer, thanks for restructuring. I've tested the patch locally and it works as expected on the test page in Comment 0.

Comment 8

[Steven Englehardt [:englehardt]]

I think we should also add to this comment to make it clear that this is different than the partitioning we want in dFPI or other OA-based partioning: https://searchfox.org/mozilla-central/rev/d2cec90777d573585f8477d5170892e5dcdfb0ab/dom/base/nsGlobalWindowInner.cpp#4471-4474.

Perhaps:

// For Partitioned Trackers, we expose a partitioned LocalStorage, which
// doesn't share data with other contexts, and it's just in memory.
// Partitioned localStorage is available only for trackers listed in the
// privacy.restrict3rdpartystorage.partitionedHosts pref. See
// nsContentUtils::IsURIInPrefList to know the syntax for the pref value.
// This is a temporary web-compatibility hack, and is unrelated to the
// partitioning provided by origin attributes (e.g., FPI, dFPI, or containers).

Gary: I've assigned you to the bug since you already have a patch ready. Thanks for updating the test as well!

Comment 9

[Gary Chen [:xeonchen]]

Attachment: Bug 1630763 - ensure PartitionedLocalStorage is only used in correct mode; r=baku

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b94981d467ea
ensure PartitionedLocalStorage is only used in correct mode; r=baku

Comment 11

[Cristina Coroiu [:ccoroiu]]

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