1410701 - Make localStorage work across content processes in private browsing

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, defect, P2)

Description

[tyler.waters]

Attachment: local-storage-test.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171019140425

Steps to reproduce:

(1) visit a site that saves a key in localStorage

(2) open this page in an inPrivate window

(3) ctrl-click a link that visits the same page and attempts to read from same localStorage key


Actual results:

localStorage is lost in the new tab.


Expected results:

localStorage is kept.

Comment 1

[tyler.waters]

The attachment is a very simple page that sets and reads local storage.  Open it in an inprivate window, click 'set storage' then open the provided link in a new tab -- press `get storage` and you won't get anything back.  This doesn't happen under 56.

Comment 2

[Josh Matthews [:jdm]]

When I follow these steps, I cannot reproduce the problem in FF nightly on macOS:
* open a new private browsing window
* load https://bug1410701.bmoattachments.org/attachment.cgi?id=8920859
* click "set storage"
* open the "Open me in a new tab" link in a new tab in the private window
* click "get storage"
Can you reproduce this in nightly Firefox?

Comment 3

[tyler.waters]

I can confirm it is working in nightly version 58.0a1 under windows 10.

Comment 4

[tyler.waters]

(In reply to tyler.waters from comment #3)
> I can confirm it is working in nightly version 58.0a1 under windows 10.

OOPS!

Turns out it only appeared to work.  I went back and checked beta and it worked there as well.  Then I went to the site I found this under and it's still a problem there.  Turns out the problem *doesn't* show when a file is served from file:/// 

I can confirm this problem does show up under nightly.  I'll attach a simple node js that spins up a server on 3000 to highlight the problem shortly.

Comment 5

[tyler.waters]

Attachment: local-storage-test.js

start the web server by running `node local-storage-test.js`
visit localhost:3000 in an inprivate browsing window
press "save local storage" then "get local storage"
open the provided link in a new tab (ctrl+click)
press "get local storage" -- nothing is returned.

Comment 6

[Josh Matthews [:jdm]]

Huh. I can reproduce when serving the localhost. I don't understand why https://bug1410701.bmoattachments.org/attachment.cgi?id=8920859 does not reproduce the problem.

Comment 7

[Honza Bambas (:mayhemer)]

based on comment 6.

Comment 8

[Honza Bambas (:mayhemer)]

This could be a duplicate of bug 666724.  Depends on how we allocate content processes for different pages at the same origin.  Note that in PB windows LS content is in-mem only and only on the respective content process.  This was a design in times we had only one content process.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

So this is not a recent regression?

Comment 10

[Jan Varga [:janv]]

It seems it's not a regression. New tabs live in separate content processes in this case and current local storage implementation for private browsing has per process cache which doesn't seem to be synced across processes. This was fixed for the non-private browsing case.
I'm going to debug it a bit ...

Comment 11

[Jan Varga [:janv]]

Well, even when we fix synchronization, there will be still a bug. If a new private browsing tab is created in a new process, there's currently no way to load data stored in previous private browsing tab.

Ideally, the cache lives in the parent process even for private browsing. This bug actually helped me a bit, because I wasn't sure if new local storage implementation should use a cache in the parent for private browsing.

For now, we could limit number of content processes for private browsing to 1, if it doesn't cause other problems.

Comment 12

[Josh Matthews [:jdm]]

(In reply to Jan Varga [:janv] from comment #11)
> For now, we could limit number of content processes for private browsing to
> 1, if it doesn't cause other problems.

For anybody running with "Never remember history" or "Always use private browsing mode" enabled, that would mean they are limited to a single process.

Comment 13

[Jan Varga [:janv]]

Well, I could somehow extract only the "cache in the parent" from new local storage implementation, but that can't land on 57 branch, it's too late for such big change.

Comment 14

[Jan Varga [:janv]]

A "quick" hack would be to let content process "persist" private browsing data in an in-memory sqlite database in the parent.

Comment 15

[Jan Varga [:janv]]

Andrew, can you assign a priority to this bug ? Thanks.

Comment 16

[Andrew Overholt [:overholt]]

This isn't great. However, I'm not sure we've ever guaranteed one could browse their whole life in private browsing and everything would work exactly the same as in non-PB (have we and I missed it? IDB still isn't working in PB, for example).

Jan, if there's a simple fix here, let's go for it and if not, let's be sure to incorporate this use case in the work in bug 1286798.

Comment 17

[Jan Varga [:janv]]

Attachment: Part 1: Remove separate dataset for private browsing

I believe the separate dataset for private browsing is useless.

Comment 18

[Jan Varga [:janv]]

Here's the plan:
Part 1 - remove separate set for private browsing - already implemented
Part 2 - separate database initialization/shutdown into own class - partly implemented
Part 3 - implement a memory database (sqlite) and allow persisting of private browsing data using the memory only database

Since this is P3 I have to work on other stuff for now.

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jan Varga [:janv] from comment #17)
> Created attachment 8923707 [details] [diff] [review]
> Part 1: Remove separate dataset for private browsing
> 
> I believe the separate dataset for private browsing is useless.

I believe the dataset still exists for the case where we have a chrome docshell, because of this invariant:
https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/docshell/base/nsDocShell.cpp#3841
...because we don't set the mPrivateBrowsingId because the System Principal is a singleton.  (I think).

In that case, nsDocShell::GetUsePrivateBrowsing() will return true for the system principal which doesn't have the origin attribute set.  (https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2313)  Aside: I think IndexedDB's MOZ_DIAGNOSTIC_ASSERT at https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/dom/indexedDB/IDBFactory.cpp#625 may run afoul of this some day.

Of course, there are ways to remove the set and either just forbid this case or compensate for it.

Comment 20

[Jan Varga [:janv]]

Thanks, good to know, but stuff in storage could be simpler if we fix the system principal case somehow.

Comment 21

[trevan]

I believe I hit this bug with an application I'm building.  There's a link on the page to take you to an oauth2 workflow on a separate domain and once you've logged in, you are redirected back to the page.  The first page sets a value in localStorage and then it uses that value after the redirect.

In private browsing, Firefox can't read the value after the redirect.  Outside of private browsing, it works fine.

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

I believe this is fixed by LSNG being enabled on release in bug 1599979. Storage is currently in-memory.