746272 - localStorage data loss when read, remove, then read

Status: RESOLVED FIXED

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

Description

[James Morrin]

Attachment: 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]

Attachment: This file shows the bug behavior

Comment 2

[Boris Zbarsky [:bzbarsky]]

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]

Attachment: 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]

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 [:bzbarsky]]

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)]

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)]

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)]

Attachment: 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 [:bzbarsky]]

Comment on attachment 616560 [details] [diff] [review]
v1

r=me

Comment 10

[James Morrin]

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)]

(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)]

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 13

[Honza Bambas (:mayhemer)]

Comment on attachment 616560 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/691dfe3b2516

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/691dfe3b2516

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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)]

(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.