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)
Firefox
Protections UI
Tracking
()
RESOLVED
FIXED
Firefox 65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: asuth, Assigned: baku)
Details
Attachments
(1 file)
3.50 KB,
patch
|
ehsan.akhgari
:
review+
asuth
:
feedback+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #9026895 -
Flags: review?(ehsan)
Attachment #9026895 -
Flags: feedback?(bugmail)
Updated•6 years ago
|
Priority: -- → P1
Comment 2•6 years ago
|
||
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
Comment 4•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Comment 5•6 years ago
|
||
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();
}
Comment 6•6 years ago
|
||
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();
Reporter | ||
Comment 7•6 years ago
|
||
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.
Description
•