Use @@iterator symbol instead of placeholder string

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: wingo, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla36
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS] {done: true})

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
Once symbols are implemented, the @@iterator key currently being implemented by a hard-to-type string should change to be a symbol.

Specific places can be found by grepping the source tree for @@iterator and also for this bug number.  Don't forget dom/bindings/Codegen.py!
(Assignee)

Updated

3 years ago
Assignee: general → jorendorff
(Assignee)

Comment 1

3 years ago
Created attachment 8437151 [details] [diff] [review]
WIP 1

Work in progress. This breaks an xpcshell test somewhere (I forget which) but I think it should be straightforward to debug.

This works by adding a JSOP_SYMBOL bytecode to push Symbol.iterator to the operand stack. The method call at the top of a for-of look now looks like this:

    symbol 0   ;; obj Symbol.iterator
    callelem   ;; obj obj[Symbol.iterator]
    swap       ;; obj[Symbol.iterator] obj
    call 0     ;; iterator

WIP 1 implements JSOP_SYMBOL in the interpreter only. I'll also patch Baseline and Ion in this bug, so we don't regress for-of.

I'm not 100% sure of the approach though. It'll be fine in the interpreter and in Ion. I'm ignorant about our Baseline GetElem/CallElem IC. Need to read the code.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]

Updated

3 years ago
Duplicate of this bug: 795885

Comment 3

3 years ago
Would be nice to have this fixed for Moz33, i.e. on the same release as symbols :)
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 4

3 years ago
My plan here is to have a JSOP_SYMBOL instruction that just pushes a well-known symbol, so that for (x of y) will do something like

    getlocal y
    symbol 0 /*==SymbolCode::iterator*/
    callelem
    call 0
    ...

Filed bug 1038859 to make that JSOP_CALLELEM fast.
(Assignee)

Comment 5

3 years ago
Created attachment 8459263 [details] [diff] [review]
bug-918828-part-1-jsapi-v1.patch

Part 1 of 2: Switch the whole JSAPI so that "@@iterator" means the well-known symbol and not the string "@@iterator".

An alternative would be: change only the APIs that involve JSPropertySpec/JSFunctionSpec, and have callers use a special macro (JS_FS_SYMBOL or whatever) rather than magically recognizing strings starting with "@@".

I'm happy with either one. Your call.
Attachment #8437151 - Attachment is obsolete: true
Attachment #8459263 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

3 years ago
Created attachment 8459264 [details] [diff] [review]
bug-918828-part-2-iteration-v1.patch

Part 2: Change all iteration (for-of loop and everything else) to use the well-known symbol rather than "@@iterator".

Part 1 changes the JSAPI, which takes care of many *providers* of iterator methods; part 2 changes the remaining @@iterator method providers (the ones written in JS) and all consumers of iterator methods.

(Part 1 alone would break stuff that is fixed by part 2, so the two parts need to land together. In fact I'll squash them into a single changeset when I land.)
Attachment #8459264 - Flags: review?(nicolas.b.pierron)
So with these patches, JS_GetProperty and JS_GetPropertyById (with the id produced by manually interning a string) will have different behavior if the string happens to start with "@@", right?

That seems like a pretty backwards-incompatible change.  The fact that the binding InitIds() needed to be changed similarly is worrysome: right now I'm pretty sure the strings coming through there have to be IDL identifiers, but people keep asking to enlarge the value space there to all string property names.

As an API consumer, I'd vastly prefer changing on JSProperty/FunctionSpec with explicit "use a symbol" bits, plus ways to get the jsid for a symbol than I can then use as needed.
Comment on attachment 8459264 [details] [diff] [review]
bug-918828-part-2-iteration-v1.patch

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4436,5 @@
>      if (Emit1(cx, bce, JSOP_DUP) < 0)                          // OBJ OBJ
>          return false;
> +    if (Emit2(cx, bce, JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator)) < 0) // OBJ OBJ @@ITERATOR
> +        return false;
> +    if (!EmitElemOpBase(cx, bce, JSOP_CALLELEM))               // FN OBJ

I am not sure to understand the comment about the stack layout. I would expect:

  OBJ                    JSOP_DUP
  OBJ OBJ                JSOP_SYMBOL
  OBJ OBJ @@ITERATOR     JSOP_CALLELEM
> OBJ FN <               JSOP_CALL

Knowing that JSOP_CALLELEM pops the 2 first and push back its result on the stack.  Why should we have > FN OBJ < instead?

::: js/src/jsapi.cpp
@@ +2161,5 @@
>  
>      JSAtom *atom;
>      if (name[0] != '@' || name[1] != '@') {
>          atom = Atomize(cx, name, strlen(name));
>      } else if (strcmp(name, "@@iterator") == 0) {

(CharsToId) Isn't this case going to be deprecated, should we output a warning and open a bug for removing this case?

@@ +5711,5 @@
>      return symbol->code();
>  }
>  
>  JS_PUBLIC_API(JS::Symbol *)
>  JS::GetWellKnownSymbol(JSContext *cx, JS::SymbolCode which)

note: This cx can be a ThreadSafeContext.
(Assignee)

Comment 9

3 years ago
Comment on attachment 8459263 [details] [diff] [review]
bug-918828-part-1-jsapi-v1.patch

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

Retracting this. bz is right, it's the wrong approach for sure.
Attachment #8459263 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

3 years ago
Attachment #8459264 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 10

3 years ago
Created attachment 8486771 [details] [diff] [review]
bug-918828-part-1-jsapi-v2.patch
Attachment #8459263 - Attachment is obsolete: true
Attachment #8486771 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 11

3 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +4436,5 @@
> >      if (Emit1(cx, bce, JSOP_DUP) < 0)                          // OBJ OBJ
> >          return false;
> > +    if (Emit2(cx, bce, JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator)) < 0) // OBJ OBJ @@ITERATOR
> > +        return false;
> > +    if (!EmitElemOpBase(cx, bce, JSOP_CALLELEM))               // FN OBJ
> 
> I am not sure to understand the comment about the stack layout. I would
> expect:
> 
>   OBJ                    JSOP_DUP
>   OBJ OBJ                JSOP_SYMBOL
>   OBJ OBJ @@ITERATOR     JSOP_CALLELEM
> > OBJ FN <               JSOP_CALL
> 
> Knowing that JSOP_CALLELEM pops the 2 first and push back its result on the
> stack.  Why should we have > FN OBJ < instead?

EmitElemOpBase emits two instructions: JSOP_CALLELEM followed by JSOP_SWAP.
(Assignee)

Comment 12

3 years ago
Created attachment 8486772 [details] [diff] [review]
bug-918828-part-2-iteration-v2.patch
Attachment #8459264 - Attachment is obsolete: true
Attachment #8486772 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 13

3 years ago
Created attachment 8486793 [details] [diff] [review]
bug-918828-part-3-decompiler-v2.patch
Attachment #8486793 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 14

3 years ago
Created attachment 8486794 [details] [diff] [review]
bug-918828-part-4-baseline-v2.patch
Attachment #8486794 - Flags: review?(nicolas.b.pierron)

Comment 15

3 years ago
Is this intended to be resolved in Fx 33?

Currently, Fx 33 Beta defines `Symbol.iterator`, but defining a method named [Symbol.iterator] on an object doesn't make it iterable, which is problematic. So, in case this bug is not resolved for version 33, `Symbol.iterator` should be removed.
Comment on attachment 8486772 [details] [diff] [review]
bug-918828-part-2-iteration-v2.patch

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

::: browser/components/customizableui/CustomizableUI.jsm
@@ -2663,3 @@
>     */
> -  windows: {
> -    "@@iterator": function*() {

nit: Any reason to not replace it by

  windows: {
    [Symbol.iterator]: function*() {
      …
    }
  },

?

@@ +3497,5 @@
>    },
>  };
> +
> +// Add CustomizableUI.windows[@@iterator] method
> +this.CustomizableUI.windows[Symbol.iterator] = function*() {

instead of moving it out-side?

::: js/src/vm/PIC.cpp
@@ +43,5 @@
>      // Shortcut returns below means Array for-of will never be optimizable,
>      // do set disabled_ now, and clear it later when we succeed.
>      disabled_ = true;
>  
> +    // Look up Array.prototype[@@iterator], ensure it's a slotful shape.

nit: Replace @@iterator by Symbol.iterator in vm/PIC.{cpp,h}
Attachment #8486772 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8486794 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 17

3 years ago
(In reply to Claude Pache from comment #15)
> Is this intended to be resolved in Fx 33?

No. (But read on.)

> Currently, Fx 33 Beta defines `Symbol.iterator`, but defining a method named
> [Symbol.iterator] on an object doesn't make it iterable, which is
> problematic. So, in case this bug is not resolved for version 33,
> `Symbol.iterator` should be removed.

Agreed. We've already disabled symbols in the mozilla-beta repository (see bug 1041631). If they're enabled in the current FF33 beta, that just means we shipped it before that patch landed; the next beta drop will be right (building now locally, to make totally sure this is the case).
(Assignee)

Comment 18

3 years ago
> (building now locally, to make totally sure this is the case).

Confirmed.
(Assignee)

Comment 19

3 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #16)
> [...] instead of moving it out-side?

Much better! I wrote the patch before computed property names and method syntax landed. With the suggested change, the change is a nice improvement:

   windows: {
-    "@@iterator": function*() {
+    *[Symbol.iterator]() {
       ...
     }
   },

> nit: Replace @@iterator by Symbol.iterator in vm/PIC.{cpp,h}

I think @@iterator is still a little better. That's how the ES6 specification refers to this symbol.
Comment on attachment 8486771 [details] [diff] [review]
bug-918828-part-1-jsapi-v2.patch

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

::: js/src/jsapi.cpp
@@ +3020,5 @@
>      RootedAtom getterNameAtom(cx, Atomize(cx, getterName, strlen(getterName)));
>      if (!getterNameAtom)
>          return false;
>  
> +    RootedAtom name(cx, IdToFunctionName(cx, id));

Seems like MOZ_ASSERT(!JSID_IS_SYMBOL(id), "self-hosted property names must not be symbols") is reasonable here.  I don't see a good reason to allow self-hosted names to be symbols, and much clarity reason to forbid it.

@@ +4118,5 @@
>  
>              RootedAtom shName(cx, Atomize(cx, fs->selfHostedName, strlen(fs->selfHostedName)));
>              if (!shName)
>                  return false;
> +            RootedAtom name(cx, IdToFunctionName(cx, id));

Same MOZ_ASSERT(!JSID_IS_SYMBOL(id)) here.

::: js/src/jsapi.h
@@ +4465,5 @@
> +/*
> + * Return true if the given JSPropertySpec::name or JSFunctionSpec::name value
> + * is actually a symbol code and not a string. See JS_SYM_FN.
> + */
> +inline bool PropertySpecNameIsSymbol(const char *name) {

inline bool
PropertySpecNameIsSymbol(const char *name)
{

::: js/src/jsfun.cpp
@@ +2072,5 @@
> + * Function names are always strings. If id is the well-known @@iterator
> + * symbol, this returns "[Symbol.iterator]".
> + */
> +JSAtom *
> +js::IdToFunctionName(JSContext *cx, HandleId id)

This method kind of shouldn't exist, right?  Looks like

  var obj = { [Symbol.iterator]() { } };

produces an obj[Symbol.iterator].name === Symbol.iterator.  But in the short run we can't do that easily, so this deviation is acceptable as a temporary matter.

::: js/src/jsfun.h
@@ +488,5 @@
>          return kind;
>      }
>  };
>  
> +JSString *

I get that |extern| is the default and all, but why remove it?  EIBTI, seems to me.  I'd rather we consistently used it in headers and prohibited it outside of them, myself.
Attachment #8486771 - Flags: review?(jwalden+bmo) → review+
Attachment #8486793 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 21

3 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> > +    RootedAtom name(cx, IdToFunctionName(cx, id));
> 
> Seems like MOZ_ASSERT(!JSID_IS_SYMBOL(id), "self-hosted property names must
> not be symbols") is reasonable here.  I don't see a good reason to allow
> self-hosted names to be symbols, and much clarity reason to forbid it.

I'm a little wary of this assertion because if I added exactly the same assertion in JS_DefineFunctions where it deals with self-hosted functions, it would fail. Symbol-keyed methods are the point of the patch, after all.

The IdToFunctionName stuff below is related to my thinking here.

> ::: js/src/jsapi.h
> > +inline bool PropertySpecNameIsSymbol(const char *name) {

yup, fixed

> ::: js/src/jsfun.cpp
> > +js::IdToFunctionName(JSContext *cx, HandleId id)
> 
> This method kind of shouldn't exist, right?

No, I think it has to exist; it implements step 4 of the SetFunctionName algorithm:
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-setfunctionname

I've added a comment to that effect (and fixed the silly bug where I was appending "[Symbol." even though the algorithm clearly only calls for "[").

> Looks like
> 
>   var obj = { [Symbol.iterator]() { } };
> 
> produces an obj[Symbol.iterator].name === Symbol.iterator.

I get this:

    js> var obj = { [Symbol.iterator]() { } };
    js> obj[Symbol.iterator].name === Symbol.iterator
    false
    js> obj[Symbol.iterator].name
    ""

But that's a bug, bug 1054599.

The method's .name should be the string "[Symbol.iterator]". See:
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-setfunctionname
which in this code is called from:
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-object-initializer-runtime-semantics-propertydefinitionevaluation

> ::: js/src/jsfun.h
> I get that |extern| is the default and all, but why remove it?  EIBTI, seems
> to me.  I'd rather we consistently used it in headers and prohibited it
> outside of them, myself.

Reverted for now.

There is no prevailing style, unfortunately. Trying to fix that on IRC...
(Assignee)

Comment 22

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3695c1c812a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4faff84eb1ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/621224c58e71
https://hg.mozilla.org/integration/mozilla-inbound/rev/b87ca498ea9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/12e7deed1b17
(Assignee)

Comment 23

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b59c02df792f
Backed out for mass bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb579e3de64b
(Assignee)

Comment 25

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=49f4fbaf6cca
(Assignee)

Comment 26

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=2bc7dc5ed1ca
(Assignee)

Comment 27

3 years ago
Two issues here.

1. The damned thing just didn't work. Lots of assertions in various test suites. I've fixed two rather straightforward bugs so far. Building and testing again now.

2. Jeff reminded me that even though I will be trying to enable symbols in the current release cycle, the patches in this bug should still support building without JS_HAS_SYMBOLS -- just in case they have to be disabled again. So that means sprinkling a few #ifdefs here and there, forking the XDR version number, and (which is 80% of the work) revisiting all the tests. All that is done, but will need fresh reviews.
(Assignee)

Comment 28

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=73feae1a53b9
(Assignee)

Updated

3 years ago
Blocks: 1066322
(Assignee)

Comment 29

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=60e5f7098f72
(Assignee)

Comment 30

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=fea2dbddd8d3
(Assignee)

Updated

3 years ago
Depends on: 1071177
(Assignee)

Comment 31

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6a365b8e6883
(Assignee)

Comment 32

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c4b9181ade33
(Assignee)

Comment 33

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=8acb89ebc0c8
(Assignee)

Comment 34

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=00003f1388f0
(Assignee)

Comment 35

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d2fe26f27674
(Assignee)

Comment 36

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b5f7d3be7b23
(Assignee)

Comment 37

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=815a4622abc8
(Assignee)

Comment 38

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c3c51c66541e
(Assignee)

Comment 39

3 years ago
I think I have found the issue here that caused all the failures I couldn't reproduce locally. I have added the following four-character fix to the first patch in the stack. We'll see if this fixes it.

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -3248,15 +3248,15 @@ JS_DefineConstIntegers(JSContext *cx, Ha
> static bool
> PropertySpecNameToId(JSContext *cx, const char *name, MutableHandleId id,
>                      js::InternBehavior ib = js::DoNotInternAtom)
> {
>     if (JS::PropertySpecNameIsSymbol(name)) {
>         uintptr_t u = reinterpret_cast<uintptr_t>(name);
>         id.set(SYMBOL_TO_JSID(cx->wellKnownSymbols().get(u - 1)));
>     } else {
>-        JSAtom *atom = Atomize(cx, name, strlen(name));
>+        JSAtom *atom = Atomize(cx, name, strlen(name), ib);
>         if (!atom)
>             return false;
>         id.set(AtomToId(atom));
>     }
>     return true;
> }

Coincidentally, when I found this, I made a number of to-the-point four-character remarks on the issue, which I won't reproduce here.
(Assignee)

Comment 40

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=8c3a57f2d8f8
(Assignee)

Comment 41

3 years ago
Created attachment 8504930 [details] [diff] [review]
part 1 - Change iteration code to call iterable[Symbol.iterator]() rather than iterable["@@iterator"]().  try: -b d -p linux64,linux64-st-an,macosx64 -u all[10.8,x64] -t none
Attachment #8504930 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

3 years ago
Attachment #8486771 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8486772 - Attachment is obsolete: true
(Assignee)

Comment 42

3 years ago
Functionally this is the same patch as the now-obsolete "part 2" (which nbp reviewed earlier).

The changes to tests here are significant enough to warrant a new review.
(Assignee)

Comment 43

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=afde8316cd27
(Assignee)

Comment 44

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=39f94ca0a97a
Blocks: 1083470
(Assignee)

Comment 45

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d3ea180fe0
(Assignee)

Comment 46

3 years ago
That changeset is just a test or two. The actual work here still has not landed.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/c0d3ea180fe0
Comment on attachment 8504930 [details] [diff] [review]
part 1 - Change iteration code to call iterable[Symbol.iterator]() rather than iterable["@@iterator"]().  try: -b d -p linux64,linux64-st-an,macosx64 -u all[10.8,x64] -t none

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

The bytecode emitter changes look seriously wrong, but I imagine that's been fixt in subsequent try-pushes and you just haven't posted a new patch/r?.

::: addon-sdk/source/lib/sdk/util/iteration.js
@@ +2,1 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this

The addon SDK is maintained in a github repository, so this needs to be upstreamed to them.  Per KWierso, "would be nice to file a dependency of your patch's bug in the add-on sdk product to sync up the changes to github."  CC peoples, get the ball rolling.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4667,5 @@
>          return false;
> +#ifdef JS_HAS_SYMBOLS
> +    if (Emit2(cx, bce, JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator)) < 0) // OBJ OBJ @@ITERATOR
> +        return false;
> +    if (!EmitElemOpBase(cx, bce, JSOP_CALLELEM))               // FN OBJ

I understand OBJ OBJ @@ITERATOR.  But doesn't JSOP_CALLELEM pop [obj, val] and push obj[val]?  So how is this not OBJ ITERFN (which seems like a better name to use in the !JS_HAS_SYMBOLS case than @@ITERATOR, now that @@iterator is an actual thing)?  And wouldn't we then need the same fall-through-to-swap that happens in the other arm?

::: js/src/tests/ecma_6/Array/from_errors.js
@@ -139,1 @@
>              }

...so this method was returning |undefined| before, as this bracing triggered a labeled arrow function that was itself useless?  But that happened to cause the same observable behavior as returning an objects whose .next property when called returned a primitive?  Sheesh.

::: js/src/vm/PIC.cpp
@@ +20,5 @@
>  
> +#ifdef JS_HAS_SYMBOLS
> +#define STD_ITERATOR_ID  SYMBOL_TO_JSID(cx->wellKnownSymbols().iterator)
> +#else
> +#define STD_ITERATOR_ID  (cx->names().std_iterator)

A little weird that STD_ITERATOR_ID has different *types* in the two cases, but if it works...

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +505,1 @@
>       "set", "copyWithin"];

Guh.  I think this is copypasta I used when writing this method, to ensure the rest of it was correct/sensible wrt those properties, that I forgot to remove before landing.  Please remove this.
Attachment #8504930 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 49

3 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #48)
> ::: addon-sdk/source/lib/sdk/util/iteration.js
> @@ +2,1 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> 
> The addon SDK is maintained in a github repository, so this needs to be
> upstreamed to them.  Per KWierso, "would be nice to file a dependency of
> your patch's bug in the add-on sdk product to sync up the changes to
> github."  CC peoples, get the ball rolling.

Thanks for noticing this! Will file one.

> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +4667,5 @@
> >          return false;
> > +#ifdef JS_HAS_SYMBOLS
> > +    if (Emit2(cx, bce, JSOP_SYMBOL, jsbytecode(JS::SymbolCode::iterator)) < 0) // OBJ OBJ @@ITERATOR
> > +        return false;
> > +    if (!EmitElemOpBase(cx, bce, JSOP_CALLELEM))               // FN OBJ
> 
> I understand OBJ OBJ @@ITERATOR.  But doesn't JSOP_CALLELEM pop [obj, val]
> and push obj[val]?  So how is this not OBJ ITERFN (which seems like a better
> name to use in the !JS_HAS_SYMBOLS case than @@ITERATOR, now that @@iterator
> is an actual thing)?  And wouldn't we then need the same
> fall-through-to-swap that happens in the other arm?

No, because EmitElemOpBase emits a JSOP_SWAP.

Assuming you find that horrible, you're not the first. I've filed (and taken)
follow-up bug 1089758 to make it clearer what's going on here.

> ::: js/src/tests/ecma_6/Array/from_errors.js
> ...so this method was returning |undefined| before, as this bracing
> triggered a labeled arrow function that was itself useless?  But that
> happened to cause the same observable behavior as returning an objects whose
> .next property when called returned a primitive?  Sheesh.

Yeah, it's my fault, I wrote that code. Horrible^2.

> ::: js/src/vm/PIC.cpp
> > +#ifdef JS_HAS_SYMBOLS
> > +#define STD_ITERATOR_ID  SYMBOL_TO_JSID(cx->wellKnownSymbols().iterator)
> > +#else
> > +#define STD_ITERATOR_ID  (cx->names().std_iterator)
> 
> A little weird that STD_ITERATOR_ID has different *types* in the two cases,
> but if it works...

I don't feel great about this; I'll try it with

    #define STD_ITERATOR_ID  jsid(cx->names().std_iterator)

which should be just as good and makes the two expressions the same type
(though you never know with C++ --- much closer, anyway).

> Guh.  I think this is copypasta I used when writing this method, to ensure
> the rest of it was correct/sensible wrt those properties, that I forgot to
> remove before landing.  Please remove this.

Done.
(Assignee)

Comment 50

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c8bebcc9453d
(Assignee)

Comment 51

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ffa7987cc72b
(Assignee)

Comment 52

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c2b76d2263aa
(Assignee)

Comment 53

3 years ago
NameToId(), not jsid(), is the right thing; that's what NativeObject::lookup(cx, name) uses for the conversion.
(Assignee)

Comment 54

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=99788b83321d
(Assignee)

Comment 55

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=3922f29dc280
(Assignee)

Comment 56

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/51a1fa4c521f
https://hg.mozilla.org/integration/mozilla-inbound/rev/606dc149145e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee7bea7db458
(Assignee)

Comment 57

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/418f66cde83d
(Assignee)

Comment 58

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd2cf3f640ed
(Assignee)

Updated

3 years ago
Whiteboard: [DocArea=JS] → [DocArea=JS] {done: true}
https://hg.mozilla.org/mozilla-central/rev/51a1fa4c521f
https://hg.mozilla.org/mozilla-central/rev/606dc149145e
https://hg.mozilla.org/mozilla-central/rev/ee7bea7db458
https://hg.mozilla.org/mozilla-central/rev/418f66cde83d
https://hg.mozilla.org/mozilla-central/rev/bd2cf3f640ed
> '@@iterator' in StyleSheetList.prototype; // Firefox 31
> '@@iterator' in CSSRuleList.prototype; // Firefox 32

Now both returns false. It that correct?
Yes.  Symbol.iterator in StyleSheetList.prototype returns true now.
Why is this bug still open? Are there any remaining works?
Oh, I see. Then this may need a site compat doc.
Keywords: site-compat
Depends on: 1092625
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)

Comment 76

3 years ago
Was this change announced anywhere? It broke a bunch of Thunderbird code I was working on, and it took me a while to figure out what changed. If I just missed the announcement, I want to be sure I subscribe to the appropriate group so I don't have have any unpleasant surprises like this in the future.
This bug apparently broke most of the add-on SDK. Can we consider backing it out until we can get things fixed?
Blocks: 1092231
Flags: needinfo?(jorendorff)

Updated

3 years ago
Keywords: addon-compat
(Assignee)

Comment 78

3 years ago
(In reply to Jim Porter (:squib) from comment #76)
> Was this change announced anywhere?

No. I apologize for that. Announcements go to dev.platform. I just blew it.
Flags: needinfo?(jorendorff)
You didn't answer comment 77.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 80

3 years ago
(In reply to Dave Townsend [:mossop] from comment #77)
> This bug apparently broke most of the add-on SDK. Can we consider backing it
> out until we can get things fixed?

Sorry for the rough landing. Some of this could have been avoided with a few announcements and some coordination.

I've been talking about this with :zombie, and the bottom line is:

<zombie>  jorendorff: so since JP tests are hidden, we are in bad shape.. will try to see
          if we can fix in next ~12 hours, and if not, ask you to backout.. sounds reasonable?

I recognize it's not good for jetpack tests to be failing for this long, and it wasn't my intent to spring the changes on y'all like this. :( I hope the fixes will be fairly straightforward, and I'm happy to help out. See you in #jetpack...
(Assignee)

Updated

3 years ago
Flags: needinfo?(jorendorff)
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
I think the [leave-open] keyword prevented the script to set the release version. I've removed the keyword and manually set the release to Mozilla 36.

If somebody could double check, it would be nice.
Keywords: leave-open
Target Milestone: --- → mozilla36
Updated following documents:
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/@@iterator
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/prototype
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla

Following documents are already updated:
  https://developer.mozilla.org/en-US/Firefox/Releases/36
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/iterable
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/@@iterator
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/@@iterator
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/@@iterator
Excellent. Thanks for the doc additions!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.