722857 - DOMStorage should obtain Private Browsing information from related docshell

Status: RESOLVED FIXED

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

Description

[Josh Matthews [:jdm]]

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 1

[Josh Matthews [:jdm]]

Attachment: Basic sanity test for before/after private browsing DOM storage implementation.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Determine private browsing status dynamically from most recent docshell accessible WIP

Comment 3

[Josh Matthews [:jdm]]

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 4

[Honza Bambas (:mayhemer)]

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.

Comment 5

[Josh Matthews [:jdm]]

Attachment: Determine private browsing status dynamically from most recent docshell accessible WIP

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

Comment 6

[Josh Matthews [:jdm]]

Attachment: Determine private browsing status dynamically from most recent docshell accessible WIP

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.

Comment 7

[Josh Matthews [:jdm]]

Attachment: Determine private browsing status dynamically from most recent docshell accessible WIP

Comment 8

[Josh Matthews [:jdm]]

Attachment: Basic sanity test for before/after private browsing DOM storage implementation.

Comment 9

[Josh Matthews [:jdm]]

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

Comment 10

[Josh Matthews [:jdm]]

Attachment: Basic sanity test for before/after private browsing DOM storage implementation.

Comment 11

[Josh Matthews [:jdm]]

Attachment: Basic sanity test for before/after private browsing DOM storage implementation.

Comment 12

[Josh Matthews [:jdm]]

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 13

[Josh Matthews [:jdm]]

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.

Comment 14

[Josh Matthews [:jdm]]

Attachment: Basic sanity test for before/after private browsing DOM storage implementation.

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

Comment 15

[Josh Matthews [:jdm]]

Attachment: Basic sanity test for before/after private browsing DOM storage implementation.

Forgot a file.

Comment 16

[Honza Bambas (:mayhemer)]

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

Comment 17

[Honza Bambas (:mayhemer)]

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 18

[Honza Bambas (:mayhemer)]

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

[Honza Bambas (:mayhemer)]

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!

Comment 20

[Josh Matthews [:jdm]]

Attachment: * * *

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

Comment 21

[Josh Matthews [:jdm]]

Attachment: Basic sanity test for before/after private browsing DOM storage implementation.

Comment 22

[Josh Matthews [:jdm]]

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

Now with more descriptive tests.

Comment 23

[Josh Matthews [:jdm]]

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

Comment 24

[Josh Matthews [:jdm]]

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

Comment 25

[Mozilla RelEng Bot]

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/[e-mail]-7beb7938da34

Comment 26

[Honza Bambas (:mayhemer)]

(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

[Honza Bambas (:mayhemer)]

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 28

[Honza Bambas (:mayhemer)]

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.

Comment 29

[Josh Matthews [:jdm]]

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

[Honza Bambas (:mayhemer)]

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

Comment 31

[Josh Matthews [:jdm]]

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

Comment 32

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 33

[Josh Matthews [:jdm]]

Attachment: Determine private browsing status for DOMStorage from owning docshell if available, and receive updates if its privacy status changes.

Comment 34

[Josh Matthews [:jdm]]

Attachment: Basic sanity test for before/after private browsing DOM storage implementation.

Comment 35

[Josh Matthews [:jdm]]

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

Comment 36

[Ed Morley [:emorley]]

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

Comment 37

[Ed Morley [:emorley]]

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

Comment 38

[Josh Matthews [:jdm]]

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.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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

Comment 40

[Jorge Villalobos [:jorgev] (he/him)]

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

Comment 41

[Josh Matthews [:jdm]]

Correct.