Closed
Bug 838541
Opened 12 years ago
Closed 12 years ago
Storage events leak from private browsing windows to normal windows
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
VERIFIED
FIXED
mozilla21
People
(Reporter: mozilla, Assigned: jdm)
References
Details
Attachments
(1 file)
10.16 KB,
patch
|
mayhemer
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Reporter | ||
Comment 1•12 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•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•12 years ago
|
Updated•12 years ago
|
Component: Untriaged → DOM
Product: Firefox → Core
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #711751 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 3•12 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•12 years ago
|
||
That's great. The MozStorageChanged event is new since I last wrote code for this. Very handy.
![]() |
||
Comment 5•12 years ago
|
||
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•12 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.
![]() |
||
Comment 7•12 years ago
|
||
Ian, Josh: based on comment 6, it seems we no longer need the PB transition observer, is that so?
Reporter | ||
Comment 8•12 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
![]() |
||
Comment 9•12 years ago
|
||
So, should http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsIPrivacyTransitionObserver.idl be removed?
Reporter | ||
Comment 10•12 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.
![]() |
||
Comment 11•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
Is hasn't gone through try, and the call looks correct to me.
![]() |
||
Comment 15•12 years ago
|
||
(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•12 years ago
|
||
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 17•12 years ago
|
||
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•12 years ago
|
||
Yeah, that's a good point. I feel a bit silly for not thinking of that.
Assignee | ||
Comment 19•12 years ago
|
||
Assignee | ||
Comment 20•12 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?
Comment 21•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment 22•12 years ago
|
||
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+
Assignee | ||
Comment 23•12 years ago
|
||
Comment 24•12 years ago
|
||
I confirm the fix is verified on Ubuntu 13.04 x86 on FF 20.0.1 and FF 21.0
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•