715700 - localStorage gets out of sync in Private Mode

Status: RESOLVED FIXED

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

Description

[Colin Blake]

Attachment: Test page.

This may be the same bug as 662511, however, that bug has already been closed and this one has a simpler test case.

When using localStorage in private browsing, if you open the same page in a second window, or open a different page in the same window, and then write to localStorage, anything that was already in localStorage is lost. This problem can be avoided by reading from localStorage (or just referencing localStorage.length) first.

Save the attachment as test1.html and test1a.html and then:

1. Go into private browsing mode.

2. Load test1.html.

3. Enter "test1" and "value1" and hit SAVE.

4. Click on "DUMP" to verify that the data was saved to localStorage.

5. Load test1a.html.

6. Enter "test2" and "value2" and hit SAVE.

7. Click on "DUMP" and you'll ONLY see "test2". Test1 has gone (although it can still be accessed via localStorage.getItem("test1")).

If you repeat the test but BEFORE step 6 click on the GET LENGTH button, then after saving the test2 data when you click on DUMP you'll see both values.

Comment 1

[Colin Blake]

This problem does NOT occur on Firefox 8, but does occur with Firefox 9.

Comment 2

[Josh Matthews [:jdm]]

Thanks! Since you're able to reproduce this problem, could you help us find a regression range? http://mozilla.github.com/mozregression/ is a tool that will help you narrow the problem down to an isolated range. Just give it 2011-09-27 for the good date, and 2011-11-08 for the bad one (those should correspond to the dates for FF8 and FF9), and we should be able to find a specific nightly build in which this bug appeared.

Comment 3

[Colin Blake]

What a GREAT tool!!

Last good nightly: 2011-09-23
First bad nightly: 2011-09-24

Comment 4

[Josh Matthews [:jdm]]

From the range of http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2011-09-23&enddate=2011-09-24, bug 683316 looks like the most likely candidate. I'll put together a build without that change so we can test that theory.

Comment 5

[Colin Blake]

Yes, that's the only thing in the changelog http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=259d1556c221&tochange=c71229984353 which mentions "storage".

Comment 6

[Josh Matthews [:jdm]]

In a couple hours, there will be a testing build available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d879e0e35fc1 . Give it a shot and see if this bug disappears.

Comment 7

[Colin Blake]

No more bug. That checkin was the culprit.

Comment 8

[Josh Matthews [:jdm]]

Thanks for confirming that, Colin!

Comment 9

[Colin Blake]

Any idea when this will get fixed? localStorage not working in Private Mode is quite a regression.

Comment 10

[Alex Keybl [:akeybl]]

Tracking for FF10 and higher since this is a recent regression. We'll still evaluate the risk vs reward once fixed, as the user scenario that this occurs in (Private Mode + LocalStorage) sounds fairly uncommon.

Comment 11

[Alex Keybl [:akeybl]]

Sending over to jst to see if somebody can take a look at this. To be clear on the tracking flags, I see no reason to block FF10/11 release due to this bug (fairly low user pain). Since it is a recent regression, however, I'd like to make sure we investigate further.

Comment 12

[Josh Matthews [:jdm]]

Sorry, I forgot to make clear that Honza asked me to look into this, and I am doing so.

Comment 13

[Josh Matthews [:jdm]]

The same problem is exhibited outside of private browsing if sessionStorage is used instead of localStorage. nsDOMStoragePersistentDB explicitly caches all keys inside SetKey, while nsDOMStorageMemoryDB does not. However, I'm having trouble writing a mochitest that demonstrates this - using an iframe caused the bug to be hidden (maybe something about the store being cloned?), and a popup window turned out to exhibit different behaviour than I was expecting, and I haven't figured out whether it's correct yet or not.

Comment 14

[Josh Matthews [:jdm]]

Ugh, my mistake for not understanding how sessionStorage works. I expect most or all of comment 13 can be ignored.

Comment 15

[Josh Matthews [:jdm]]

Attachment: Cache all keys when setting a value in memory DBs

Comment 16

[Josh Matthews [:jdm]]

Attachment: Test to check that localStorage across multiple windows remains in sync while in private browsing mode.

Comment 17

[Josh Matthews [:jdm]]

Attachment: Test to check that localStorage across multiple windows remains in sync while in private browsing mode.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 587885 [details] [diff] [review]
Cache all keys when setting a value in memory DBs

Review of attachment 587885 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, that's it.  Thanks.

I think the yet better solution here is to move call of CacheKeysFromDB() from nsDOMStoragePersistentDB::SetKey to DOMStorageImpl::SetDBValue (before call of gStorageDB->SetKey).

For the record: the reason we are our out of sync is because of the following call stack:

>	xul.dll!DOMStorageImpl::SetCachedVersion(unsigned __int64 version=5)  Line 275	C++
 	xul.dll!nsDOMStorageBaseDB::MarkScopeDirty(DOMStorageImpl * aStorage=0x0b0bf7c0)  Line 95	C++
 	xul.dll!nsDOMStorageMemoryDB::SetKey(DOMStorageImpl * aStorage=0x0b0bf7c0, const nsAString_internal & aKey={...}, const nsAString_internal & aValue={...}, bool aSecure=false, int aQuota=5242880, bool aExcludeOfflineFromUsage=true, int * aNewUsage=0x0049bad4)  Line 240	C++
 	xul.dll!nsDOMStorageDBWrapper::SetKey(DOMStorageImpl * aStorage=0x0b0bf7c0, const nsAString_internal & aKey={...}, const nsAString_internal & aValue={...}, bool aSecure=false, int aQuota=5242880, bool aExcludeOfflineFromUsage=true, int * aNewUsage=0x0049bad4)  Line 166 + 0x78 bytes	C++
>	xul.dll!DOMStorageImpl::SetDBValue(const nsAString_internal & aKey={...}, const nsAString_internal & aValue={...}, bool aSecure=false)  Line 926 + 0x4b bytes	C++
 	xul.dll!DOMStorageImpl::SetValue(bool aIsCallerSecure=false, const nsAString_internal & aKey={...}, const nsAString_internal & aData={...}, nsAString_internal & aOldValue={...})  Line 1236 + 0x1a bytes	C++
 	xul.dll!nsDOMStorage::SetItem(const nsAString_internal & aKey={...}, const nsAString_internal & aData={...})  Line 1659 + 0x30 bytes	C++


This sets the version of data held by the instance wrongly to the latest (up-tp-date) version because of trust that the instance's cache is up to date.  But it is not, if we call SetItem on a brand new localStorage instance, because of missing call to CacheKeysFromDB().

How I'd love to remove this code completely!

Comment 19

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/740578328b88
http://hg.mozilla.org/integration/mozilla-inbound/rev/4053b191975d

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/740578328b88
https://hg.mozilla.org/mozilla-central/rev/4053b191975d

Comment 21

[Alex Keybl [:akeybl]]

This hasn't caused significant user pain since FF10 was released - we'll let this ride the trains.

Comment 22

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0

Verified on Windows 7 in Firefox 12 beta 4 using steps in comment 0. both values (test 1 and 2) are remembered now when selecting Dump in Private browsing mode.