532062 - localStorage/sessionStorage should return undefined (not null) for undefined keys

Status: RESOLVED FIXED

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

Description

[bugzilla33]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; InfoPath.2; FDM)
Build Identifier: DOM Storage bug


empty values are casted to null instead undefined


Reproducible: Always

Steps to Reproduce:

Run attachment
Actual Results:  

Firefox 3.6: null


Expected Results:  

Firefox 3.6: undefined



Safari: undefined
Chrome 4.0: undefined
IE: undefined

Comment 1

[bugzilla33]

Attachment: source

Comment 2

[bugzilla33]

alert(localStorage.dfgergdfghsadgsdfasd) // shoul be undefined, no null

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

This depends on WebIDL specification, which is far from being ready, IIRC.

Comment 4

[bugzilla33]

In JS all no existing arrays/objects keys returns undefined.
NULL value, which had to be set by the programmer.

Comment 5

[bugzilla33]

Other browsers behave correctly.

Comment 6

[bugzilla33]

Try this code:

a=[]
alert(a.dfgergdfghsadgsdfasd)

Even Firefox returns undefined.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Ah, sorry, this is about localStorage.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I'm not quite sure what the summary should be.

Comment 9

[bugzilla33]

I think that the object localStorage should behave like any other objects and arrays. Should return undefined for undefined keys.

I am surprised other behavior for this one object in Firefox.

Other browsers do not have the specific behavior of the Storage object. They traditionally return undefined for non-existing keys.

Comment 10

[bugzilla33]

Where this idea that the Storage object behaved no-standard compared to all JS objects?

Comment 11

[bugzilla33]

In FF:

alert({}['qwerty']) // undefined
alert(localStorage['qwerty']) // null

mishmash

Comment 12

[bugzilla33]

Chrome 4 dev, IE, Safari:

alert({}['qwerty']) // undefined
alert(localStorage['qwerty']) // undefined

consistent behavior

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Other objects aside, localStorage is designed to behave like a JS object, so it's pretty clear that what we're doing is wrong here.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

mayhemer knows a lot more about localStorage than I do.

I assume the current behavior is based on the following
http://dev.w3.org/html5/webstorage/
"The getItem(key) method must return a structured clone of the current value
associated with the given key. If the given key does not exist in the list
associated with the object then this method must return null."

So we probably map storage.foo to storage.getItem("foo") even if storage
doesn't have foo as a key. The *draft* spec doesn't seem to define this case
properly, but we should probably do what others do.

mayhemer, has this been discussed somewhere?

Comment 15

[Honza Bambas (:mayhemer)]

Olli, that's exactly what I wanted to write here. It's not specified what "dynamic" properties and array indexing should return. What we do is delegate to getItem every time the property is not a method of the storage. This leads to returning null when the key is not present in (was not stored to) dom storage.

It seems to be a problem in the spec, more then in Gecko itself. We should rise this in WHATWG or W3C forums.

Changing back to UNCONFIRMED at the moment.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Where this idea that the Storage object behaved no-standard compared to all JS
> objects?

That's an immediate consequence of it being a host object, not a native object.  I suggest reading the ES spec carefully.

Un-ccing from this spam-fest.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree that the spec is unclear. However I always got the impression that Hixie intended for localStorage to largely behave like a normal JS object that got persisted.

Comment 18

[bugzilla33]

I think: FF has no caching for webStorage, look at speed tests:
https://bugzilla.mozilla.org/show_bug.cgi?id=536544

Comment 19

[Anne (:annevk)]

As I understand it from Web IDL and Web Storage localStorage.key should return undefined and localStorage.getItem("key") should return null if "key" is not actually a stored item.

Comment 20

[Honza Bambas (:mayhemer)]

-> me

Comment 21

[Ernst de Haan]

I think one of the tests in Microsoft's HTML5 test suite may also point out this same error, see this test:
http://samples.msdn.microsoft.com/ietestcenter/HTML5/DOMStorage/sessionStorage_empty_key.htm

Expected would be a PASS, but in Firefox 3.6.3 (Mac OS X 10.6.3) I get a FAIL.

If that is a different issue, please let me know and I will create a separate bug report.

For the whole list of Microsoft's HTML5 tests, see:
http://samples.msdn.microsoft.com/ietestcenter/html5.htm

Comment 22

[Martin Prebio]

Firefox 3.6.3:
console.log( typeof localStorage['inexistent'] );
object
console.log( localStorage['inexistent'] );
null

Chrome 5.0.375.55 beta:
console.log( typeof localStorage['inexistent']);
undefined
console.log( localStorage['inexistent']);
undefined

IE 8.0.7600.16385:
console.log( typeof localStorage['inexistent']);
undefined
console.log( localStorage['inexistent']);
undefined

Opera 10.53 Build 3374:
console.log( typeof localStorage['inexistent'] ); 
undefined
console.log( localStorage['inexistent']); 
undefined

;)

Comment 23

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The spec requires that when you do direct property access, like localStorage.foo, nonexistent items are undefined.  When you do localStorage.getItem("foo"), they're null.  This is unreasonable, but it's what all other browsers do, and what the spec says.

Web Storage says:

"""
getter any getItem(in DOMString key);

...

The supported property names on a Storage object are the keys of each key/value pair currently present in the list associated with the object.
"""
http://dev.w3.org/html5/webstorage/#the-storage-interface

WebIDL says:

"""
If an interface supports named properties, then the interface definition must be accompanied by a description of what names the object can be indexed with at any given time. These names are called the supported property names.
"""
http://dev.w3.org/2006/webapi/WebIDL/#idl-named-properties

Then in the ECMAScript binding:

"""
Whenever a given string N becomes a supported property name on the host object the following steps must be followed:
...
10. Call the [[DefineOwnProperty]] method of O passing P, Property Descriptor { [[Get]]: get, [[Set]]: set, [[Enumerable]]: true, [[Configurable]]: configurable }, and false as arguments.
"""
http://dev.w3.org/2006/webapi/WebIDL/#named-properties

And in the same section:

"""
As soon as a given string N stops being supported property name, then its corresponding named property, if it exists, must be removed from the host object.
"""

So WebIDL requires that you behave as though you've added a property every time a property name becomes supported, and remove it when it becomes unsupported.  The value on getting that property is then defined to be as if you called the getter.  If the property name is not supported -- in our case, not a key in localStorage -- then it was never set to anything, so getting it is supposed to return undefined.

It would be nice if getItem() were consistent with direct access, but we have 3/4 implementations plus the spec against that, so Firefox should change to match everything else.

Comment 24

[Honza Bambas (:mayhemer)]

No time right now to work on this.

Comment 25

[Josh Matthews [:jdm]]

It sounds like we just need to add an else clause to the relevant blocks in nsStorage2SH::NewResolve and nsStorageSH::NewResolve that sets *objp to JSVAL_VOID. It worth trying to see what happens, at least.

Comment 26

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

Got a patch. There's one issue though; we can't currently distinguish between null-as-a-real-key and null-because-there's-no-such-key. However, the latter isn't possible per spec, so we should fix that anyway. I filed bug 740771.

Comment 27

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

Attachment: Patch v1

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/c410b2d6d570