Full Text Bug Listing

Jeremy Orlow

Reported 2009-05-22 16:01:24 PDT

DOM Storage is supposed to only store data in string key/value pairs. setItem seems to work as expected, but the implicit setter for LocalStorage and SessionStorage appears to not handle this case correctly. The following reduction demonstrates this: <html><head><title>Safari Reductions</title> <script> function test() { window.localStorage.a = null; document.getElementById("a").innerHTML = typeof window.localStorage.a; window.localStorage.setItem('b', null); document.getElementById("b").innerHTML = typeof window.localStorage.b; } </script> </head><body onload="test();"> window.localStorage.a = null; typeof window.localStorage.a = (It should be 'string') window.localStorage.setItem('b', null); typeof window.localStorage.b = (It should be 'string') </body> </html> Note that this code triggers an ASSERT(!value.isNull()) check in the setItem implementation. I'll attempt to fix this (and create a layout test to catch it) in the next couple weeks.

Jeremy Orlow

Comment 1 2009-05-29 11:52:25 PDT

Created attachment 30782 [details] A potential fix.

Jeremy Orlow

Comment 2 2009-05-29 11:56:07 PDT

Created attachment 30784 [details] Fixed changelog from last patch.

Darin Adler

Comment 3 2009-05-29 11:56:33 PDT

Comment on attachment 30782 [details] A potential fix. I supposed it's OK to put the fix down here at the lowest level, but the use of the string "null" seems specific to JavaScript and so it seems to me this should be handled in the JavaScript bindings.

Michael Nordman

Comment 4 2009-05-29 17:35:28 PDT

Do other non-string values types (in particular undefined) have similar issues... var undefinedVar; var array = [1,2,3]; var number = 1.01; localStorage.a = T; localStorage.setItem('b', T); ... where the behavior is different depending on the style of setter used? Also, I gotta wonder about the utility of translating null to 'null'.

Jeremy Orlow

Comment 5 2009-05-29 19:01:07 PDT

(In reply to comment #4) > Do other non-string values types (in particular undefined) have similar > issues... > > var undefinedVar; > var array = [1,2,3]; > var number = 1.01; > localStorage.a = T; > localStorage.setItem('b', T); > > ... where the behavior is different depending on the style of setter used? > > Also, I gotta wonder about the utility of translating null to 'null'. > I added in testing to do an int and a function in an attempt to catch such things, but I suppose I could add a new Array() to it, but it's pretty clear if you look at the code that null is a special case because strings can be null or an actual string.

Jeremy Orlow

Comment 6 2009-05-29 19:07:59 PDT

(In reply to comment #4) > Do other non-string values types (in particular undefined) have similar > issues... > > var undefinedVar; > var array = [1,2,3]; > var number = 1.01; > localStorage.a = T; > localStorage.setItem('b', T); > > ... where the behavior is different depending on the style of setter used? > > Also, I gotta wonder about the utility of translating null to 'null'. > I added in testing to do an int and a function in an attempt to catch such things, but I suppose I could add a new Array() to it, but it's pretty clear if you look at the code that null is a special case because strings can be null or an actual string. (In reply to comment #3) > (From update of attachment 30782 [details] [review]) > I supposed it's OK to put the fix down here at the lowest level, but the use of > the string "null" seems specific to JavaScript and so it seems to me this > should be handled in the JavaScript bindings. > I agree; I don't like having this outside of the JavaScript code either. That said, the setter function is a pretty generic one that looks to be shared with many other things that where it being null is not desirable. Maybe it's worth creating another one and creating a flag in the .idl bindings that'd tell it to use it? (Not sure exactly how that'd work, but I could research it.)

Sam Weinig

Comment 7 2009-05-29 20:24:32 PDT

This should be easily fixable in JSStorageCustom.cpp in the customPut function. Replacing the valueToStringWithNullCheck(exec, value), with value.toString(exec) should do the trick.

Jeremy Orlow

Comment 8 2009-05-29 20:45:58 PDT

(In reply to comment #7) > This should be easily fixable in JSStorageCustom.cpp in the customPut function. > Replacing the valueToStringWithNullCheck(exec, value), with > value.toString(exec) should do the trick. > Ugh. You're right. I was looking at the wrong function. I'm testing a new patch right now.

Jeremy Orlow

Comment 9 2009-05-29 22:53:58 PDT

Created attachment 30801 [details] Fix this the right way.

Brent Fulgham

Comment 10 2009-05-30 15:39:11 PDT

Landed in @r44288 and @r44289 (oops!)

Attachments
A potential fix. (8.63 KB, patch) 2009-05-29 11:52 PDT, Jeremy Orlow	no flags	Details Formatted Diff Diff
Fixed changelog from last patch. (8.33 KB, patch) 2009-05-29 11:56 PDT, Jeremy Orlow	no flags	Details Formatted Diff Diff
Fix this the right way. (8.28 KB, patch) 2009-05-29 22:53 PDT, Jeremy Orlow	sam: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Bug 25970