Closed Bug 1509132 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Protections UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: asuth, Assigned: baku)

Details

Attachments

(1 file)

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
Status: ASSIGNED → RESOLVED
Closed: 6 years 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.

Attachment

General

Created:
Updated:
Size: