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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[JustOff]

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

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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!

Comment 2

[Andrea Marchesini [:baku]]

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?

Comment 3

[JustOff]

@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

[Ian Nartowicz]

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.

Comment 5

[Marcos Caceres [:marcosc]]

(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?

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

NI for comment 5

Comment 7

[JustOff]

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.

Comment 8

[Andrea Marchesini [:baku]]

In this case, I guess, we can have 2 separate notifications.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: storage.patch

Comment 10

[Andrea Marchesini [:baku]]

Janv, any news?

Comment 11

[Jan Varga [:janv]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7623a66ba08
dom-private-storage2-changed notification, r=janv

Comment 13

[Carsten Book [:Tomcat]]

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