dom-storage2-changed notifications are not useful for private windows

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: Off.Just.Off, Assigned: baku)

Tracking

({addon-compat})

48 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160923210021

Steps to reproduce:

1. Add a "dom-storage2-changed" observer
2. Start a normal browsing window
3. Start a private browsing window
4. Browse


Actual results:

"dom-storage2-changed" notifications are received from both normal and private windows


Expected results:

There should be two different notifications: "dom-storage2-changed" and "private-dom-storage2-changed" like it was done for cookies in bug 837091

Or notifications should indicate whether they relate to normal or private window

Or notifications should only be received from normal windows
Reporter

Updated

3 years ago
Keywords: addon-compat
OS: Unspecified → All
Hardware: Unspecified → All
Component: Storage → DOM
Product: Toolkit → Core
(In reply to JustOff from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101
> Firefox/48.0
> Build ID: 20160923210021
> 
> Steps to reproduce:
> 
> 1. Add a "dom-storage2-changed" observer
> 2. Start a normal browsing window
> 3. Start a private browsing window
> 4. Browse
> 
> 
> Actual results:
> 
> "dom-storage2-changed" notifications are received from both normal and
> private windows
> 
> 
> Expected results:
> 
> There should be two different notifications: "dom-storage2-changed" and
> "private-dom-storage2-changed" like it was done for cookies in bug 837091
> 
> Or notifications should indicate whether they relate to normal or private
> window
> 
> Or notifications should only be received from normal windows

Hi :baku, what do you think about the expected behaviours? Any idea of what we want to do here? Thanks!
Flags: needinfo?(amarchesini)
Assignee

Comment 2

3 years ago
Also private windows have localStorage and sessionStorage. I don't why they should receive a different notification.
dom-storage2-changed is an internal notification. having 2 different notification, is it useful for some other components?
Flags: needinfo?(amarchesini)
Reporter

Comment 3

3 years ago
@Andrea, it's useful for addons such as https://addons.mozilla.org/addon/self-destructing-cookies/ and https://addons.mozilla.org/addon/cookies-exterminator/ and so was done for cookies in bug 837091, is this not enough reason?

Comment 4

3 years ago
The use case is that if you are running in a private window and receive this notification relating to a change in a non-private window, then you are going to be very confused because the change will not be visible to you. The local storage areas in private and non-private windows are completely separate and have different contents.

Of course dom-storage2-changed may also be received in response to session storage changes that are not visible in a particular window, so overall its usefulness is limited. The MozSessionStorageChanged and MozLocalStorageChanged events (or MozStorageChanged before FF44) are much easier to work with at an individual window level. Or even the somewhat bizarrely-specified but official StorageEvent, which is only received in windows *other than* the one the storage area is in.
(In reply to JustOff from comment #3)
> @Andrea, it's useful for addons such as
> https://addons.mozilla.org/addon/self-destructing-cookies/ and
> https://addons.mozilla.org/addon/cookies-exterminator/ and so was done for
> cookies in bug 837091, is this not enough reason?

Is the self-destructing-cookies affected by this, or is a potential issue?
NI for comment 5
Flags: needinfo?(Off.Just.Off)
Reporter

Comment 7

3 years ago
I'm not an author of Self-Destructing Cookies, but yes, SDC is listening for dom-storage2-changed (https://addons.mozilla.org/en-US/firefox/files/browse/524654/file/lib/main.js#L247) to track and then to remove localStorage objects. Since SDC can't distinguish the private objects from the normal, it tries to do that for all recorded objects, regardless of their existance (https://addons.mozilla.org/en-US/firefox/files/browse/524654/file/lib/domstorage.js#L117, https://addons.mozilla.org/en-US/firefox/files/browse/524654/file/lib/domstorage.js#L302). It's not effective workaround, it wastes cpu and memory, even though that works.
Flags: needinfo?(Off.Just.Off)
Assignee

Comment 8

3 years ago
In this case, I guess, we can have 2 separate notifications.
Assignee

Comment 9

3 years ago
Posted patch storage.patchSplinter Review
Assignee: nobody → amarchesini
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8808529 - Flags: review?(jvarga)
Assignee

Comment 10

3 years ago
Janv, any news?
Flags: needinfo?(jvarga)
Comment on attachment 8808529 [details] [diff] [review]
storage.patch

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

::: dom/storage/DOMStorage.cpp
@@ +12,5 @@
>  #include "nsIScriptSecurityManager.h"
>  #include "nsIPermissionManager.h"
>  #include "nsIPrincipal.h"
>  #include "nsICookiePermission.h"
> +#include "nsGlobalWindow.h"

Nit: since this is not an interface it should technically go to the next include section.

@@ +188,5 @@
>  
>  private:
>    nsCOMPtr<nsISupports> mSubject;
>    const char16_t* mType;
> +  bool mPrivateBrowsing;

Nit: const bool

@@ +228,5 @@
>    RefPtr<StorageEvent> event =
>      StorageEvent::Constructor(nullptr, NS_LITERAL_STRING("storage"), dict);
>  
> +  bool privateBrowsing =
> +    mWindow && nsGlobalWindow::Cast(mWindow)->IsPrivateBrowsing();

I'm a bit nervous about this |if mWindow &&|
What if mWindow is null ? We will treat it as normal storage ?
There's DOMStorage::IsPrivate() method which can be used instead.
If you change it then you don't need to include nsGlobalWindow.h
Attachment #8808529 - Flags: review?(jvarga) → review+

Updated

3 years ago
Flags: needinfo?(jvarga)

Comment 12

3 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7623a66ba08
dom-private-storage2-changed notification, r=janv

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a7623a66ba08
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.