Bug 25970 - The implicit setter for DOM Storage does not handle null correctly.
: The implicit setter for DOM Storage does not handle null correctly.
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-05-22 16:01 PST by
Modified: 2009-05-30 15:39 PST (History)


Attachments
A potential fix. (8.63 KB, patch)
2009-05-29 11:52 PST, Jeremy Orlow
no flags Review Patch | Details | Formatted Diff | Diff
Fixed changelog from last patch. (8.33 KB, patch)
2009-05-29 11:56 PST, Jeremy Orlow
no flags Review Patch | Details | Formatted Diff | Diff
Fix this the right way. (8.28 KB, patch)
2009-05-29 22:53 PST, Jeremy Orlow
sam: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-05-22 16:01:24 PST
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 From 2009-05-29 11:52:25 PST -------
Created an attachment (id=30782) [details]
A potential fix.
------- Comment #2 From 2009-05-29 11:56:07 PST -------
Created an attachment (id=30784) [details]
Fixed changelog from last patch.
------- Comment #3 From 2009-05-29 11:56:33 PST -------
(From update of attachment 30782 [details])
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 From 2009-05-29 17:35:28 PST -------
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 From 2009-05-29 19:01:07 PST -------
(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 From 2009-05-29 19:07:59 PST -------
(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 From 2009-05-29 20:24:32 PST -------
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 From 2009-05-29 20:45:58 PST -------
(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 From 2009-05-29 22:53:58 PST -------
Created an attachment (id=30801) [details]
Fix this the right way.
------- Comment #10 From 2009-05-30 15:39:11 PST -------
Landed in @r44288 and @r44289 (oops!)