localStorage data loss when read, remove, then read

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: James Morrin, Assigned: mayhemer)

Tracking

(Depends on: 1 bug)

11 Branch
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Severity: normal → major
(Reporter)

Comment 1

5 years ago
Created attachment 615817 [details]
This file shows the bug behavior
(Reporter)

Updated

5 years ago
Attachment #615814 - Attachment description: localStoreBug.html → This is how you get the expected behavior.
Component: Untriaged → DOM
Product: Firefox → Core
QA Contact: untriaged → general

Updated

5 years ago
Attachment #615814 - Attachment mime type: text/plain → text/html

Updated

5 years ago
Attachment #615817 - Attachment mime type: text/plain → text/html

Comment 2

5 years ago
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?
(Reporter)

Comment 3

5 years ago
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.
Attachment #615814 - Attachment is obsolete: true
Attachment #615817 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #616081 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 4

5 years ago
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

5 years ago
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...
(Assignee)

Updated

5 years ago
Assignee: nobody → honzab.moz
(Assignee)

Comment 6

5 years ago
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Comment 8

5 years ago
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.
Attachment #616560 - Flags: review?(bzbarsky)

Comment 9

5 years ago
Comment on attachment 616560 [details] [diff] [review]
v1

r=me
Attachment #616560 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
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...
Attachment #616560 - Flags: approval-mozilla-central?
Attachment #616560 - Flags: approval-mozilla-central? → approval-mozilla-central+
(Assignee)

Comment 13

5 years ago
Comment on attachment 616560 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/691dfe3b2516
https://hg.mozilla.org/mozilla-central/rev/691dfe3b2516
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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...

Updated

5 years ago
Depends on: 749182
(Assignee)

Comment 16

5 years ago
(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.
Depends on: 962029
You need to log in before you can comment on or make changes to this bug.