Bug 25970

Summary: The implicit setter for DOM Storage does not handle null correctly.
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
A potential fix.
none
Fixed changelog from last patch.
none
Fix this the right way. sam: review+

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();"> <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.
Attachments
A potential fix. (8.63 KB, patch)
2009-05-29 11:52 PDT, Jeremy Orlow 		no flags
DetailsFormatted DiffDiff
Fixed changelog from last patch. (8.33 KB, patch)
2009-05-29 11:56 PDT, Jeremy Orlow 		no flags
DetailsFormatted DiffDiff
Fix this the right way. (8.28 KB, patch)
2009-05-29 22:53 PDT, Jeremy Orlow 		sam: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
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!)
Note You need to log in before you can comment on or make changes to this bug.