573875 - for (var p in localStorage) is failtastic

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Not sure what's up here, the presence of both nsStorageSH and nsStorage2SH confuses my code-inspection abilities.  Probably an easy diagnosis/fix with a debugger, but I don't have an up-to-date debug build to check just right now.

Comment 1

[Jeff Walden [:Waldo]]

(The result is a "null is not a function" exception, to be clear -- should enumerate storage keys, I believe, and at the very least it shouldn't throw.)

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am seeing this problem in Firebug too:

in Fx3.6 following iteration works:

for (var name in window.sessionStorage)
{
    var value = window.sessionStorage[name];
    ...
}

while in Firefox 4, the first line throws "null is not a function" exception.

Honza

Comment 3

[John J. Barton]

regression, probably also affects localStorage.

Test case:
1. Install Firebug http://getfirebug.com/releases/firebug/1.7X (we use head but 1.7a8 should be out 1/15)
2. https://getfirebug.com/tests/content/dom/storage/storage.html
3. Follow instructions on page. Note that Firebug traps the exception so you won't see it in the Error Console. Use Firebug > Firebug Icon Menu > Open Tracing with Option FBS_ERRORS

OR:

1. same
2. Firebug > Console > paste:
for (var name in window.sessionStorage)
{
    var value = window.sessionStorage[name];
    console.log(value);
}

Comment 4

[Steve Fink [:sfink] [:s:]]

What's happening here is that when the iterator is fetched, nsStorage2SH::NewResolve is called, which has this comment:

  // GetItem() will return null if the caller can't access the session
  // storage item.

...which is what happens, because in nsDOMStorage.cpp, IsCallerSecure() is called and looks up the subject principal from some global security manager thing, which gives back the system principal. Then it tests:

  nsCOMPtr<nsIURI> codebase;
  subjectPrincipal->GetURI(getter_AddRefs(codebase));

  if (!codebase) {
    return PR_FALSE;
  }

and the system principal's GetURI() method is hardwired to return nsnull, so this returns PR_FALSE. If I do a total hack and return PR_TRUE if GetOrigin() returns the string "[System Principal]", then the test case executes successfully.

Seems odd that the system principal is considered to be an insecure caller, but that code hasn't changed since CVS days, so it probably makes sense. I am way, way out of my depth here.

Is there some reason why this is a disallowed access?

I'm gonna be rude and randomly pick someone to CC from the change log in the hopes that this is trivial to someone familiar with this stuff. If not, I guess we wait for blocker+.

Comment 5

[John J. Barton]

(In reply to comment #4)
> What's happening here is that when the iterator is fetched,
> nsStorage2SH::NewResolve is called, which has this comment:
> 
>   // GetItem() will return null if the caller can't access the session
>   // storage item.

Is GetItem() the implementation of storage getItem? null is a fine answer from getItem, see http://dev.w3.org/html5/webstorage/#dom-storage-getitem

Comment 6

[Steve Fink [:sfink] [:s:]]

I think it's used for that, yes, but in this case it's being called with the key "__iterator__" to look up a custom iterator.

But in looking closer, that IsCallerSecure() return value shouldn't immediately break things. It's used to restrict the returned items to the insecure subset. (So if you're a secure caller, you should see everything. If not, you should only see the "insecure" items. Or something.)

In which case, I don't know why my GetOrigin() hack changes anything. Investigating...

Comment 7

[Steve Fink [:sfink] [:s:]]

More confused than ever. Now it has completely different behavior -- the principal is now associated with the test page, and as a result my GetOrigin() hack has no effect. I really thought I had a clean build, but this is puzzling. Anyway, ignore my speculation above; it no longer seems to bear any resemblance to reality.

Comment 8

[Josh Matthews [:jdm]]

If there's an example of how other special DOM classes handle __iterator__, I'd be interested in pointers.  The immediate failure here seems to be that nsStorage2SH::NewResolve is retrieving a key named __iterator__ from localStorage, which gives back a null value so we just return.

Comment 9

[John J. Barton]

From code by Atul Varma, note how the failure of the key is backed up by another function, eg, 1st the obj then the default

http://code.google.com/p/fbug/source/browse/examples/firebug1.7/HelloModule/modules/securable-module-requirejs.js?spec=svn8654&r=8654
             // Override the default Iterator function with one that passes
             // a second argument to custom iterator methods that identifies
             // the call as originating from an Iterator function so the custom
             // iterator method can return [key, value] pairs just like default
             // iterators called via the default Iterator function.

            sandbox.defineProperty('Iterator',
               (function(DefaultIterator) {
                 return function Iterator(obj, keysOnly) {
                   if ("__iterator__" in obj && !keysOnly)
                     return obj.__iterator__.call(obj, false, true);
                   return DefaultIterator(obj, keysOnly);
                 };
               })(sandbox.globalScope.Iterator)
             );

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Is there some reason why this is a disallowed access?

ccing the guy with the blame.... if he still remembers, and we still care.  ;)  Was there a reason you didn't look up the blame yourself?  We have a whole website for that sort of thing.

> If there's an example of how other special DOM classes handle __iterator__

They don't, usually.  Storage is a very rare thing that makes all keys look like they exist...  You could experiment with things like <form><input name="__iterator__"> and then enumerating the form's properties, though.  I bet it breaks!

Josh, who's resolving __iterator__ and why are they doing it for a class that has a newEnumerate hook?

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to comment #10)
> > Is there some reason why this is a disallowed access?
> 
> ccing the guy with the blame.... if he still remembers, and we still care.  ;) 
> Was there a reason you didn't look up the blame yourself?  We have a whole
> website for that sort of thing.

What website? I used hg annotate, but it just said it dated from revision 1, which I assume means CVS days. MXR has an hg blame link that points to hg.mozilla.org, which does the same thing.

Anyway, it was a red herring in this case. I still have no idea why it was giving me the system principal before. Now I can't get it to do anything other than the firebug principal, which makes more sense.

> > If there's an example of how other special DOM classes handle __iterator__
> 
> They don't, usually.  Storage is a very rare thing that makes all keys look
> like they exist...  You could experiment with things like <form><input
> name="__iterator__"> and then enumerating the form's properties, though.  I bet
> it breaks!
> 
> Josh, who's resolving __iterator__ and why are they doing it for a class that
> has a newEnumerate hook?

nsDOMStorage is resolving it, along with everything else. I can't follow when the newEnumerate hook gets used. The only calls to eg getOps()->enumerate seem to be for skipping over stuff.

I could special-case GetItem to return undefined for "__iterator__" rather than null, but I don't know if there's a better way to have it go straight to the class's newEnumerate.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> What website? 

http://mxr.mozilla.org/firefox/ (which is basically MXR for the Gecko 1.9.0 branch).

The file listings there have links to CVS blame, not hg blame, since that code lives in CVS.

> nsDOMStorage is resolving it

nsDOMStorage is returning a value for it, sure.  My question is who is _asking_ for the value.  For example, what's the callstack at that point (and in which hg revision)?

> I can't follow when the newEnumerate hook gets used

It's used in various places; what I want to know is how whoever asks for __iterator__ is related to those places.

> I could special-case GetItem to return undefined for "__iterator__"

That would happen to be a bug, per spec, no?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fwiw, my current guess based on code inspection is that we land in GetIterator in jsiter.cpp in the keysOnly case.  This does some stuff including:

677                 if (!pobj->isNative() ||
678                     obj->getOps()->enumerate ||
679                     pobj->getClass()->enumerate != JS_EnumerateStub) {
680                     shapes.clear();
681                     goto miss;
682                 }

and the "miss" label has:

709         if (obj->isProxy())
710             return JSProxy::iterate(cx, obj, flags, vp);
711         if (!GetCustomIterator(cx, obj, flags, vp))
712             return false;
713         if (!vp->isUndefined())
714             return true;

Do we really want to have GetCustomIterator stuff going on for classes with an enumerate hook?  That seems... odd.  It _really_ doesn't play well with DOM stuff of various sorts, not just storage.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Though I can't quite tell for _this_ particular case what webidl calls for.  The supported property names are the ones that there are values for in storage, per <http://dev.w3.org/html5/webstorage/#the-storage-interface>.  What does that mean in terms of what storage.__iterator__ should do if there is nothing stored in the storage for the string "__iterator__".  Should it be calling the getter defined on that interface?  The text at http://www.w3.org/TR/WebIDL/#dfn-support-named-properties for getters and creators looks identical; neither mentions "supported property names".

Comment 15

[Josh Matthews [:jdm]]

Attachment: NewResolve backtrace

Here's the backtrace, if it's still interesting.  We end up hitting the miss: case in GetIterator and call GetCustomIterator from there.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Excellent.  So comment 13 sounds like what's going on, yes.

Comment 17

[Andreas Gal :gal]

Attachment: patch

Comment 18

[Andreas Gal :gal]

Attachment: patch

Passes tests.

Comment 19

[Andreas Gal :gal]

Comment on attachment 503731 [details] [diff] [review]
patch

r- via sms from Brendan, not the right fix.

Comment 20

[Brendan Eich [:brendan]]

The perils of JS1.7's Pythonic unstratified meta-object protocol traps (for more, see http://brendaneich.com/2010/11/proxy-inception/). Here's what a Fx3-era js shell does with a minimal test:

js> o = {__iterator__:null, a:1, b:2, c:3}
[object Object]
js> for (i in o) print(i)
typein:2: TypeError: null has invalid __iterator__ value null

The error message is poor, it should cite "o", not "null" (the first "null").

Did localStorage behave differently then? I don't have an xpcshell build from that era.

Anyway, current builds behave about the same, but with a less ambitious error message:

js> o = {__iterator__:null, a:1, b:2, c:3}
({__iterator__:null, a:1, b:2, c:3})
js> for (i in o) print(i)
typein:2: TypeError: null is not a function

We want to get rid of JS1.7's iteration protocol, but the Harmony version based on Proxies (which are in harmony:proposals, so likely to be in ES6 or whatever the next major edition will be numbered), but the Harmony iterators proposal is not yet approved. TC39 did approve having a meta-object protocol for iteration based on proxies, just not the detailsl and especially syntax.

We wouldn't get rid of __iteartor__ without a release of deprecation (Firefox 5) and a harmony:proposals status for its replacement.

So in the mean time, two ideas:

1. Break compatibility by insisting on __iterator__ having object value, not merely being defined.

2. Do not blindly resolve every key queried against localStorage.

(1) ought to be enough to fix this bug.

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 22

[Brendan Eich [:brendan]]

Oops, I meant to write this instead of the mangled paragraph in comment 20:

"We want to get rid of JS1.7's iteration protocol. While proxies are approved in harmony:proposals (so likely to be in ES6 or whatever the next major edition will be numbered), the Harmony iterators proposal is not yet approved. TC39 did approve having a meta-object protocol for iteration based on proxies, just not the detailsl and especially syntax."

/be

Comment 23

[Andreas Gal :gal]

Comment on attachment 503851 [details] [diff] [review]
proposed fix

I don't think this is a better fix. I like the idea of not doing this non-stratified protocol on host objects at all. Does this really fix the localStorage text case? If so, r=me, but reluctantly.

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> Comment on attachment 503851 [details] [diff] [review]
> proposed fix
> 
> I don't think this is a better fix. I like the idea of not doing this
> non-stratified protocol on host objects at all.

We've supported that for over four years, and people do it. Doesn't matter how you or I feel about it at this point -- the way to break this is with a harmony status iteration protocol based on proxies, *after* we've shipped that in a real release.

> Does this really fix the localStorage text case?

Yes, I tested this bug's URL in a tm build with the patch.

But why did you have to ask? If someone else should review, I'll ask. This is an easy bug: null !== undefined, and the GetCustomIterator code takes any value other than undefined as a valid __iterator__ value and tries to invoke it.

/be

Comment 25

[Brendan Eich [:brendan]]

(In reply to comment #24)
> (In reply to comment #23)
> > Comment on attachment 503851 [details] [diff] [review]
> > proposed fix
> > 
> > I don't think this is a better fix. I like the idea of not doing this
> > non-stratified protocol on host objects at all.
> 
> We've supported that for over four years, and people do it.

One reason people do it is because for-in on DOM nodes is not very useful. A proxy-based iteration protocol should work on DOM nodes too, although it will require wrapping (stratification). The use-case is valid, and compatibility is what it is. Random incompatibility to "fix" this bug is just a bad fix, which is why I minused that patch after too-hastily plussing it.

/be

Comment 26

[Brendan Eich [:brendan]]

Sorry, should have clarified:

"Random incompatibility" means a behavior change to error-free code, neither removing nor adding existing error cases. That's what the first patch did.

The second patch removes inevitable error cases trying to invoke primitive __iterator__ values, so it is fully backward compatible with error-free code where __iterator__ is an iterator factory, e.g. a generator function.

/be

Comment 27

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/1681112ed440

/be

Comment 28

[Jan de Mooij [:jandem]]

http://hg.mozilla.org/mozilla-central/rev/1681112ed440