Closed
Bug 722857
Opened 13 years ago
Closed 13 years ago
DOMStorage should obtain Private Browsing information from related docshell
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: jdm, Assigned: jdm)
References
Details
(Keywords: addon-compat, dev-doc-needed)
Attachments
(2 files, 12 obsolete files)
52.36 KB,
patch
|
Details | Diff | Splinter Review | |
13.21 KB,
patch
|
Details | Diff | Splinter Review |
The global PB service is going away and the DOMStorageManager uses it to maintain a global state for consumers. I have a WIP patch that looks in the JS stack and finds the most recent related docshell, which sounds like it might work for content script.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
Assignee | ||
Comment 3•13 years ago
|
||
Comment on attachment 593345 [details] [diff] [review]
Determine private browsing status dynamically from most recent docshell accessible WIP
Honza, is this sane? Do you have any thoughts on a better way to accomplish this?
Attachment #593345 -
Flags: feedback?(honzab.moz)
Comment 4•13 years ago
|
||
Comment on attachment 593345 [details] [diff] [review]
Determine private browsing status dynamically from most recent docshell accessible WIP
Review of attachment 593345 [details] [diff] [review]:
-----------------------------------------------------------------
nsDOMStorageManager::InPrivateBrowsingMode() is used in both chrome and child process. And also in IndexedDB code which I'm not familiar with. I believe there will be cases you will access storage from a different context it was previously made for.
This won't be, as usual, that simple:
For DOM storage you will have to set the flag of the PB state directly on the storage instance when it is being created by 1) docshell in case of sessionStorage, 2) global window in case of localStorage and globalStorage (there is patch to remove GS very soon, so I wouldn't bother about it!).
You have to carry this flag to the parent process to let DOMStorageImpl know. (the flag wont change during the instance(s) lifetime)
You have to change all calls to nsDOMStorageManager::gStorageManager->InPrivateBrowsingMode() in nsDOMStorageDBWrapper to query the flag on the storage, where possible. Where not, probably assume PB:off, but maybe for more discussion or followups.
You have to ensure to always get rid of cached instances of sessionStorage/localStorage when PB mode is left for a docshell/global window.
Attachment #593345 -
Flags: feedback?(honzab.moz) → feedback-
Assignee | ||
Comment 5•13 years ago
|
||
Just checkpointing my work. I still need to figure out what to do in some of the nsDOMStorageWrapperDB methods.
Assignee | ||
Updated•13 years ago
|
Attachment #593345 -
Attachment is obsolete: true
Assignee | ||
Comment 6•13 years ago
|
||
This is close to being ready for review. I just need to fix up the content process handling for the observer notification and test what happens when the same storage is accessed by a private and non-private context concurrently.
Assignee | ||
Updated•13 years ago
|
Attachment #598528 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs updated patch]
Assignee | ||
Comment 7•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #599926 -
Attachment is obsolete: true
Assignee | ||
Comment 8•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #593343 -
Attachment is obsolete: true
Assignee | ||
Comment 9•13 years ago
|
||
I still need to test concurrent access; these are just rebased and multiprocess-friendly.
Assignee | ||
Comment 10•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #616893 -
Attachment is obsolete: true
Assignee | ||
Comment 11•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #617027 -
Attachment is obsolete: true
Assignee | ||
Comment 12•13 years ago
|
||
Comment on attachment 617028 [details] [diff] [review]
Basic sanity test for before/after private browsing DOM storage implementation.
These tests ensure that IndexedDB is denied in private mode; that concurrent access to the same localStorage doesn't allow values to leak between privacy context; and that a simple enter and exit + set value test shows the correct behaviour.
Attachment #617028 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 13•13 years ago
|
||
Comment on attachment 616892 [details] [diff] [review]
Determine private browsing status dynamically from most recent docshell accessible WIP
Here's the meat of the work.
Attachment #616892 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → josh
Whiteboard: [needs updated patch] → [needs review]
Assignee | ||
Comment 14•13 years ago
|
||
Now with tests that pass as part of the overall test suite and not just when run standalone.
Attachment #617272 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•13 years ago
|
Attachment #617028 -
Attachment is obsolete: true
Attachment #617028 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•13 years ago
|
Attachment #617272 -
Attachment is obsolete: true
Attachment #617272 -
Flags: review?(honzab.moz)
Comment 16•13 years ago
|
||
Josh, I started the review. Check bug 536509, it may collide (by means of need to merge) with your patches.
Comment 17•13 years ago
|
||
Comment on attachment 616892 [details] [diff] [review]
Determine private browsing status dynamically from most recent docshell accessible WIP
Review of attachment 616892 [details] [diff] [review]:
-----------------------------------------------------------------
Doesn't look bad.
r=honzab with some details addressed.
::: dom/base/nsGlobalWindow.cpp
@@ +8262,5 @@
> return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
> }
> +
> + nsCOMPtr<nsIPrivacyTransitionObserver> obs = do_QueryInterface(mSessionStorage);
> + mDocShell->AddWeakPrivacyTransitionObserver(obs);
Indention
@@ +8312,5 @@
>
> + nsIDocShell* docShell = GetDocShell();
> + nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(docShell);
> + if (!loadContext) {
> + return NS_OK;
Are we sure we want to exit here this way? Pretend we are not in PB ? (sure, may be unsafe)
::: dom/indexedDB/CheckPermissionsHelper.cpp
@@ +95,3 @@
> // TODO Support private browsing indexedDB?
> return nsIPermissionManager::DENY_ACTION;
> }
So, here when there is no load context, just pass through? Contrast against what you do for localStorage.
::: dom/src/storage/StorageChild.cpp
@@ +276,5 @@
> +NS_IMETHODIMP
> +StorageChild::PrivateModeChanged(bool enabled)
> +{
> + mInPrivateBrowsing = enabled;
> + SendUpdatePrivateState(enabled);
Pity there isn't a central point for this. There may be a lot of instances active and therefor a lot of messages. Do you plan something like that? (Not needed for this bug, though)
::: dom/src/storage/nsDOMStorage.cpp
@@ +820,5 @@
> // If this is a cross-process situation, we don't have a real storage owner.
> // All the correct checks have been done on the child, so we just need to
> // make sure that our session-only status is correctly updated.
> if (!mOwner)
> + return nsDOMStorage::CanUseStorage(this, &mSessionOnly);
Hmm.. maybe just change the method to pass only |this| ?
@@ +1313,5 @@
>
> +NS_IMETHODIMP
> +DOMStorageImpl::PrivateModeChanged(bool enabled)
> +{
> + mInPrivateBrowsing = enabled;
Update also mSessionOnly. Maybe just run CanUseStorage on |this|.
Attachment #616892 -
Flags: review?(honzab.moz) → review+
Comment 18•13 years ago
|
||
Comment on attachment 617273 [details] [diff] [review]
Basic sanity test for before/after private browsing DOM storage implementation.
Review of attachment 617273 [details] [diff] [review]:
-----------------------------------------------------------------
Hard to track.
::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_concurrent.js
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Test opening two tabs that share a localStorage, but keep one in private mode.
> +// Ensure that values from one don't leak into the other, and that values from
> +// earlier private storage sessions aren't visible later.
Can you please list the steps one by one? Like:
- in non private, set value=test
- in private check length=0 and value is null
- etc..
@@ +13,5 @@
> + let browser = gBrowser.selectedBrowser;
> + browser.addEventListener('load', function() {
> + browser.removeEventListener('load', arguments.callee, true);
> + gBrowser.selectedTab = gBrowser.addTab();
> + let browser2 = gBrowser.selectedBrowser;
name there more descriptively like private_tab, non_private_tab.
::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_localStorage_before_after.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
Please add a comment what steps this test does, what external files it uses, and what is the overall purpose.
Comment 19•13 years ago
|
||
Comment on attachment 616892 [details] [diff] [review]
Determine private browsing status dynamically from most recent docshell accessible WIP
Review of attachment 616892 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/interfaces/storage/nsIDOMStorageManager.idl
@@ +65,5 @@
> * for a single origin.
> */
> nsIDOMStorage getLocalStorageForPrincipal(in nsIPrincipal aPrincipal,
> + in DOMString aDocumentURI,
> + in bool aPrivate);
Change IID!
Assignee | ||
Comment 20•13 years ago
|
||
- Determine private browsing status dynamically from most recent docshell accessible WIP
Assignee | ||
Comment 21•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #617273 -
Attachment is obsolete: true
Attachment #617273 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 22•13 years ago
|
||
Comment on attachment 619893 [details] [diff] [review]
Basic sanity test for before/after private browsing DOM storage implementation.
Now with more descriptive tests.
Attachment #619893 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 23•13 years ago
|
||
Comment on attachment 619891 [details] [diff] [review]
* * *
This should be a pretty quick review; here's what I changed:
* addressed your comments
* made some tests pass by adding GetInterface nsDOMStorage[2] and switching to do_GetInterface in places that obtain an observer
* updated all call sites of getLocalStorageForPrincipal, thus making the remaining tests pass
Attachment #619891 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 24•13 years ago
|
||
The change to getLocalStorageForPrincipal might affect addons. We should check what kind of use exists in mxr.mozilla.org/addons.
Keywords: addon-compat,
dev-doc-needed
Comment 25•13 years ago
|
||
Try run for 7beb7938da34 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=7beb7938da34
Results (out of 131 total builds):
success: 93
warnings: 38
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-7beb7938da34
Comment 26•13 years ago
|
||
(In reply to Josh Matthews [:jdm] (travelling until June 25th, not reading non-CCed bugmail) from comment #24)
> The change to getLocalStorageForPrincipal might affect addons. We should
> check what kind of use exists in mxr.mozilla.org/addons.
I think we could make the second and the (new) third argument both optional.
Comment 27•13 years ago
|
||
Comment on attachment 619891 [details] [diff] [review]
* * *
Review of attachment 619891 [details] [diff] [review]:
-----------------------------------------------------------------
Few comments left, all mostly nits.
r=honzab with that or arguments against.
::: dom/base/nsGlobalWindow.cpp
@@ +2546,5 @@
> +
> + if (mLocalStorage) {
> + nsCOMPtr<nsIPrivacyTransitionObserver> obs = do_GetInterface(mLocalStorage);
> + if (obs) {
> + mDocShell->AddWeakPrivacyTransitionObserver(obs);
Good catch, I didn't realize this the last time.
::: dom/indexedDB/CheckPermissionsHelper.cpp
@@ +92,5 @@
> }
>
> + nsCOMPtr<nsIWebNavigation> webNav = do_GetInterface(aWindow);
> + nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(webNav);
> + if (loadContext && loadContext->UsePrivateBrowsing()) {
Do we have a test for this?
::: dom/interfaces/storage/nsIDOMStorageManager.idl
@@ +65,5 @@
> * for a single origin.
> */
> nsIDOMStorage getLocalStorageForPrincipal(in nsIPrincipal aPrincipal,
> + in DOMString aDocumentURI,
> + in bool aPrivate);
Make both aPrivate and aDocumentURI optional. JS compatibility is then preserved and you don't need to update existing tests as well.
::: dom/src/storage/nsDOMStorage.cpp
@@ +1441,3 @@
> //static
> bool
> +nsDOMStorage::CanUseStorage(DOMStorageBase* aStorage, bool* aSessionOnly)
Any reason why not to just pass aStorage? (And default it to NULL)
You can then |if (aStorage) aStorage->mSessionOnly = ...|, simply just make the member friendly if it's not public already.
@@ +1802,5 @@
> +NS_IMETHODIMP
> +nsDOMStorage2::GetInterface(const nsIID & aIID, void **result)
> +{
> + nsCOMPtr<nsIInterfaceRequestor> req =
> + do_QueryInterface(NS_ISUPPORTS_CAST(nsIDOMStorageObsolete*, mStorage));
mStorage is nsDOMStorage, just do |mStorage->GetInterface(aIID, result);| directly?
::: dom/src/storage/nsDOMStorage.h
@@ +162,5 @@
> nsAString& aOldValue) = 0;
> virtual nsresult Clear(bool aCallerSecure, PRInt32* aOldCount) = 0;
> +
> + // Call the above with |this| and |&this->mSessionOnly|
> + bool CanUseStorage();
Not sure what "the above" is. Otherwise, I like this method.
Attachment #619891 -
Flags: review?(honzab.moz) → review+
Comment 28•13 years ago
|
||
Comment on attachment 619893 [details] [diff] [review]
Basic sanity test for before/after private browsing DOM storage implementation.
Review of attachment 619893 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_concurrent.js
@@ +8,5 @@
> +
> +// Step 1: create new tab, load a page that sets test=value in non-private storage
> +// Step 2: create a new tab, load a page that sets test2=value2 in private storage
> +// Step 3: load a page in the tab from step 1 that checks the value of test2 and the total count in non-private storage
> +// Step 4: load a page in the tab from step 2 that checks the value of test and the total count in private storage
Step 3(,4): ... test2 *is value2* and the total count in non-private storage *is 1*
::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_concurrent_page.html
@@ +10,5 @@
> + aItKey = aCouples[nKeyId].split("=");
> + oGetVars[unescape(aItKey[0])] = aItKey.length > 1 ? unescape(aItKey[1]) : "";
> + }
> + }
> + localStorage.length; // ensure all keys are cached
Is this necessary to make test pass? If so, then it is a bug!
::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_localStorage_before_after.js
@@ +29,5 @@
> + let browser2 = gBrowser.selectedBrowser;
> + gBrowser.removeTab(tab);
> + browser2.addEventListener('load', function() {
> + browser2.removeEventListener('load', arguments.callee, true);
> + is(browser2.contentWindow.document.title, '1', 'localStorage should contain 1 item');
Hmm... so you set 'test' = 'value' in a PB tab and then you set again 'test' = 'value' in a non PB page, so if the value were already there you would just overwrite it. The second load should better only check the value is not there (with getItem), right? Or what is the purpose of this test?
In all your tests I don't initial and final localStorage cleanup.
Attachment #619893 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•13 years ago
|
Attachment #616892 -
Attachment is obsolete: true
Assignee | ||
Comment 29•13 years ago
|
||
>::: dom/indexedDB/CheckPermissionsHelper.cpp
>@@ +92,5 @@
>> }
>>
>> + nsCOMPtr<nsIWebNavigation> webNav = do_GetInterface(aWindow);
>> + nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(webNav);
>> + if (loadContext && loadContext->UsePrivateBrowsing()) {
>
>Do we have a test for this?
Yes. It's in the test patch with the existing indexeddb permission tests.
>In all your tests I don't initial and final localStorage cleanup.
I'm not sure what this means.
Comment 30•13 years ago
|
||
(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #29)
> >In all your tests I don't initial and final localStorage cleanup.
>
> I'm not sure what this means.
Sorry, has to be: In all your tests I don't *see* initial and final localStorage cleanup.
Assignee | ||
Comment 31•13 years ago
|
||
What sort of init/cleanup are you looking for? I didn't see any happening in the localStorage mochitests.
Comment 32•13 years ago
|
||
Comment on attachment 619891 [details] [diff] [review]
* * *
Review of attachment 619891 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/src/storage/nsDOMStorage.cpp
@@ +613,5 @@
> +{
> + nsresult rv = mStorageImpl->QueryInterface(aIID, result);
> + if (NS_SUCCEEDED(rv))
> + return rv;
> + return QueryInterface(aIID, result);;
Double ;;
Assignee | ||
Comment 33•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #619891 -
Attachment is obsolete: true
Assignee | ||
Comment 34•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #619893 -
Attachment is obsolete: true
Assignee | ||
Comment 35•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/5a903d496ab0
http://hg.mozilla.org/integration/mozilla-inbound/rev/70cde80fa095
Target Milestone: --- → mozilla15
Comment 36•13 years ago
|
||
Sorry had to back out for crashes:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=70cde80fa095
eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12062124&tree=Mozilla-Inbound
{
PROCESS-CRASH | file:///Users/cltbld/talos-slave/test/build/reftest/tests/content/base/crashtests/646184.html | application crashed (minidump found)
Crash dump filename: /var/folders/Hs/HsDn6a9SG8idoIya6p9mtE+++TI/-Tmp-/tmp3V6FL3/minidumps/5A7518E5-E1D7-4372-81A6-743E4C2A3723.dmp
Operating system: Mac OS X
10.6.8 10K549
CPU: amd64
family 6 model 23 stepping 10
2 CPUs
Crash reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0
Thread 0 (crashed)
0 XUL!nsGlobalWindow::GetLocalStorage [nsGlobalWindow.cpp : 8162 + 0x7]
rbx = 0x00000000 r12 = 0x41f858a0 r13 = 0x00188ef0 r14 = 0x5fbfa280
r15 = 0x41f85a68 rip = 0x02500407 rsp = 0x5fbfa0c0 rbp = 0x5fbfa1c0
Found by: given as instruction pointer in context
1 XUL!nsIDOMWindow_GetLocalStorage [dom_quickstubs.cpp : 503 + 0x4]
rbx = 0x00000000 r12 = 0x09112290 r13 = 0x5fbfa4d0 r14 = 0x5fbfb390
r15 = 0x5fbfa4c0 rip = 0x02c54a10 rsp = 0x5fbfa1d0 rbp = 0x5fbfa400
Found by: call frame info
2 XUL!js::CallJSPropertyOp [jscntxtinlines.h : 444 + 0xf]
rbx = 0x00000000 r12 = 0x09112290 r13 = 0x5fbfb390 r14 = 0x5fbfa420
r15 = 0x5fbfa4c0 rip = 0x03b914d0 rsp = 0x5fbfa410 rbp = 0x5fbfa460
Found by: call frame info
3 XUL!js_NativeGetInline [jsscopeinlines.h : 281 + 0x11]
rbx = 0x5fbfa4c0 r12 = 0x25a60060 r13 = 0x5fbfa4d0 r14 = 0x25a5b070
r15 = 0x09112290 rip = 0x03b6c790 rsp = 0x5fbfa470 rbp = 0x5fbfa510
Found by: call frame info
4 XUL!js_GetPropertyHelperInline [jsobj.cpp : 5087 + 0x11]
rbx = 0x5fbfa600 r12 = 0x09112290 r13 = 0x5fbfb390 r14 = 0x25a60060
r15 = 0x00000000 rip = 0x03b7098d rsp = 0x5fbfa520 rbp = 0x5fbfa5d0
Found by: call frame info
5 XUL!js::Wrapper::get [jsobjinlines.h : 162 + 0x12]
rbx = 0x25a5a060 r12 = 0x5fbfb390 r13 = 0x04b1eb30 r14 = 0x25a5f080
r15 = 0x09112290 rip = 0x03c74ddb rsp = 0x5fbfa5e0 rbp = 0x5fbfa660
Found by: call frame info
6 XUL!js::Proxy::get [jsproxy.cpp : 1090 + 0x20]
rbx = 0x09112290 r12 = 0x25a5f080 r13 = 0x5fbfb390 r14 = 0x5fbfa680
r15 = 0x080c7700 rip = 0x03bc877b rsp = 0x5fbfa670 rbp = 0x5fbfa6d0
Found by: call frame info
7 XUL!proxy_GetGeneric [jsproxy.cpp : 1303 + 0xd]
rbx = 0x25a5f080 r12 = 0x5fbfb390 r13 = 0x04b22d30 r14 = 0x25a4d2c0
r15 = 0x09112290 rip = 0x03bc87b2 rsp = 0x5fbfa6e0 rbp = 0x5fbfa6e0
Found by: call frame info
8 XUL!js::Wrapper::get [jsobjinlines.h : 159 + 0xf]
rbx = 0x25a5f080 r12 = 0x5fbfb390 r13 = 0x04b22d30 r14 = 0x25a4d2c0
r15 = 0x09112290 rip = 0x03c74d9c rsp = 0x5fbfa6f0 rbp = 0x5fbfa770
Found by: call frame info
9 XUL!js::CrossCompartmentWrapper::get [jswrapper.cpp : 525 + 0x38]
rbx = 0x09112290 r12 = 0x25a4d2c0 r13 = 0x5fbfa790 r14 = 0x5fbfb390
r15 = 0x04b22d30 rip = 0x03c77659 rsp = 0x5fbfa780 rbp = 0x5fbfa830
}
https://hg.mozilla.org/integration/mozilla-inbound/rev/f023c835794e
Target Milestone: mozilla15 → ---
Comment 37•13 years ago
|
||
Presume this is also related:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12063725&tree=Mozilla-Inbound
Assignee | ||
Comment 38•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/b1e2749bd72b
http://hg.mozilla.org/integration/mozilla-inbound/rev/0729990b395c
Try run was all green, so hopefully this will stick.
Whiteboard: [needs review]
Target Milestone: --- → mozilla15
Comment 39•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b1e2749bd72b
https://hg.mozilla.org/mozilla-central/rev/0729990b395c
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 40•12 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #24)
> The change to getLocalStorageForPrincipal might affect addons. We should
> check what kind of use exists in mxr.mozilla.org/addons.
I'm getting about 40 matches in the MXR.
I see the additional parameter is optional. This mean that current users of this function won't break, right?
Assignee | ||
Comment 41•12 years ago
|
||
Correct.
You need to log in
before you can comment on or make changes to this bug.
Description
•