Last Comment Bug 783829 - Using for-in on a proxy should call the enumerate trap, not the iterate trap
: Using for-in on a proxy should call the enumerate trap, not the iterate trap
Status: RESOLVED FIXED
[js:t]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
: 783826 (view as bug list)
Depends on: 783826 1091900 1115844
Blocks: 978228 703537 986294 1099399 1110457 1114070
  Show dependency treegraph
 
Reported: 2012-08-18 16:17 PDT by Eddy Bruel [:ejpbruel]
Modified: 2015-01-07 12:57 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
a showcase that for( .. in .. ) is not handed over to a Proxy handler at all (3.14 KB, text/plain)
2013-07-04 01:08 PDT, Peter Kehl
no flags Details
Rename enumerate to getEnumerablePropertyKeys (31.58 KB, patch)
2014-11-13 13:58 PST, Tom Schuster [:evilpie]
efaustbmo: review+
evilpies: checkin+
Details | Diff | Splinter Review
preview, gets stuff working in the shell (78.92 KB, patch)
2014-11-13 14:29 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
shim-legacy-generator (1.95 KB, patch)
2014-11-14 14:01 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
shim-legacy-iterator (6.57 KB, patch)
2014-11-14 14:02 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
new-iter-proto (5.80 KB, patch)
2014-11-14 14:04 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
change-iterate-to-enumerate (58.34 KB, patch)
2014-11-14 14:06 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
shim-legacy-generator v2 (1.87 KB, patch)
2014-11-15 10:25 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
shim-legacy-iterator v2 (6.65 KB, patch)
2014-11-15 10:25 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
Fix JS tests (12.35 KB, patch)
2014-11-15 10:56 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v1 - Factor out NativeIteratorNext (4.25 KB, patch)
2014-12-12 06:51 PST, Tom Schuster [:evilpie]
efaustbmo: review+
Details | Diff | Splinter Review
v1 - Change from iterate to [[enumerate]] (64.35 KB, patch)
2014-12-12 06:56 PST, Tom Schuster [:evilpie]
bobbyholley: review-
Details | Diff | Splinter Review
v1 - Some tests (2.67 KB, patch)
2014-12-12 06:57 PST, Tom Schuster [:evilpie]
efaustbmo: review+
Details | Diff | Splinter Review
v1 - Fix a single thing in the browser (2.52 KB, patch)
2014-12-12 09:04 PST, Tom Schuster [:evilpie]
mak77: review+
Details | Diff | Splinter Review
WIP - Remove getEnumerablePropertyKeys (44.25 KB, patch)
2014-12-12 15:35 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v2 - Change from iterate to [[enumerate]] (64.34 KB, patch)
2014-12-13 08:02 PST, Tom Schuster [:evilpie]
efaustbmo: review+
bobbyholley: review+
Details | Diff | Splinter Review
v1 - Remove getEnumerablePropertyKeys (44.42 KB, patch)
2014-12-13 08:03 PST, Tom Schuster [:evilpie]
bobbyholley: review+
efaustbmo: review+
Details | Diff | Splinter Review
v1 - Fix for enumerating enumerable symbols (3.59 KB, patch)
2014-12-13 08:05 PST, Tom Schuster [:evilpie]
efaustbmo: review+
Details | Diff | Splinter Review

Description Eddy Bruel [:ejpbruel] 2012-08-18 16:17:48 PDT
The for-in statement doesn't actually call Proxy::enumerate, as per spec. Instead, it calls Proxy::iterate (see jsiter.cpp:683). This should be fixed.
Comment 1 David Bruant 2012-09-02 12:10:08 PDT
Mentionned in https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Proxy#Handler_API
Update the doc when this bug is fixed
Comment 2 Peter Kehl 2013-07-04 01:06:01 PDT
Actually, in both Firefox 22 and 23b02 (on CentOS 6.2 x64), for( .. in .. ) doesn't invoke neither enumerate() nor iterate() of a Proxy object - the for( .. in .. ) trap doesn't work at all. See attached Load2.html.

The only workaround is to have an iterator/generator on the target object (not on the Proxy itself), as per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Iterators_and_Generators
Comment 3 Peter Kehl 2013-07-04 01:08:12 PDT
Created attachment 771226 [details]
a showcase that for( .. in .. ) is not handed over to a Proxy handler at all

See my comment just a moment ago
Comment 4 Tom Schuster [:evilpie] 2013-07-04 07:38:30 PDT
Interesting, I came around this bug yesterday as well. I was trying to figure out when the Proxy::enumerate trap is invoked and I wasn't really able to find any solution to that.
Comment 5 Tom Schuster [:evilpie] 2014-10-28 13:29:37 PDT
I believe this still needs to be fixed. Eric is that something you would be interested in?
Comment 6 Tom Schuster [:evilpie] 2014-11-13 13:13:05 PST
*** Bug 783826 has been marked as a duplicate of this bug. ***
Comment 7 Tom Schuster [:evilpie] 2014-11-13 13:58:12 PST
Created attachment 8522508 [details] [diff] [review]
Rename enumerate to getEnumerablePropertyKeys

Our implementation of enumerate is not what expects today. So rename it to something better.
Comment 8 Eric Faust [:efaust] 2014-11-13 14:06:19 PST
Comment on attachment 8522508 [details] [diff] [review]
Rename enumerate to getEnumerablePropertyKeys

Review of attachment 8522508 [details] [diff] [review]:
-----------------------------------------------------------------

Yes. This is a better name, and leaves enumerate free to write the actual [[Enumerate]] work. r=me with question below answered. Otherwise, this should land immediately, I would think.

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +760,5 @@
>  }
>  
>  // ES6 (22 May, 2014) 9.5.11 Proxy.[[Enumerate]]
>  bool
> +ScriptedDirectProxyHandler::getEnumerablePropertyKeys(JSContext *cx, HandleObject proxy, AutoIdVector &props) const

We are going to want to do something about this when the rest of this bug lands, right? This function should eventually not directly invoke the JS callback, and instead stub out to the [[Enumerate]] that does, or there'll be a helper or something?
Comment 9 Tom Schuster [:evilpie] 2014-11-13 14:29:44 PST
Created attachment 8522530 [details] [diff] [review]
preview, gets stuff working in the shell
Comment 10 Tom Schuster [:evilpie] 2014-11-14 07:23:33 PST
I missed some trivial pieces in the patch that I uploaded, sorry! I renamed an overload of "enumerate" to the better fitting "getPropertyKeys" in XrayWrapper.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b57e073e6fb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/494c68e8fe37
Comment 11 Tom Schuster [:evilpie] 2014-11-14 14:01:23 PST
Created attachment 8523198 [details] [diff] [review]
shim-legacy-generator

We are introducing a new iterator protocol, which works like that of "for .. of". i.e. return an object with {done, value}. So we invoke the same code that is called for @@iterator on those.
Comment 12 Tom Schuster [:evilpie] 2014-11-14 14:02:30 PST
Created attachment 8523200 [details] [diff] [review]
shim-legacy-iterator

The objects that were used with __iterator__ obviously used the old iterator protocol, so wrap those as well. I really hope we can remove this in the future.
Comment 13 Tom Schuster [:evilpie] 2014-11-14 14:04:18 PST
Created attachment 8523203 [details] [diff] [review]
new-iter-proto

Introduce the new ES6 iteration protocol. Also changes PropertyIteratorObject.next to the new way of returning done/value. Most of the time however nothing really changes, because we still directly poke at the NativeIterator.
Comment 14 Tom Schuster [:evilpie] 2014-11-14 14:06:32 PST
Created attachment 8523206 [details] [diff] [review]
change-iterate-to-enumerate

Renames the iterate trap to enumerate. Drops the now necessary flags argument, because this is only supposed to be used in [[Enumerate]] case. This makes the enumerate trap mandatory, because this is one of the basic ES6 traps. I think we maintain backwards-compatibility with both direct and indirect proxies.
Comment 15 Ryan VanderMeulen [:RyanVM] 2014-11-14 14:54:48 PST
https://hg.mozilla.org/mozilla-central/rev/494c68e8fe37
Comment 16 Tom Schuster [:evilpie] 2014-11-15 04:49:02 PST
Now that we pass around LegacyGeneratorObjects as a JS shim the code in CloseIterator that tests for is<LegacyGenerator> won't work. Is that a big deal?
Andy I asked you for review, because I think you probably have the most experience with Iterators and all that shimming business. However feel free to cancel and I am going to ask somebody else.
Comment 17 Tom Schuster [:evilpie] 2014-11-15 09:29:09 PST
Okay, I probably have to adjust the wrapping a little bit. Right now we also do the wrapping for Iterator(), but we should only do it when we are going to use for .. in on the object. This should fix a few test failures.
Comment 18 Tom Schuster [:evilpie] 2014-11-15 10:25:20 PST
Created attachment 8523441 [details] [diff] [review]
shim-legacy-generator v2
Comment 19 Tom Schuster [:evilpie] 2014-11-15 10:25:51 PST
Created attachment 8523442 [details] [diff] [review]
shim-legacy-iterator v2
Comment 20 Tom Schuster [:evilpie] 2014-11-15 10:56:15 PST
Created attachment 8523444 [details] [diff] [review]
Fix JS tests

Most of these is just a slight change in behavior, mostly caused by not invoking close properly and changes to how try .. finally works. I doubt this is big issue, it's kind of an edge case. Two tests fail because next() doesn't throw StopIterator anymore. One test fails, because we don't invoke the ::iterate trap anymore unless you use for .. in directly.
All in all surprising little actual problems.
Comment 21 Tom Schuster [:evilpie] 2014-11-15 13:34:58 PST
There are still quite a few test failures that aren't fun to debug https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=120a30ddb9fe. During some of that investigation I found out about /toolkit/components/osfile, seems like that code use __iterator__, legacy generators & co. quite a bit. And it looks like it uses .close to close file handles?! http://mxr.mozilla.org/mozilla-central/search?string=DirectoryIterator_prototype_close
Comment 22 Andy Wingo [:wingo] 2014-12-03 02:46:46 PST
Comment on attachment 8523441 [details] [diff] [review]
shim-legacy-generator v2

Review of attachment 8523441 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the late review.  I must be thick this morning but I am missing the big picture.  What is the problem, and what is being fixed?  If it is user-visible, where are the test cases?

::: js/src/jsiter.cpp
@@ +705,5 @@
> +        return true;
> +    }
> +
> +    if (obj->is<LegacyGeneratorObject>()) {
> +        // Only shim when the Iterator is used for "for .. in" directly

I don't understand what the intention is.  What are you trying to do? :)
Comment 23 Tom Schuster [:evilpie] 2014-12-03 12:08:35 PST
Consider, something like this:
var f = function() { yield 1;}
var i = Iterator(f())
for (x in i) {}

I want to avoid shimming the generator that is returned from Iterator, and instead just shim it when we will actually use "for .. in" to iterate over it.
Comment 24 Andy Wingo [:wingo] 2014-12-05 05:37:24 PST
(In reply to Tom Schuster [:evilpie] from comment #23)
> Consider, something like this:
> var f = function() { yield 1;}
> var i = Iterator(f())
> for (x in i) {}
> 
> I want to avoid shimming the generator that is returned from Iterator, and
> instead just shim it when we will actually use "for .. in" to iterate over
> it.

Why?

Also, why are you using for-in in this way?  This is legacy syntax.  The way forward is to use for-of instead.
Comment 25 Tom Schuster [:evilpie] 2014-12-08 06:23:09 PST
(In reply to Andy Wingo [:wingo] from comment #24)
> (In reply to Tom Schuster [:evilpie] from comment #23)
> > Consider, something like this:
> > var f = function() { yield 1;}
> > var i = Iterator(f())
> > for (x in i) {}
> > 
> > I want to avoid shimming the generator that is returned from Iterator, and
> > instead just shim it when we will actually use "for .. in" to iterate over
> > it.
> 
> Why?
> 
> Also, why are you using for-in in this way?  This is legacy syntax.  The way
> forward is to use for-of instead.
Well I don't, but tons of Firefox/add-on code does. I am not really interested in fixing that myself.
Comment 26 Tom Schuster [:evilpie] 2014-12-11 12:38:24 PST
I am going to try a new approach here. Instead of shimming we will just support both the old and new style iteration protocol in js::IteratorMore. But we should really have a plan to fix this properly soonish.
Comment 27 Tom Schuster [:evilpie] 2014-12-12 06:51:47 PST
Created attachment 8535607 [details] [diff] [review]
v1 - Factor out NativeIteratorNext

Not really necessary now, but makes the code easier to understand.
Comment 28 Tom Schuster [:evilpie] 2014-12-12 06:56:53 PST
Created attachment 8535609 [details] [diff] [review]
v1 - Change from iterate to [[enumerate]]

Only calls the Proxy::enumerate trap when GetIterator is called in a for-in context, or from the DirectProxyHandler. Instead of shimming when now just allow both the old and new iterator protocol in IteratorMore. This seems to yield basically perfect backwards-compatibility. If we go ahead with remvoing __iterator__, we could just check for PropertyIterator and LegacyGenerator and totally move to the new protocol. I am not sure about SandboxProxyHandler::enumerate.
Comment 29 Tom Schuster [:evilpie] 2014-12-12 06:57:35 PST
Created attachment 8535611 [details] [diff] [review]
v1 - Some tests
Comment 30 Tom Schuster [:evilpie] 2014-12-12 09:04:08 PST
Created attachment 8535664 [details] [diff] [review]
v1 - Fix a single thing in the browser

Honestly I don't really understand what is going to wrong here. Using 'for each' with generators is kind of confusing anyway.
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2014-12-12 12:09:03 PST
Comment on attachment 8535609 [details] [diff] [review]
v1 - Change from iterate to [[enumerate]]

Review of attachment 8535609 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/Sandbox.cpp
@@ +742,3 @@
>  {
> +    // ??
> +    return DirectProxyHandler::enumerate(cx, proxy, objp);

The point here is that this operation needs to be implemented in terms of the other fundamental traps that are overridden for this proxy handler. IIUC, bouncing to DirectProxyHandler is just going to forward to the underlying object, which doesn't take the other traps into account.

That being said, it doesn't look like any of the special logic we do in getPropertyDescriptor would actually affect enumerate here (it doesn't change the existence of properties or their enumerability - just their values).

If I'm right, we can kill this override entirely for SandboxProxyHandler, but it should happen in a separate patch.
Comment 32 Tom Schuster [:evilpie] 2014-12-12 13:52:31 PST
So should I call BaseProxyHandler::enumerate? I made ::enumerate pure virtual so it has to be implemented, and if we also don't want want DirectProxyHandler::enumerate. BaseProxyHandler::enumerate would call getEnumerablePropertyKeys which is interestingly also not overridden.
Comment 33 Bobby Holley (:bholley) (busy with Stylo) 2014-12-12 13:54:42 PST
(In reply to Tom Schuster [:evilpie] from comment #32)
> So should I call BaseProxyHandler::enumerate? I made ::enumerate pure
> virtual so it has to be implemented, and if we also don't want want
> DirectProxyHandler::enumerate. BaseProxyHandler::enumerate would call
> getEnumerablePropertyKeys which is interestingly also not overridden.

My point was that we don't need to be overriding this at for this handler.
Comment 34 Eric Faust [:efaust] 2014-12-12 14:10:28 PST
Comment on attachment 8535609 [details] [diff] [review]
v1 - Change from iterate to [[enumerate]]

Review of attachment 8535609 [details] [diff] [review]:
-----------------------------------------------------------------

I cannot speak to the Sandbox stuff, but we should fix the getEnumerablePropertyKeys thing before this goes in. Most of it looks good, but I would like to see the resolution again.

::: js/ipc/WrapperOwner.cpp
@@ +271,5 @@
>  bool
> +CPOWProxyHandler::enumerate(JSContext *cx, HandleObject proxy, MutableHandleObject objp) const
> +{
> +    // Using a CPOW for the Iterator would slow down for .. in performance, instead
> +    // call the base hook, that will us our implementation of getEnumerablePropertyKeys.

typo: that will us

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +801,5 @@
> +}
> +
> +// Non-standard, try to convert iterator result to ids
> +bool
> +ScriptedDirectProxyHandler::getEnumerablePropertyKeys(JSContext *cx, HandleObject proxy, AutoIdVector &props) const

As per IRC discussion, this is incorrect because of its invocation for proxies on the prototype chain of native objects, and thus, if I understand correctly, this is no case in which we should have this function.
Comment 35 Eric Faust [:efaust] 2014-12-12 14:13:46 PST
Comment on attachment 8535611 [details] [diff] [review]
v1 - Some tests

Review of attachment 8535611 [details] [diff] [review]:
-----------------------------------------------------------------

When we're ready, we should add a test for proxies on the prototype chain of native objects in for-in interators. As discussed, that can live in the followup, though.
Comment 36 Tom Schuster [:evilpie] 2014-12-12 15:35:51 PST
Created attachment 8535900 [details] [diff] [review]
WIP - Remove getEnumerablePropertyKeys
Comment 37 Tom Schuster [:evilpie] 2014-12-13 08:02:06 PST
Created attachment 8536022 [details] [diff] [review]
v2 - Change from iterate to [[enumerate]]

Sorry Bobby, but you answer is still don't grasp your answer. I switched to BaseProxyHandler::enumerate now. Can you please explicitly say what you want me to do, thanks.
Comment 38 Tom Schuster [:evilpie] 2014-12-13 08:03:48 PST
Created attachment 8536023 [details] [diff] [review]
v1 - Remove getEnumerablePropertyKeys

I am not sure if it appropriate to call js::GetPropertyKeys in the BaseProxyHandler?
Comment 39 Tom Schuster [:evilpie] 2014-12-13 08:05:49 PST
Created attachment 8536024 [details] [diff] [review]
v1 - Fix for enumerating enumerable symbols

I believe this was broken before already. We should also call ownkeys when we want symbols, because getOwnEnumerablePropertyKeys doesn't return them.
Could be a followup if you want.
Comment 40 Eric Faust [:efaust] 2014-12-15 13:38:13 PST
Comment on attachment 8536022 [details] [diff] [review]
v2 - Change from iterate to [[enumerate]]

Review of attachment 8536022 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsiter.cpp
@@ +670,5 @@
>      }
>  
> +    // We should only call the enumerate trap for "for-in".
> +    // Or when we call GetIterator from the Proxy [[Enumerate]] hook.
> +    // In the future also for Reflect.enumerate.

Can we add a "sleeper-test" a la Jorendorff for this, where we test if Reflect.enumerate is a function, throw if it is, and leave the test we want to replace it with commented out in the file?

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +801,5 @@
> +}
> +
> +// Non-standard, try to convert iterator result to ids
> +bool
> +ScriptedDirectProxyHandler::getEnumerablePropertyKeys(JSContext *cx, HandleObject proxy, AutoIdVector &props) const

Complained about previously. There's now a patch to remove this later in this stack. OK by me :)
Comment 41 Bobby Holley (:bholley) (busy with Stylo) 2014-12-15 13:42:40 PST
Comment on attachment 8536022 [details] [diff] [review]
v2 - Change from iterate to [[enumerate]]

Review of attachment 8536022 [details] [diff] [review]:
-----------------------------------------------------------------

My suggestion was to just get rid of the override in Sandbox altogether, but this is also fine.
Comment 42 Eric Faust [:efaust] 2014-12-15 13:48:44 PST
Comment on attachment 8536023 [details] [diff] [review]
v1 - Remove getEnumerablePropertyKeys

Review of attachment 8536023 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Thanks for taking care of the inherited scripted proxy case :)

::: js/src/jsiter.cpp
@@ +327,5 @@
>  
>          if (flags & JSITER_OWNONLY)
>              break;
>  
> +        if (!JSObject::getProto(cx, pobj, &pobj))

OK, this site is prepared for getPrototypeOf to get called on scripted direct proxy handlers, right? Even if they change objects lower on the prototype chain, the algorithm is very clear. If it creates a circular prototype chain, then this spins, but that's unavoidable.

::: js/src/proxy/BaseProxyHandler.cpp
@@ +239,2 @@
>      AutoIdVector props(cx);
> +    if (!GetPropertyKeys(cx, proxy, 0, &props))

I am really happy that you shared code here, rather than writing another re-implementation.

::: js/src/proxy/Proxy.cpp
@@ +378,5 @@
> +
> +    AutoIdVector protoProps(cx);
> +    return GetPropertyKeys(cx, proto, 0, &protoProps) &&
> +           AppendUnique(cx, props, protoProps) &&
> +           EnumeratedIdVectorToIterator(cx, proxy, 0, props, objp);

Nice.
Comment 43 Eric Faust [:efaust] 2014-12-15 13:56:25 PST
Comment on attachment 8536024 [details] [diff] [review]
v1 - Fix for enumerating enumerable symbols

Review of attachment 8536024 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: js/src/jsiter.cpp
@@ +319,5 @@
> +
> +                    // We need to filter, if the caller just wants enumerable
> +                    // symbols.
> +                    if (!(flags & JSITER_HIDDEN)) {
> +                        if (!Proxy::getOwnPropertyDescriptor(cx, pobj, proxyProps[n], &desc))

This matches spec observability of *2* getOwnPropertyDescriptor calls per returned OwnPropertyKey. We should have some notion of how many times the trap is called in the above or a separate test.
Comment 44 Bobby Holley (:bholley) (busy with Stylo) 2014-12-15 20:57:46 PST
Comment on attachment 8536023 [details] [diff] [review]
v1 - Remove getEnumerablePropertyKeys

Review of attachment 8536023 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the XPConnect bits.
Comment 47 Tom Schuster [:evilpie] 2014-12-17 04:51:28 PST
I had to fix tests/js1_8_5/regress/regress-620376-{1,2}.js, because these tests use Proxy.create and wanted "enumerate" to be called, however we now always call "keys".
Comment 49 Florian Scholz [:fscholz] (MDN) 2015-01-07 12:57:18 PST
Thanks!

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