838541 - Storage events leak from private browsing windows to normal windows

Status: VERIFIED FIXED

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

Description

[Ian Nartowicz]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:10.0.10) Gecko/20100101 Firefox/10.0.10
Build ID: 20121024073834

Steps to reproduce:

1. Open a private browsing window with an event listener on "storage" event.
2. Open a normal window to the same page with an event listener on "storage" event.
3. Write to DOM storage in the private browsing window.



Actual results:

Event fires in the normal window.  The event contains a copy of the private window storage item before and after the change.


Expected results:

Preferably DOM storage events should only fire in windows with the same private browsing status.

There is an observer notification "dom-storage2-changed" which is fired from the DOM storage code and is used by nsGlobalWindow to send out the events.  Possibly that is what needs to change, in a similar way to the cookie-changed notifications?

Comment 1

[Ian Nartowicz]

It may be more important to change the observer notification than the events themselves, at least for addons.  Because the events are specified not to be triggered in the window that made the storage change, they are nearly useless and I can't imagine very many people are using them.  The observer notifications are obviously received by everyone regardless, but at least you can be sure of getting them.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Only dispatch storage events to windows of the same privacy status.

Comment 3

[Josh Matthews [:jdm]]

Fixing the storage events is far more important than the notification, since it actually affects web content. Addons can watch for the MozStorageChanged DOM event which is fired on the original window.

Comment 4

[Ian Nartowicz]

That's great.  The MozStorageChanged event is new since I last wrote code for this.  Very handy.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 711751 [details] [diff] [review]
Only dispatch storage events to windows of the same privacy status.

So, you are carrying the IsPrivate() state of the storage via the actual storage object bound to the event.

Problem is that the event is posted as an async event.  The storage private state could potentially change between dispatch and handling.

I think changes to private state of the load context will immediately reflect state of all bound storages.  Is today possible the context changes at run-time somehow?  

If not, then the patch is OK.

If yes, then it means that you may still in some corner case receive a private event in non-private context and vice versa.  Am I right?  The privacy flag should be then carried in the event, and not read from the storage object.

Comment 6

[Ian Nartowicz]

As of per-window private browsing, windows cannot change privacy status.  Whatever they were opened with, that's the way they stay until they're closed.  I think that means storage areas cannot change state, ever.  I'm a little hazy on how it all hangs together, because there seems to be a single DOM storage shared between all the private windows, but still seems that it cannot ever change.

Comment 7

[Honza Bambas (:mayhemer)]

Ian, Josh: based on comment 6, it seems we no longer need the PB transition observer, is that so?

Comment 8

[Ian Nartowicz]

The following observer notifications will no longer occur:
private-browsing
private-browsing-cancel-vote
private-browsing-change-granted
private-browsing-transition-complete

New observer notifications indicate that the last private browsing window is closing, in case you need to do something at that point:
last-pb-context-exiting
last-pb-context-exited

Comment 9

[Honza Bambas (:mayhemer)]

So, should http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIPrivacyTransitionObserver.idl be removed?

Comment 10

[Ian Nartowicz]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> So, should
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/
> nsIPrivacyTransitionObserver.idl be removed?

Seems like you shouldn't need it.  The only use now is to notify the storage code at initialisation (of the doc shell?) that a window is a private browsing window.  Should never be called again.

So assuming you can determine by other means that you're in a private browsing window when you initialise the storage areas then you should be good to go.  Then it can never change.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Ian Nartowicz from comment #10)
> So assuming you can determine by other means that you're in a private
> browsing window when you initialise the storage areas then you should be
> good to go.  Then it can never change.

Time for a new bug to remove that bloody interface then!

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #11)
> Time for a new bug to remove that bloody interface then!

Ehm.. bug 800195.  Now I found it...

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 711751 [details] [diff] [review]
Only dispatch storage events to windows of the same privacy status.

>+++ b/dom/tests/browser/browser_localStorage_privatestorageevent.js

>+  let loads = 0;
>+  function waitForLoad() {
>+    loads++;
>+    if (loads != 2) {
>+      return;
>+    };
>+    pubWin.postMessage('setKey', 'http://mochi.test:8888');

Is this call correct?


Also, did this go through try?

Comment 14

[Josh Matthews [:jdm]]

Is hasn't gone through try, and the call looks correct to me.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> >+  let loads = 0;
> >+  function waitForLoad() {
> >+    loads++;
> >+    if (loads != 2) {
> >+      return;
> >+    };
> >+    pubWin.postMessage('setKey', 'http://mochi.test:8888');
> 
> Is this call correct?

Ah, it's called after all test went through.  I missed this first.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 711751 [details] [diff] [review]
Only dispatch storage events to windows of the same privacy status.

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

r=honzab

::: dom/interfaces/storage/nsPIDOMStorage.h
@@ +44,5 @@
>    virtual bool CanAccess(nsIPrincipal *aPrincipal) = 0;
>  
>    virtual nsDOMStorageType StorageType() = 0;
> +
> +  virtual bool IsStoragePrivate() = 0;

Please rename to IsPrivate()

::: dom/src/storage/nsDOMStorage.h
@@ +327,5 @@
>    virtual nsIPrincipal* Principal();
>    virtual bool CanAccess(nsIPrincipal *aPrincipal);
>    virtual nsDOMStorageType StorageType();
> +  virtual bool IsStoragePrivate() {
> +    return mStorageImpl->IsPrivate();

rather add null check as well.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 711751 [details] [diff] [review]
Only dispatch storage events to windows of the same privacy status.

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

::: dom/base/nsGlobalWindow.cpp
@@ +9316,5 @@
>      nsPIDOMStorage::nsDOMStorageType storageType = pistorage->StorageType();
>  
> +    nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(GetDocShell());
> +    bool isPrivate = loadContext && loadContext->UsePrivateBrowsing();
> +    if (pistorage->IsStoragePrivate() ^ isPrivate) {

And BTW, this could use just != instead of XOR, no?

Comment 18

[Josh Matthews [:jdm]]

Yeah, that's a good point. I feel a bit silly for not thinking of that.

Comment 19

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88044166268f

Comment 20

[Josh Matthews [:jdm]]

Comment on attachment 711751 [details] [diff] [review]
Only dispatch storage events to windows of the same privacy status.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 722857
User impact if declined: Web sites will receive incorrect DOM events if the same page is open in both a public and private window. 
Testing completed (on m-c, etc.): m-c, automated test
Risk to taking this patch (and alternatives if risky): UUID change; rest of change isn't risky.
String or UUID changes made by this patch: nsPIDOMStorage - internal interface unlikely to be used by addons.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88044166268f

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 711751 [details] [diff] [review]
Only dispatch storage events to windows of the same privacy status.

Interface change is OK on Aurora so let's get this uplifted there this week.

Comment 23

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/94c2049e7483

Comment 24

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on Ubuntu 13.04 x86 on FF 20.0.1 and FF 21.0