AntiTracking needs to re-GetLocalStorage() when granting storage access to a parent window to ensure "storage" events are hooked up when switching from PartitionedLocalStorage

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P1
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: asuth, Assigned: baku)

Tracking

unspecified
Firefox 65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

As discussed on IRC with :baku, currently when AntiTracking grants permission to use real storage to a window, the switch from PartitionedLocalStorage to real LocalStorage does not happen until the next GetLocalStorage() call happens.  GetLocalStorage() notices partitioned LS is in use and then creates the real LocalStorage instance.  The creation of the real LocalStorage hooks up the LocalStorage e10s propagation that is necessary for correct "storage" semantics.  Requiring the content page to trigger a localStorage access is too late, so it's necessary for the permission grant to trigger GetLocalStorage if there is already a (fake) mLocalStorage.
Attachment #9026895 - Flags: review?(ehsan)
Attachment #9026895 - Flags: feedback?(bugmail)
Priority: -- → P1
Comment on attachment 9026895 [details] [diff] [review]
notifyWindow.patch

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

::: dom/base/nsGlobalWindowInner.cpp
@@ +7991,5 @@
> +{
> +  if (mLocalStorage &&
> +      mLocalStorage->Type() == Storage::ePartitionedLocalStorage) {
> +    IgnoredErrorResult error;
> +    GetLocalStorage(error);

Maybe add a comment here explaining why this is needed?
Attachment #9026895 - Flags: review?(ehsan) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f77d9f3460c
Inform the 3rd party tracker window when the storage permission is granted, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/5f77d9f3460c
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
FYI, additional patch for LSNG looks like this:

diff --git a/dom/base/nsGlobalWindowInner.cpp b/dom/base/nsGlobalWindowInner.cpp
--- a/dom/base/nsGlobalWindowInner.cpp
+++ b/dom/base/nsGlobalWindowInner.cpp
@@ -8027,16 +8026,24 @@ nsGlobalWindowInner::StorageAccessGrante
     IgnoredErrorResult error;
     GetLocalStorage(error);
     if (NS_WARN_IF(error.Failed())) {
       return;
     }

     MOZ_ASSERT(mLocalStorage &&
                mLocalStorage->Type() == Storage::eLocalStorage);
+
+    if (NextGenLocalStorageEnabled() &&
+        mListenerManager &&
+        mListenerManager->HasListenersFor(nsGkAtoms::onstorage)) {
+      auto object = static_cast<LSObject*>(mLocalStorage.get());
+
+      object->EnsureObserver();
+    }
   }
 }

 mozilla::dom::TabGroup*
 nsPIDOMWindowInner::TabGroup()
 {
   return nsGlobalWindowInner::Cast(this)->TabGroupInner();
 }
more changes needed for LSNG:
diff --git a/toolkit/components/antitracking/test/browser/localStorage.html b/toolkit/components/antitracking/test/browser/localStorage.html
--- a/toolkit/components/antitracking/test/browser/localStorage.html
+++ b/toolkit/components/antitracking/test/browser/localStorage.html
@@ -1,15 +1,22 @@
 <h1>Here a tracker!</h1>
 <script>
 
 if (window.opener) {
   SpecialPowers.wrap(document).userInteractionForTesting();
   localStorage.foo = "opener" + Math.random();
-  window.close();
+  // Don't call window.close immediatelly. It can happen that adding the
+  // "storage" event listener below takes more time than usual (it may need to
+  // synchronously subscribe in the parent process to receive storage
+  // notifications). Spending more time in the initial script can prevent
+  // the "load" event from being fired for the window opened by "open and test".
+  setTimeout(() => {
+    window.close();
+  }, 0);
 }
 
 if (parent) {
   window.onmessage = e => {
     if (e.data == "test") {
       let status;
       try {
         localStorage.foo = "value" + Math.random();
Comment on attachment 9026895 [details] [diff] [review]
notifyWindow.patch

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

Love the comment in nsGlobalWindowInner.h!
Attachment #9026895 - Flags: feedback?(bugmail) → feedback+
You need to log in before you can comment on or make changes to this bug.