The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: JustOff, Assigned: baku)

Tracking

({addon-compat})

48 Branch
mozilla53
addon-compat
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 months 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

5 months ago
Keywords: addon-compat
OS: Unspecified → All
Hardware: Unspecified → All

Updated

5 months ago
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

5 months 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

5 months 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

5 months 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

5 months 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

5 months ago
In this case, I guess, we can have 2 separate notifications.
(Assignee)

Comment 9

5 months ago
Created attachment 8808529 [details] [diff] [review]
storage.patch
Assignee: nobody → amarchesini
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8808529 - Flags: review?(jvarga)
(Assignee)

Comment 10

4 months ago
Janv, any news?
Flags: needinfo?(jvarga)

Comment 11

4 months ago
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

4 months ago
Flags: needinfo?(jvarga)

Comment 12

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

Comment 13

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a7623a66ba08
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.