New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

yield * broken for Firefox < 45 #274

Closed
AnilRedshift opened this Issue Jan 31, 2017 · 5 comments

Comments

Projects
None yet
3 participants
@AnilRedshift

AnilRedshift commented Jan 31, 2017

So this is a fun bug.
Try running this code through regenerator on FF 44 or below:

function *inner() { yield 1; }
function *outer() { yield *inner(); }

function runMe() {
    var g = outer();
    console.log(g.next());
    console.log(g.next());
    console.log(g.next());
}

You'll note that the generator never reports done as true. Here are the notes from my investigation. I am not 100% of the reason because some of the macro code in gecko is not easy to parse.

  1. Firefox used to support some alternative form of the iterator.
  2. It looks like they implemented the ES2015 version in FF 27, but then had some backcompat work to detect when code was trying to use the old protocol.
  3. Specifically, note that LegacyIteratorNext always returns done: false
  4. Now note that any delegate relies on next() working in order to iterate through the delegate
  function values(iterable) {
    if (iterable) {
      var iteratorMethod = iterable[iteratorSymbol];
      if (iteratorMethod) {
        return iteratorMethod.call(iterable);
      }

where iteratorSymbol in this case is Symbol.iterator
5. The scene is almost set. wrap uses the Generator.prototype to create its functions:

  function wrap(innerFn, outerFn, self, tryLocsList) {
    // If outerFn provided and outerFn.prototype is a Generator, then outerFn.prototype instanceof Generator.
    var protoGenerator = outerFn && outerFn.prototype instanceof Generator ? outerFn : Generator;
    var generator = Object.create(protoGenerator.prototype);
  1. And lastly (this is where some educated amount of speculation comes in), it looks like the iterator methods on the Generator prototype in FF < 45 are completely busted.

So, putting it all together, this is what I believe happens:
Due to the bad Generator prototype, firefox decides that the code trying to get the iterator in values() is legacy and shoves it down the legacy iterator codepath. This means that next will ALWAYS return done: false unless a StopIteration exception is thrown.
The current code does not do such a thing and therefore the generator never ends. It just keeps on going on my friends....

So: What's the fix? It looks like you have a few options:

  • Don't use the generator prototype for these builds of firefox
  • Polyfill next() and related iterator methods for these builds of firefox (note that is what we chose to do to fix this bug)
  • Write some code that does browser detection and throws a StopIteration exception instead
  • Convince FF some other way to give you the normal codepath. I was able to do this by using Symbol["@@iterator"] instead of Symbol.iterator. However it appears that is fraught with peril.

tl;dr: yield * never worked on at the very least Firefox 44 and probably several builds earlier than that. It's a comedy of browser issues that you now get to work around. Thanks!

@AnilRedshift AnilRedshift changed the title from yield * broken for FF < 45 to yield * broken for Firefox < 45 Jan 31, 2017

@benjamn

This comment has been minimized.

Collaborator

benjamn commented Jan 31, 2017

Thanks for the in-depth analysis, @AnilRedshift!

Rather than browser detection, I would prefer to detect this specific (mis)feature, and then polyfill next() appropriately. Is there a snippet of code that determines if this bug exists?

I may be making a silly mistake, but I'm not able to reproduce the problem using your reproduction in the Regenerator sandbox in Firefox 44. What version of Regenerator are you using? Does other code have to run first to cause the problem to appear?

@AnilRedshift

This comment has been minimized.

AnilRedshift commented Jan 31, 2017

Hmm, the repro is pretty consistent for us here. This is what I've been using to debug:
ffbug.zip

It consists of just running regenerator --include-runtime and then hooking that up to a simple html file.

 ./regenerator --version
0.9.5

Using https://ftp.mozilla.org/pub/firefox/releases/44.0/Firefox-44.0.en-US.mac-x86_64.sdk.tar.bz2

to test.

@rayd

This comment has been minimized.

Contributor

rayd commented Feb 9, 2017

I'm also able to reproduce this in Firefox 42 with the above code, but not in the sandbox page, which is a little odd to me. There's nothing else loading in the test page other than the regenerator runtime. (I'm also running into this in a big way in one of my projects). Stepping through the code, I've noticed the following happening:

  • The info object that is returned when invoking the "next" method of the delegate/inner generator comes back with a value of
    {
      done: false,
      value: 1
    }
  • As it's passed up the chain from "invoke" through the helper and to the tryCatch function, it's somehow mutated to become
    {
      done: false,
      value: {
        done: false,
        value: 1
      }
    }
  • It's hard to get any visibility into what's happening -- when I'm looking at the <return> value at this point in the code:
    // Helper for defining the .next, .throw, and .return methods of the
    // Iterator interface in terms of a single ._invoke method.
    function defineIteratorMethods(prototype) {
      ["next", "throw", "return"].forEach(function(method) {
        prototype[method] = function(arg) {
          return this._invoke(method, arg);
        };
      });
    }
    the value looks correct (i.e. not nested). But by the time we make it here (which at least in the debugger is just the parent in the call stack) the arg value has become nested as show above:
    function tryCatch(fn, obj, arg) {
      try {
        return { type: "normal", arg: fn.call(obj, arg) };
      } catch (err) {
        return { type: "throw", arg: err };
      }
    }

If I hack the tryCatch function like so

function tryCatch(fn, obj, arg) {
  try {
    var newarg = fn.call(obj, arg);
    if (newarg && newarg.value && newarg.value.hasOwnProperty('value')) {
      newarg.done = newarg.value.done;
      newarg.value = newarg.value.value;
    }
    return { type: "normal", arg: newarg };
  } catch (err) {
    return { type: "throw", arg: err };
  }
}

then things work as expected, but this is obviously just masking the problem, not getting to the root of it. Hope this is somehow helpful. I'd be glad to help further to get to the bottom of this however I can.

@AnilRedshift

This comment has been minimized.

AnilRedshift commented Feb 10, 2017

For folks hitting this bug, this is the quick workaround we did in the meantime:

<script>
if (navigator.userAgent.toLowerCase().indexOf('firefox') > -1) {
    window.Symbol = undefined;
}
</script>

Running that script before including the babel-polyfill causes babel to polyfill all things related to symbols, including iterators. It's definitely a crude approach, but it is effective.

@rayd

This comment has been minimized.

Contributor

rayd commented Feb 15, 2017

A bit more info on this -- try running the browser tests (from test/index.html) in FF < 45, and things start to fail (they're fine in more recent Firefox and in Chome):

# Install browserify if you don't have it so the browser tests are built
npm install browserify
npm test
firefox test/index.html 

I should have a PR coming shortly with what appears to be a fix using specific (mis)feature-detection. I say "appears" because it makes the browser tests pass in FF 42, at least :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment