1104014 - Disable old-style generators in web content

Status: RESOLVED DUPLICATE of bug 1083482

(Core :: JavaScript Engine, defect)

Description

[Domenic Denicola]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141027150301

Steps to reproduce:

Try this test which was meant to detect nonstandard generator implementations:

   typeof StopIteration !== "undefined"



Actual results:

It returned true, since Firefox still exposes StopIteration on the global


Expected results:

It should return false, since StopIteration is no longer used and Firefox's generator implementation is now up to the standard.

Comment 1

[Domenic Denicola]

As discovered in https://github.com/kriskowal/q/issues/616#issuecomment-64163153, there is library code (notably the popular Q promise library) which uses this test to detect Firefox's old nonstandard generators. It is giving a false positive now, causing Q to go down the wrong code path and thus causing infinite loops with promise + generator code that presumably used to work in older Firefox versions.

Comment 2

[Till Schneidereit [:till]]

Hum, this is a problem. We still have to keep our non-standard generators around for now: there's content using them. Based on our telemetry[1] it's not that much, but it is there.

OTOH, Q might be used more, so that might cause more content to be broken than removing the old generators :(

Jorendorff, do you think we should go forward and disable old-style generators for web content? We could still keep them for chrome/addons code for now.

Comment 3

[Till Schneidereit [:till]]

Forgot the footnote link: http://telemetry.mozilla.org/#filter=beta%2F34%2FJS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT&aggregates=multiselect-all!Submissions&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph

Old-style generators are `2` in the enum, so it's not all that many hits, but also not none. (`3` is expression closure, btw :()

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to Till Schneidereit [:till] from comment #2)
> Jorendorff, do you think we should go forward and disable old-style
> generators for web content?

Yes.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Domenic Denicola from comment #1)
> As discovered in
> https://github.com/kriskowal/q/issues/616#issuecomment-64163153, there is
> library code (notably the popular Q promise library) which uses this test to
> detect Firefox's old nonstandard generators. It is giving a false positive
> now, causing Q to go down the wrong code path and thus causing infinite
> loops with promise + generator code that presumably used to work in older
> Firefox versions.

Is there anything we can do to help with this, at this point? We haven't removed the old nonstandard generators; in fact I can't think of anything that changed in their behavior when ES6 generators were added. So it's something of a mystery why the old code in Q would stop working.

Comment 6

[Domenic Denicola]

> We haven't removed the old nonstandard generators; in fact I can't think of anything that changed in their behavior when ES6 generators were added. So it's something of a mystery why the old code in Q would stop working.

Hmm, that is curious. I will investigate further. So how it works is that `function f() { ... yield ... }` is old-style with StopIteration, but `function* f() { ... yield ... }` is new-style?

Comment 7

[Kris Kowal]

The problem is that Q has not *started* working with new generators in Firefox because it detects the existence of StopIteration and assumes that all generators are old style. I believe there are Firefox extensions depending on Q.async to continue working. Q will perhaps need to be upgraded to detect whether an iterator implements the old or new protocol per instance. Any recommended way to distinguish old and new iterators by inspection of the instance? See:

https://github.com/kriskowal/q/blob/3159ac47c46cb7e2200191736ca430a011615523/q.js#L1217

Comment 8

[Till Schneidereit [:till]]

> Yes.

Ok. Unfortunately, I won't have the time to work on this before Friday, realistically speaking, so if we want to get this in before the uplift, I'm afraid somebody else would have to do it.

Comment 9

[Jeff Walden [:Waldo]]

If you're flat-out *guaranteed* that |generator| is either a legacy generator, or an ES6 star-style generator, then I think the prototype chain is your best bet to distinguish them.

The chain for a * generator is supposed to be:

  function* f() {}
  f() -> f.prototype -> %GeneratorPrototype% -> %IteratorPrototype% -> Object.prototype -> null

The chain for a legacy generator is:

  function g() { yield 17; }
  g() -> legacy iterator prototype -> Object.prototype -> null

Now, it happens we don't implement |f|'s chain correctly (bug 1091945?), so inspecting based on that is probably a bad idea.  What's actually on each prototype chain, in terms of properties?

js> function dump(obj)                                      
{
  while (obj !== Object.prototype)
  {
    print("[" + Object.getOwnPropertyNames(obj) + "]");
    obj = Object.getPrototypeOf(obj);
  }
}
js> f
function* f() {}
js> dump(f())
[]
[]
[next,throw,constructor]
js> g
function g() { yield 3; }
js> dump(g());
[]
[next,send,throw,close]

I think your best bet is to check for the "next", "send", "throw", and "close" properties all existing as own properties on the prototype -- and .constructor.name being "Object".  Because f.prototype.* can be assigned and affect generator instances in ES6, if I stretch I can imagine the properties all existing on the prototype -- maybe -- for something like a Socket generator.  But %GeneratorPrototype% has a constructor property/function on it, and that function is supposed to have the name "GeneratorFunction" if I read ES6 right.  I don't foresee a situation where that changes.  So I would recommend this test:

function isLegacyGenerator(gen)
{
  var prot = Object.getPrototypeOf(gen);
  return prot.hasOwnProperty("send") &&
         prot.hasOwnProperty("close") &&
         prot.hasOwnProperty("next") &&
         prot.hasOwnProperty("throw") &&
         prot.constructor.name === "Object";
}

This could still fail for something like this (with our current buggy prototype chain that doesn't set %GeneratorPrototype%.constructor.name to "GeneratorFunction"):

function* Socket()
{
}
Socket.prototype = {
  send() { },
  close() { },
  next() { },
  throw() { }
};

but such would also lose easy recognition of the instance as a Socket when debugging (except via instanceof), so I think it's somewhat unlikely.

It's not a perfect test, but I suspect it's as close as you can get without using Function() to try legacy syntax directly.  (But even that wouldn't work, because you'd have to have q.js loaded as JS1.7 or so, which presumably it wasn't.  So, back to closest-to.)

Comment 11

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/legacy-iterator-and-generator-support-will-be-removed/

Comment 12

[Masatoshi Kimura [:emk]]

Now legacy generators usage in content is almost zero. If Q breaks content, it should already break it because nobody uses legacy generators in content.

So I think we can disable legacy generators in content now.

Comment 13

[Tooru Fujisawa [:arai] (away until 08-17)]

we should be able to remove it now, both from content and chrome, at the same time.