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();"> <p>window.localStorage.a = null; <i>typeof window.localStorage.a</i> = <span id="a"></span> <br />(It should be 'string')</p> <p>window.localStorage.setItem('b', null); <i>typeof window.localStorage.b</i> = <span id="b"></span> <br />(It should be 'string')</p> </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.
Created attachment 30782 [details] A potential fix.
Created attachment 30784 [details] Fixed changelog from last patch.
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.
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'.
(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 #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.)
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.
(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.
Created attachment 30801 [details] Fix this the right way.
Landed in @r44288 and @r44289 (oops!)