Last Comment Bug 746272 - localStorage data loss when read, remove, then read
: localStorage data loss when read, remove, then read
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 11 Branch
: x86 Mac OS X
: -- major (vote)
: mozilla15
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on: 962029 749182
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-17 12:14 PDT by James Morrin
Modified: 2014-01-21 03:29 PST (History)
5 users (show)
khuey: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This is how you get the expected behavior. (750 bytes, text/html)
2012-04-17 12:14 PDT, James Morrin
no flags Details
This file shows the bug behavior (752 bytes, text/html)
2012-04-17 12:16 PDT, James Morrin
no flags Details
Better Example Of Bug (880 bytes, text/html)
2012-04-18 05:08 PDT, James Morrin
no flags Details
v1 (3.42 KB, patch)
2012-04-19 07:24 PDT, Honza Bambas (:mayhemer)
bzbarsky: review+
mark.finkle: approval‑mozilla‑central+
Details | Diff | Splinter Review

Description James Morrin 2012-04-17 12:14:30 PDT
Created attachment 615814 [details]
This is how you get the expected behavior.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120312181643

Steps to reproduce:

I am reading a value from localStorage. Then I remove that item from localStorage. 


Actual results:

After removing the item from localStorage, all items are now missing not just the one removed. 


Expected results:

Only the removed item should be missing.
Comment 1 James Morrin 2012-04-17 12:16:28 PDT
Created attachment 615817 [details]
This file shows the bug behavior
Comment 2 Boris Zbarsky [:bz] 2012-04-17 19:53:02 PDT
I'm not sure I follow.  I load the "This file shows the bug behavior" testcase.  It says to refresh, as it should  I refresh it.  It logs "1" and the "[object Storage]", as expected.  At that point, running "window.localStorage.getItem('foo')" in the console returns "thing", also as expected.  What's the bug?
Comment 3 James Morrin 2012-04-18 05:08:56 PDT
Created attachment 616081 [details]
Better Example Of Bug

This file has a better example of the bug behavior using localStorage.length to illustrate expectations after reading a value and then removing it.
Comment 4 James Morrin 2012-04-18 05:23:04 PDT
I have updated the attachment with a better test case.

I am using ff 11 on mac OSX 10.7.3

When you run the test case it will stage items into the localStorage and ask you to refresh the page. It will also log the number of items that were staged. 

When you refresh the page it will read baz remove it and then print expected number of items in localStorage along with actual number of items. In my testing the number of items is always zero.

If you uncomment the console.log before the removal of baz it will work as expected.

As I was updating my test case I ran into some more interesting behaviors. 

After removing baz the length is 0. If I getItem('foo') I get the value. Then I check length and it is 1. If I then getItem('bar') I get the value and then I check length and it is 2. 

This is a problem in our web application and I am working around it by reading localStorage before removing an Item like in the test case. This behavior does not show up in any of our other supported browsers.
Comment 5 Boris Zbarsky [:bz] 2012-04-18 07:25:33 PDT
James, thanks.  I can reproduce that, and it definitely looks wrong.

Honza, what's going on here?  My first guess was the caching, but we MarkScopeDirty on RemoveKey...
Comment 6 Honza Bambas (:mayhemer) 2012-04-19 06:36:27 PDT
Cause: missing CacheKeysFromDB() call in RemoveItem().

- removeItem is the first operation on the newly create instance of localStorage (after refresh)
- since on all operations we always sync the in-memory cache (the mItems hashtable) with content of the database, we mark the storage instance as 'up-to-date' to save DB load
- but when removeItem is the first operation and we (mistakenly) don't load the cache with content of the database, the mItems cache is empty, hence the length is then reported as zero

The "interesting behavior" is then expected, since we put items in GetItem() to the mItems hash table.  So, every getItem of an existing item will increase number of items in mItems hashtable ; that is used to produce localStorage.length.

Patch coming.  Test as well.
Comment 7 Honza Bambas (:mayhemer) 2012-04-19 07:13:16 PDT
Side note: getItem is not supposed to cache items.

This code needs cleanup (not in this bug).  Some of the tricks are here just because of artifact of now unsupported backwards compatibility.
Comment 8 Honza Bambas (:mayhemer) 2012-04-19 07:24:10 PDT
Created attachment 616560 [details] [diff] [review]
v1

- adds CacheKeysFromDB() to RemoveItem() ; safe and simple
- adds a test based on the test case

James, thanks for this report and the test case!  It helped me to fix this bug so quickly.
Comment 9 Boris Zbarsky [:bz] 2012-04-19 08:34:37 PDT
Comment on attachment 616560 [details] [diff] [review]
v1

r=me
Comment 10 James Morrin 2012-04-19 09:01:34 PDT
No problem. It was a little tricky to track down! :) 

One last question from what I read in the comments I think I can simplify my work around?

Would reading any value, even if it doesnt exist in localStorage, force the cache on page load?

so I could do

localStorage.getItem('force_cache') and it would correct the issue? (force_cache would most likely not exist)

I would just like to make it very obvious what I am doing in my code with comments and self documenting code.
Comment 11 Honza Bambas (:mayhemer) 2012-04-19 09:09:41 PDT
(In reply to James Morrin from comment #10)
> No problem. It was a little tricky to track down! :) 
> 
> One last question from what I read in the comments I think I can simplify my
> work around?
> 
> Would reading any value, even if it doesnt exist in localStorage, force the
> cache on page load?

Use localStorage.length as the first operation, it caches all items.  Not just getItem or removeItem, those don't cache.

> 
> so I could do
> 
> localStorage.getItem('force_cache') and it would correct the issue?
> (force_cache would most likely not exist)

No, it won't work.  See comment 6 bellow why.

> 
> I would just like to make it very obvious what I am doing in my code with
> comments and self documenting code.
Comment 12 Honza Bambas (:mayhemer) 2012-04-19 09:13:26 PDT
Comment on attachment 616560 [details] [diff] [review]
v1

Simple, low risk patch.  However, this bug is here for a long time and there is a simple workaround.  I'll create a known-issue page for it somewhere...
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-25 20:40:27 PDT
https://hg.mozilla.org/mozilla-central/rev/691dfe3b2516
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2012-04-26 04:35:17 PDT
Comment on attachment 616560 [details] [diff] [review]
v1

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

::: dom/tests/mochitest/localstorage/test_bug746272-1.html
@@ +1,1 @@
> +<html xmlns="http://www.w3.org/1999/xhtml">

Quirks mode?

@@ +1,3 @@
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +<head>
> +<title>incomplete UTF-16 test</title>

I don't see incomplete UTF-16...
Comment 16 Honza Bambas (:mayhemer) 2012-04-26 11:01:23 PDT
(In reply to Ms2ger from comment #15)
> Comment on attachment 616560 [details] [diff] [review]
> v1
> 
> Review of attachment 616560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/tests/mochitest/localstorage/test_bug746272-1.html
> @@ +1,1 @@
> > +<html xmlns="http://www.w3.org/1999/xhtml">
> 
> Quirks mode?
> 
> @@ +1,3 @@
> > +<html xmlns="http://www.w3.org/1999/xhtml">
> > +<head>
> > +<title>incomplete UTF-16 test</title>
> 
> I don't see incomplete UTF-16...

I'll push a followup.  Thanks for noting.

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