Storage events leak from private browsing windows to normal windows

VERIFIED FIXED in Firefox 20

Status

()

defect
VERIFIED FIXED
7 years ago
3 months ago

People

(Reporter: mozilla, Assigned: jdm)

Tracking

20 Branch
mozilla21
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox20+ verified, firefox21+ verified)

Details

Attachments

(1 attachment)

Reporter

Description

7 years ago
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?
Assignee

Updated

7 years ago
Assignee: nobody → josh
Blocks: PBnGen
Reporter

Comment 1

7 years ago
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.
Assignee

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → DOM
Product: Firefox → Core
Assignee

Comment 3

7 years ago
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.
Reporter

Comment 4

7 years ago
That's great.  The MozStorageChanged event is new since I last wrote code for this.  Very handy.
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.
Reporter

Comment 6

7 years ago
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.
Ian, Josh: based on comment 6, it seems we no longer need the PB transition observer, is that so?
Reporter

Comment 8

7 years ago
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
Reporter

Comment 10

7 years ago
(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.
(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!
(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 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?
Assignee

Comment 14

6 years ago
Is hasn't gone through try, and the call looks correct to me.
(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 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.
Attachment #711751 - Flags: review?(honzab.moz) → review+
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?
Assignee

Comment 18

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

Comment 20

6 years ago
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.
Attachment #711751 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/88044166268f
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Attachment #711751 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I confirm the fix is verified on Ubuntu 13.04 x86 on FF 20.0.1 and FF 21.0
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.