Cc-ing some randomly chosen DOM and WebIDL people in hopes of getting free advice. The for-of loop (added in bug 699565) is currently fairly magical. The iterator objects are not exposed to JS code. It is impossible for plain old Objects to be for-of-iterable; the only way for a user-defined object to be iterable is if it's a Proxy. I've talked this over very briefly with dherman and samth, and I think we can make this simpler and at the same time a lot more user-friendly. I propose that the for-of loop: for (V of EXPR) STMT work like this: { let %iter = (EXPR).iterator(); for (;;) { let %val; try { %val = %iter.next(); } catch (exc) { if (exc instanceof StopIteration) break; else throw exc; } V = %val; STMT } } where %val and %iter are different from all other identifiers. With this change, it's easy to make an object iterable. Give it an iterator() method. We would define Array.prototype.iterator and so on--and also for the DOM.

Less magic sounds better to me. We could easily have Web IDL require all objects with indexed properties (like NodeList) to have an iterator() function on their prototype. Sounds like a no-brainer in terms of usefulness. (On Window too? Maybe...)

Implementation notes. JSOP_ITER will change to call .iterator(). The iternext/itermore opcodes already call .next as a fallback; it'll just become the normal behavior in the for-of case. This will be slow, for now. Type inference could help a lot. Array.prototype.iterator will be added. It'll return an Iterator object with proto Iterator.prototype. The native function Iterator.prototype.next will be changed to be slightly generic, so that it'll work with Array iterators, generator iterators, and other types (like Map and Set iterators; see bug 725909), though not with arbitrary objects. Proxy::iteratorNext and ProxyHandler::iteratorNext go away; next is an ordinary method call now.

This patch doesn't affect user-visible behavior. Just a little C++ refactoring and moving stuff around. I considered calling this "Enumerator" but PropertyIteratorObject is precisely what it is. Note that this kind of object is sometimes exposed to script (if you call Iterator({a:1, b:2}) you get a property iterator object; ES6 will probably change the API but still expose such iterators somehow) and sometimes not (a for-in loop makes an iterator of the same Class, but it lives in a stack slot, has getProto() == NULL, and doesn't escape).

I just realized there's a pretty big bug in part 2 of this work. I will have to come back to it in 2 weeks since all next week is dedicated to debugger.

Just for fun. The bug is that I didn't add an iterator method to the DOM classes that need one. bz already pointed out how to do it, I just forgot, and haven't submitted this stack to the try server yet.

This is the main part, though it's not much bigger than part 1 or part 2... The following changes are happening in this patch: - Up to now, for-of iteration was achieved by js_IteratorNext having magical knowledge of certain kinds of iterator object. With this patch, those special cases are removed and js_IteratorNext just calls iter.next(). Simple sane semantics. Well, except for PropertyIteratorObjects, which are still special-cased. But that is strictly for speed (for-in is still the common case here and the only thing that shows up on benchmarks). - All iterators inherit from Iterator.prototype, which already has a .next() method. This method just calls js_IteratorNext. But now that would be circular, so I changed it to call a class hook: iterobj->getClass()->ext.iteratorNext If you try to do Iterator.prototype.next.call(obj) on an object that has a NULL iteratorNext class hook, it'll throw a TypeError at you. Not that you ever would. - PropertyIteratorObjects and ElementIteratorObjects each have an iteratorNext hook that gets the next property/element. - Wrapper::iteratorNext was really buggy. The replacement does the obvious thing and just delegates to the wrapped object's iteratorNext class hook. This also adds a "next" trap for scripted proxies. Some of these design decisions are pretty subtle. Things may change later as ES6 coalesces. This is a snapshot of the current design. Certainly having for-of treat everything as a plain old object and call plain old .iterator() and .next() methods is a simplicity win for users; I don't expect TC39 to back away from that. This will also help me make Maps and Sets iterable, which I'm eager to finish (bug 725909).

Comment on attachment 617074 [details] [diff] [review] Part 1 - minor C++ refactoring, rename Iterator to PropertyIteratorObject Review of attachment 617074 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsiter.cpp @@ +486,3 @@ > } > > + return (PropertyIteratorObject *) NewBuiltinClassInstance(cx, &PropertyIteratorObject::class_); Use &asPropertyIterator(), please, like above. ::: js/src/jsiter.h @@ +100,5 @@ > > void mark(JSTracer *trc); > }; > > +class PropertyIteratorObject : public JSObject { { on new line per style guide. ::: js/src/jsobjinlines.h @@ +49,5 @@ > #include "jscntxt.h" > #include "jsdate.h" > #include "jsfun.h" > #include "jsgcmark.h" > +#include "jsinterp.h" Why this addition?

I need to add a few lines of code here to finish this bug: https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dombindingsgen.py?rev=7082192622e6#717

Unbitrotted, carrying forward review.

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #9) > Use &asPropertyIterator(), please, like above. Good idea. Done. > > +class PropertyIteratorObject : public JSObject { > { on new line per style guide. OK. > > +#include "jsinterp.h" > Why this addition? I dunno. It works fine without it, so I removed it again.

What comment 10 said still applies to this patch. It needs more work (and tests) in order not to break for-of on arraylike DOM objects.

Unbitrotted, but this is the wrong approach and I'm going to reimplement it almost from scratch.

Unbitrotted.

Same as v1 actually, I think.

Carrying r+ forward again.

Things to look out for: - Are there any arraylike DOM objects that don't get the new bindings, or don't have an IndexGetter? - Are there any other DOM objects that should be iterable? - Are there cases where one prototype object for an iterable interface inherits from another, so that we're pointlessly shadowing that .iterator method with another .iterator?

Jim's not around, but that's OK. This part can wait a week or three.

dev-doc-needed: Note that what it currently says here is already wrong: https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of It says that for-of loops get property values. That isn't right. 'for (x in obj)' iterates over the names of obj's properties. But 'for (x of obj)' will call obj.iterator() and loop over the values produced by that iterator's .next() method. This is almost exactly how Python's for-in loop works, only instead of a .iterator() method, Python has a .__iter__() method. When this bug lands, users will be able to write their own iterators in order to make the for-of loop do whatever they want. For example: var obj = {iterator: function () { yield 1; yield 2; yield 3; }}; for (var x of obj) print(x); This prints 1, then 2, then 3. Also, any object that inherits from Array.prototype is automatically iterable, just because Array.prototype has a .iterator method on it. var obj = Object.create(Array.prototype); obj.length = 3; obj[0] = 'A'; obj[1] = 'B'; obj[2] = 'C'; for (var x of obj) print(x); This prints A, then B, then C.

Comment on attachment 633647 [details] [diff] [review] Part 2 - Make for-of loops just call .iterator(), v3 Review of attachment 633647 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsatom.tbl @@ +53,5 @@ > DEFINE_ATOM(index, "index") > DEFINE_ATOM(input, "input") > DEFINE_ATOM(toISOString, "toISOString") > +DEFINE_ATOM(iterator, "iterator") > +DEFINE_ATOM(iterator_, "__iterator__") I think that iterator_ should have a more verbose/ugly name to distinguish it from the 'iterator' proper, maybe iteratorIntrinsic?

Comment on attachment 633667 [details] [diff] [review] Part 3 - Add .next() method to iterator objects and make for-of call it, v3 Review of attachment 633667 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsiter.cpp @@ +841,5 @@ > + GlobalObject *global = GetCurrentGlobal(cx); > + RootedObject proto(cx, global->getOrCreateElementIteratorPrototype(cx)); > + if (!proto) > + return NULL; > + RootedObject iterobj(cx, NewObjectWithGivenProto(cx, &class_, proto, global)); The root on iterobj itself isn't necessary, as setReservedSlot cannot trigger a GC. @@ +872,5 @@ > + obj = ValueToObject(cx, target); > + if (!obj) > + return false; > + if (!js_GetLengthProperty(cx, obj, &length)) > + goto error; The semantics here look weird and this at least needs a comment. What are the conditions when it is OK to just propagate failures vs. necessary to close the iterator down? Also, the error label should be renamed to closeIterator or something. ::: js/src/jsiter.h @@ +97,5 @@ > + * to any other object to create iterators over that object's elements. > + * > + * - The next() method of an Array iterator is non-reentrant. Trying to reenter, > + * e.g. by using it on an object with a length getter that calls it.next() on > + * the same iterator, causes a TypeError. How do we ensure that next() is non-reentrant? ElementIteratorObject::next doesn't seem to do any guarding. Either way, a comment in the implementation about this would be good.

(In reply to Brian Hackett (:bhackett) from comment #25) > The root on iterobj itself isn't necessary, as setReservedSlot cannot > trigger a GC. Removed. You found two bugs in ElementIteratorObject::next(): > The semantics here look weird and this at least needs a comment. [...] Bug 1. Easily fixed. I changed it so that any time we return false, we do it via 'goto close;'. > How do we ensure that next() is non-reentrant? Bug 2. This one's a bit more work; I'll fix (and test) in a separate patch. For the time being, re-entering will not cause anything horrible to happen, it'll just behave contrary to spec in some corner cases (and honestly there is no spec, the comment is sort of a guess). BTW, both bugs have the same cause: I turned the old iteratorNext() code into a script-visible .next() method without re-reading it closely. And I had, perhaps foolishly, optimized iteartorNext() with the knowledge that iterator objects were not script-visible. Oops. Thanks.

(In reply to Jason Orendorff [:jorendorff] from comment #26) > For the time being, re-entering will not cause anything horrible to happen, > it'll just behave contrary to spec in some corner cases (and honestly there > is no spec, the comment is sort of a guess). Hmm, if the spec doesn't insist on this, it might be better to just leave this as is and fix the later comment to not discuss reentrancy. The behavior looks fine to me in the reentrant case, some implementation details exposed for sure but oh well.

(In reply to Brian Hackett (:bhackett) from comment #27) > Hmm, if the spec doesn't insist on this, it might be better to just leave > this as is and fix the later comment to not discuss reentrancy. The > behavior looks fine to me in the reentrant case, some implementation details > exposed for sure but oh well. Yup. I agree and I went through and re-spec'd it that way http://wiki.ecmascript.org/doku.php?id=harmony:iterators#arrayiteratorprototype_.next and changed the comment.

Comment on attachment 633666 [details] [diff] [review] Part 2a - Implement .iterator() for arraylike DOM objects r=me. Sorry for the lag....

Landed the first four patches on m-i. Two to go. https://hg.mozilla.org/integration/mozilla-inbound/rev/3a488b71b69a https://hg.mozilla.org/integration/mozilla-inbound/rev/cb49c3730a97 https://hg.mozilla.org/integration/mozilla-inbound/rev/4313740f1adc https://hg.mozilla.org/integration/mozilla-inbound/rev/24feaa8bd894

https://hg.mozilla.org/mozilla-central/rev/3a488b71b69a https://hg.mozilla.org/mozilla-central/rev/cb49c3730a97 https://hg.mozilla.org/mozilla-central/rev/4313740f1adc https://hg.mozilla.org/mozilla-central/rev/24feaa8bd894

https://hg.mozilla.org/integration/mozilla-inbound/rev/f44eca03418c One more to go. Please keep open still.

Comment on attachment 633668 [details] [diff] [review] Part 4 - make findReferences accept string arguments and add some gc tests, v3 Review of attachment 633668 [details] [diff] [review]: ----------------------------------------------------------------- I think you'll also need to include strings in the array of roots maintained at the top of HeapReverser::traverseEdge. HeapReverser::nodeToValue needs to be fixed as well. I'm also surprised ReferenceFinder::representable didn't need to be changed. Until it is, I think findReferences will fail to notice ropes referring to their left and right halves.

Comment on attachment 633668 [details] [diff] [review] Part 4 - make findReferences accept string arguments and add some gc tests, v3 Review of attachment 633668 [details] [diff] [review]: ----------------------------------------------------------------- I think you'll also need to include strings in the array of roots maintained at the top of HeapReverser::traverseEdge. HeapReverser::nodeToValue needs to be fixed as well. I'm also surprised ReferenceFinder::representable didn't need to be changed. Until it is, I think findReferences will fail to notice ropes referring to their left and right halves. ::: js/src/jit-test/tests/for-of/string-iterator-gc.js @@ +1,4 @@ > +// String iterators keep the underlying string alive. > + > +load(libdir + "referencesVia.js"); > +var song = Array(17).join("na") + "batman!"; You've *got* use use the NaN trick here!!!

This bug, or something which was merged to mozilla-central in the same push, caused a permanent orange on Linux32 mochitest-chrome for builds without frame pointers (example log: <https://tbpl.mozilla.org/php/getParsedLog.php?id=13227357&tree=Profiling&full=1>). This is important because framepointers are not enabled on Aurora, so this permaorange will appear there on the next uplift. I backed out this patch as part of the rest of the js and xpconnect patches in the same merge push. Please test this patch locally (and push to the try server if needed by taking out the --enable-profiling command from the Linux32 mozconfig) and reland when you make sure that it's not the cause behind the perma-orange. I apologize in advance for the inconvenience in case this patch is not at fault. Backout changeset: https://hg.mozilla.org/mozilla-central/rev/b9c98f0d0fde https://hg.mozilla.org/mozilla-central/rev/0d2b03dff288 https://hg.mozilla.org/mozilla-central/rev/aadf6091245b https://hg.mozilla.org/mozilla-central/rev/86cf7f8a124a https://hg.mozilla.org/mozilla-central/rev/015cbcaf8552

In order to test whether this patch is at fault, push it to try with --enable-profiling removed from browser/config/mozconfigs/linux32/nightly, and if you get a green Linux Moth run, you're good to go! And in turn you can reproduce the crash on try with your patch, I'd suggest you keep the possibility of having hit a compiler bug in mind. :-)

Also, note that these backouts indeed made the Linux32 Moth runs on the profiling branch green again, so we can be fairly certain that one of the backed out patches was at fault.

I have been trying to land this for weeks now. The backouts in comment 36 were due to the craziness in bug 749010, but my latest Try Server runs also show some orange. https://tbpl.mozilla.org/?tree=Try&rev=db99191b1353 On OSX only, the browser doesn't start. No clue why, no info in the log. On my machine (also a mac), a debug browser build runs fine.

Try again?

https://hg.mozilla.org/mozilla-central/rev/18295d17cba2 https://hg.mozilla.org/mozilla-central/rev/7ecd5e911ba4 https://hg.mozilla.org/mozilla-central/rev/81b3c7b3a561 https://hg.mozilla.org/mozilla-central/rev/a59567902ad0

Can this bug be closed, then?

(In reply to Jim Blandy :jimb from comment #42) > Can this bug be closed, then?

Yes. This is done.

Updated following documents: https://developer.mozilla.org/en-US/Firefox/Releases/17 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of (updated compat table only, since the behavior is changed again) https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla