Last Comment Bug 715700 - localStorage gets out of sync in Private Mode
: localStorage gets out of sync in Private Mode
Status: RESOLVED FIXED
[qa+]
: regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 9 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Josh Matthews [:jdm]
:
Mentors:
Depends on:
Blocks: 683316
  Show dependency treegraph
 
Reported: 2012-01-05 15:37 PST by Colin Blake
Modified: 2012-04-06 01:32 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
+
verified


Attachments
Test page. (3.42 KB, text/html)
2012-01-05 15:37 PST, Colin Blake
no flags Details
Cache all keys when setting a value in memory DBs (984 bytes, patch)
2012-01-11 17:09 PST, Josh Matthews [:jdm]
honzab.moz: review+
Details | Diff | Splinter Review
Test to check that localStorage across multiple windows remains in sync while in private browsing mode. (6.23 KB, patch)
2012-01-11 17:10 PST, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Test to check that localStorage across multiple windows remains in sync while in private browsing mode. (5.76 KB, patch)
2012-01-11 17:11 PST, Josh Matthews [:jdm]
honzab.moz: review+
Details | Diff | Splinter Review

Description Colin Blake 2012-01-05 15:37:58 PST
Created attachment 586252 [details]
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 2012-01-05 15:51:44 PST
This problem does NOT occur on Firefox 8, but does occur with Firefox 9.
Comment 2 Josh Matthews [:jdm] 2012-01-05 16:20:53 PST
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 2012-01-05 16:55:27 PST
What a GREAT tool!!

Last good nightly: 2011-09-23
First bad nightly: 2011-09-24
Comment 4 Josh Matthews [:jdm] 2012-01-05 17:27:27 PST
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 2012-01-05 17:36:03 PST
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] 2012-01-05 17:54:49 PST
In a couple hours, there will be a testing build available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-d879e0e35fc1 . Give it a shot and see if this bug disappears.
Comment 7 Colin Blake 2012-01-06 07:15:26 PST
No more bug. That checkin was the culprit.
Comment 8 Josh Matthews [:jdm] 2012-01-06 09:10:40 PST
Thanks for confirming that, Colin!
Comment 9 Colin Blake 2012-01-06 09:42:01 PST
Any idea when this will get fixed? localStorage not working in Private Mode is quite a regression.
Comment 10 Alex Keybl [:akeybl] 2012-01-09 12:45:42 PST
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] 2012-01-10 10:29:58 PST
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] 2012-01-10 10:41:17 PST
Sorry, I forgot to make clear that Honza asked me to look into this, and I am doing so.
Comment 13 Josh Matthews [:jdm] 2012-01-10 23:28:46 PST
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] 2012-01-10 23:53:53 PST
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] 2012-01-11 17:09:39 PST
Created attachment 587885 [details] [diff] [review]
Cache all keys when setting a value in memory DBs
Comment 16 Josh Matthews [:jdm] 2012-01-11 17:10:23 PST
Created attachment 587886 [details] [diff] [review]
Test to check that localStorage across multiple windows remains in sync while in private browsing mode.
Comment 17 Josh Matthews [:jdm] 2012-01-11 17:11:42 PST
Created attachment 587887 [details] [diff] [review]
Test to check that localStorage across multiple windows remains in sync while in private browsing mode.
Comment 18 Honza Bambas (:mayhemer) 2012-01-19 13:11:43 PST
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 21 Alex Keybl [:akeybl] 2012-02-13 12:49:28 PST
This hasn't caused significant user pain since FF10 was released - we'll let this ride the trains.
Comment 22 Virgil Dicu [:virgil] [QA] 2012-04-06 01:32:58 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.