624047 - LocalStorage value is lost after a few seconds

Status: VERIFIED FIXED

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

Description

[Daniel]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.0b9pre) Gecko/20110106 Firefox/4.0b9pre
Build Identifier: 4.0b9pre (2011-01-06)

On the page which I linked to there is an example of the problem I have encountered. The value of a localStorage property is set to 1 and then once every second I check to see if it is equal to null. After a few seconds the value seems to be reset. For some reason this only happens after one reload of the page and only on every other reload after that. On the very first page load the data is persistent. 

I've tried on both windows 7(64) and vista (x86) with the same results. It doesn't seem to happen in firefox 3.6. (i've tried in beta 8 & beta 9 pre)

Reproducible: Always

Steps to Reproduce:
1. Load http://jsbin.com/onegu4/2/
2. Open the console
3. reload the page once
4. wait and look at the results of "localStorage.name === null". Most often it will return "true" after 6-8 s but sometimes it takes longer (no more than a minute)
5. Reload the page twice and it will do the same thing
Actual Results:  
The localStorage property which is set to "1" returns "null" after about 10 s

Expected Results:  
The data should be persistent and always return "1" (if not altered)

Comment 1

[Boris Zbarsky [:bzbarsky]]

I can reproduce using that testcase.  Jonas, Honza, what's going on here?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Attempted testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Smaller testcase

Comment 4

[Honza Bambas (:mayhemer)]

This might be dup of bug 607418 which I wasn't able to figure out.  Thanks for the test case!  Going to take a look at this.

Comment 5

[Honza Bambas (:mayhemer)]

The cause:
- we create a storage for the first page load (first instance of the global window)
- on the first usage of this storage the temp table is loaded (flag DOMStorageImpl::mLoadedTemporaryTable of that storage is set)
- timer is started to flush the temp table after 5 seconds
- in the meantime we reload the page
- new storage and global window pair is created
- on the first usage of the second storage the temp table is loaded again which is also a bug (then flag mLoadedTemporaryTable of the second storage is set)
- timer fires, we flush and delete the temp table, mLoadedTemporaryTable flag is dropped on the first storage but remains on the second one
- on the next usage of the second storage we read from the temp table that is empty
- the temp table is not reloaded from the disk table because the second storage's mLoadedTemporaryTable is still true (it believes the temp table is up-to-date)

I want to move the flag out of the storage to a hash table and move the code that flushes the table from the storage to the storage manager.  I think I can have a patch today, including a test.

I think this can fix bug 607418 as well.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1

Again, thanks for reporting this bug and making a good test case.

Changes:
- when a temp table is loaded for a storage, a timestamp=NOW for the domain is added to a hash table
- this hash table is located in the persistent db implementation; there are two instances of it: for content data and for chrome data
- the timer lives in the db wrapper; this has not been changed
- the timer now fires to the dom storage manager class that also watches for shutdown and profile change, delegates flush attempt to the wrapper and ticks every 5 seconds
- no need for DOMStorageImpl to implement nsIObserver any more
- the flush timing logic has been simplified: a domain data is always flushed in 10 seconds after the load
- when all data is flushed (no data in the temp table) the timer is stopped (bug 616090)

Comment 7

[Josh Matthews [:jdm]]

>+  } else if (DOMStorageImpl::gStorageDB &&
>...
>+    if (DOMStorageImpl::gStorageDB) {

Unnecessary.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to comment #7)
> >+  } else if (DOMStorageImpl::gStorageDB &&
> >...
> >+    if (DOMStorageImpl::gStorageDB) {
> 
> Unnecessary.

Necessary for the test.

Comment 9

[Honza Bambas (:mayhemer)]

Ah, it is duplicated.  I see.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1

- just updated to comment 7

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v1 [Check in comment 12]

- updated conditions in Observe() ones more: missing parenthesis were causing crash when there were no dom storage created for lifetime of the session

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 503223 [details] [diff] [review]
v1 [Check in comment 12]

http://hg.mozilla.org/mozilla-central/rev/0d08db0bf676

Comment 13

[Daniel]

Brilliant, now it works! Great job!!

Comment 14

[Steve Scott (pxbugz)]

Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre