Bug 25970 - The implicit setter for DOM Storage does not handle null correctly.
Summary: The implicit setter for DOM Storage does not handle null correctly.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-22 16:01 PDT by Jeremy Orlow
Modified: 2009-05-30 15:39 PDT (History)
0 users

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 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.
Comment 1 Jeremy Orlow 2009-05-29 11:52:25 PDT
Created attachment 30782 [details]
A potential fix.
Comment 2 Jeremy Orlow 2009-05-29 11:56:07 PDT
Created attachment 30784 [details]
Fixed changelog from last patch.
Comment 3 Darin Adler 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.
Comment 4 Michael Nordman 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'.
Comment 5 Jeremy Orlow 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.  
Comment 6 Jeremy Orlow 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.)
Comment 7 Sam Weinig 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.
Comment 8 Jeremy Orlow 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.
Comment 9 Jeremy Orlow 2009-05-29 22:53:58 PDT
Created attachment 30801 [details]
Fix this the right way.
Comment 10 Brent Fulgham 2009-05-30 15:39:11 PDT
Landed in @r44288 and @r44289 (oops!)