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.

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?

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.

Just checkpointing my work. I still need to figure out what to do in some of the nsDOMStorageWrapperDB methods.

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.

I still need to test concurrent access; these are just rebased and multiprocess-friendly.

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.

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.

Now with tests that pass as part of the overall test suite and not just when run standalone.

Forgot a file.

Josh, I started the review. Check bug 536509, it may collide (by means of need to merge) with your patches.

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|.

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 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!

- Determine private browsing status dynamically from most recent docshell accessible WIP

Comment on attachment 619893 [details] [diff] [review] Basic sanity test for before/after private browsing DOM storage implementation. Now with more descriptive tests.

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

The change to getLocalStorageForPrincipal might affect addons. We should check what kind of use exists in mxr.mozilla.org/addons.

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

(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 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.

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.

>::: 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.

(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.

What sort of init/cleanup are you looking for? I didn't see any happening in the localStorage mochitests.

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 ;;

http://hg.mozilla.org/integration/mozilla-inbound/rev/5a903d496ab0 http://hg.mozilla.org/integration/mozilla-inbound/rev/70cde80fa095

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

Presume this is also related: https://tbpl.mozilla.org/php/getParsedLog.php?id=12063725&tree=Mozilla-Inbound

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.

https://hg.mozilla.org/mozilla-central/rev/b1e2749bd72b https://hg.mozilla.org/mozilla-central/rev/0729990b395c

(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?

Correct.