New issue
Advanced search Search tips

Issue 3692 link

Starred by 27 users

Issue metadata

Status: Invalid
Owner: ----
Closed: Apr 2015
HW: ----
NextAction: ----
OS: ----
Priority: ----
Type: ----



Sign in to add a comment

function suddenly becomes undefined

Reported by mobilebr...@gmail.com, Nov 12 2014

Issue description

This test shows what we believe is v8 generating invalid optimized code, causing a function to suddenly become undefined.  This appears to affect when.js versions 3.4.2 and higher, although the failure appears to happen under different conditions depending on the version of when.js and/or the version of v8 (via Node.js).

Here's an executable showing the problem, and a more complete description of the issue, expected and actual results, etc.:

https://github.com/briancavalier/when-v8-invalid-optimization

### Environment

The test in the repo above is design to show the failure on:

* Node 0.10.33
* when.js 3.6.0

### Expected result

Program completes without error.

### Actual result

Program fails at a seemingly random, but reproducible iteration.

The stack trace below points to this line in the `_beget` function: https://github.com/cujojs/when/blob/3.6.0/lib/makePromise.js#L166).

```
<lots more output above>
#57 reading file with promise: `dist/lodash.compat.js`
#57 parsing file: `dist/lodash.compat.js`
**
 * @license
 * Lo-Dash 2.4.1 (Custom Build) <http://lodash.com/>
 * Build: `lodash -o ./dist/loda Program
registering catch # 393
Potentially unhandled rejection [1] TypeError: object is not a function
    at Promise._beget (/private/tmp/345/node_modules/when/lib/makePromise.js:166:16)
    at Promise.then (/private/tmp/345/node_modules/when/lib/makePromise.js:140:17)
    at Promise.catch (/private/tmp/345/node_modules/when/lib/makePromise.js:156:16)
    at Promise.catch.Promise.otherwise (/private/tmp/345/node_modules/when/lib/decorators/flow.js:37:22)
    at /private/tmp/345/test.js:87:24
    at next (/private/tmp/345/node_modules/when/lib/decorators/iterate.js:57:20)
    at /private/tmp/345/node_modules/when/lib/decorators/array.js:39:24
    at tryCatchReject (/private/tmp/345/node_modules/when/lib/makePromise.js:838:30)
    at runContinuation1 (/private/tmp/345/node_modules/when/lib/makePromise.js:797:4)
    at Fulfilled.when (/private/tmp/345/node_modules/when/lib/makePromise.js:588:4)
    at Pending.run (/private/tmp/345/node_modules/when/lib/makePromise.js:479:13)
#57 reading file with promise: `dist/lodash.compat.min.js`
#57 parsing file: `dist/lodash.compat.min.js`
**
 * @license
 * Lo-Dash 2.4.1 (Custom Build) lodash.com/license | Underscore.js 1.5.2 underscorejs Program
<end of output, program terminates>
```

### Potential workaround

Note that in at least some cases (not sure about all yet), adding an empty try/finally as first line of the `_beget` function avoids the failure, presumably because that either prevents optimization, or changes the optimization in a way that avoids generating the erroneous compiled code.

```js
Promise.prototype._beget = function() {
	try {} finally {};
	...
}
```

### References

For more information see the following github issues:

* Original issue report: https://github.com/cujojs/when/issues/345 long, best to read from bottom up or start here: https://github.com/cujojs/when/issues/345#issuecomment-51775158
* Followup with test case: https://github.com/cujojs/when/issues/403 by @anodynos https://github.com/anodynos)



 

Comment 1 by jh...@pivotal.io, Nov 12 2014

I've also encountered this behavior (an in-scope function becoming undefined), but have not been able to create a simple test case.
I found that if I change the _beget method to delegate to another function, the failure also goes away without having to introduce a try/finally.  for example:

```js
        Promise.prototype._beget = function() {
            return begetFrom(this._handler, this.constructor);
        };

        function begetFrom(parent, Promise) {
            var child = new Pending(parent.receiver, parent.join().context);
            return new Promise(Handler, child);
        }
```

Which is functionally equivalent to the original, but perhaps changes the optimized bytecode output?

Comment 3 by i...@bnoordhuis.nl, Nov 12 2014

@OP: node.js v0.10 ships with V8 3.14, which is no longer supported by upstream.

For that matter, the latest v0.11 release bundles V8 3.26, which isn't supported anymore, either.
Is there another party, besides upstream who is supporting those versions of v8?
Sorry, a more productive question probably is: Who supports those versions of v8?  The node team?  I'm happy to file this issue with the appropriate folks if this isn't the right place.

Comment 6 by i...@bnoordhuis.nl, Nov 12 2014

> Who supports those versions of v8?  The node team?

Sadly, no.  It's a bug in the current development process.  Sometimes, it's possible to cherry-pick patches from upstream but only if they are small and apply cleanly.  In general, however, you should not expect any fixes to be forthcoming.
Thanks for the candid answer.

Since this problem is manifesting via the when.js package, as the maintainer, it seems I have no real recourse but to try to devise a workaround (like the one above that splits _beget), and cross my fingers that the problem no longer manifests in applications that depend on the package.

That's a pretty frustrating situation for me, and for users of when.js.

To end on a better note, node 0.11.14 hasn't yet hit the same failure with the test program.

Comment 8 by m.go...@gmail.com, Nov 12 2014

Re #6:
>> Who supports those versions of v8?  The node team?
> 
> Sadly, no.  It's a bug in the current development process.

Wow, that is sad. Considering the importance of Node I think Google should evaluate the concept of v8 LTS releases. I don't mean the span from Node 0.10 to 0.12, it is way too long but at least a few months perhaps?

Either that or Node should be able to update v8 each 6 weeks, even on stable releases. Current situation is really scary for Node public apps.
In the same vein, it might be worth it for the Node team to consider more frequent patch releases releases that focus only on maintaining parity with V8.    Lot's of projects have this problem, I'm not sure it's a good idea to put the burden solely on the V8 team.
I def don't want to point fingers at any of the teams involved, and I hope my frustration above didn't come across as finger pointing.  It's the situation that's unfortunate.  At this point, I think it's important to ask the question of how it came to be, and more importantly, what can be done to ensure it doesn't happen in the future.

There just has to be a way to keep release versions of node within a reasonable v8 version window.  Like #8 and #9 mentioned, more frequent releases, patch releases dedicated solely to v8 updates, internal node  abstractions to isolate v8 APIs so that changes to v8 have less of an impact, or are easier to deal with, etc.

Does anyone know if there's a new plan already in place for doing that with 0.12?

Comment 11 by i...@bnoordhuis.nl, Nov 14 2014

> Does anyone know if there's a new plan already in place for doing that with 0.12?

The tentative plan post-v0.12 is to switch to time-based releases, so we can track upstream V8 more closely.  Release frequency and support (i.e. how long to support a release) are still TBD; much hinges on available man power.
Besides V8 integration strategies/politics in nodejs, may I add that we should check if this very weird optimization bug still exists in more current V8 versions like chrome's Perhaps its still there but has gone largely unnoticed, like it has in the nodejs community. (PS: I'm https://github.com/anodynos who managed to reproduce it on node 10 with an anorthodox way).
Status: Invalid
#12: We've fixed a handful of bugs that would manifest like this last summer. It's very likely that this bug was among them; IIUC node 0.11 doesn't reproduce it, which matches this assumption.

Of course, if you do encounter bugs in recent V8 versions, keep the reports coming!

I'll close this issue as 3.14 is not going to get fixed. Feel free to continue your discussion, though.

Comment 14 by dort...@gmail.com, Nov 15 2014

Lol I wonder how someone from PayPal would  feel if they encountered this thread. 
Like we would with any other bug: track it, work around it, fix it.
@jkummerow I totally agree with you about staleness of v8 in v0.10. There are tons of stuff that was fixed by just rewriting, and even more plain bug fixes.

I have debugged this issue a bit, and I am not totally sure if this bug could be applied to trunk v8. Do you mind helping me verify that it is fixed now?

Basically, the reproduction test case for old node is there: https://gist.github.com/indutny/3130c097d11570476f22 .

The idea is that if we have ArgumentsElements in outer-to-inlined function, it will PushArgument and shift the stack. But the deoptimizer does not seem to take in account that stack shift when it sees `arg:0` in it's environment, thus loading the wrong value on deopt right before CallNew.

The responsible code is at WriteTranslation (at least in older v8 versions). Could you please help me in verifying that newer v8 is not vulnerable to this?

Thank you,
Fedor.
@16: I don't understand your request. The repro case doesn't do anything obviously wrong with recent V8; o.outer(o) always returns |undefined| as I would expect. So...?

Sign in to add a comment