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

Error after upgrading to Ember 2.4 #13071

Closed
workmanw opened this Issue Mar 9, 2016 · 79 comments

Comments

Projects
None yet
@workmanw
Contributor

workmanw commented Mar 9, 2016

This error only occurs after the app has been active and running for a while, giving the user a chance to navigate between several different routes. It's very hard to reproduce, it seems to "just happen" after a while. We've been able to reproduce it a few times using our production build (ember.min.js), but never using a debug build (ember.debug.js).

Here is the stack:

 "Cannot read property '_lookupFactory' of undefined"

TypeError: Cannot read property '_lookupFactory' of undefined
    at i (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:7:2712)
    at o (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:7:2833)
    at Object.a [as default] (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:7:2888)
    at Object.i [as subexpr] (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:6:4717)
    at a (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:15:16476)
    at i (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:15:16302)
    at n (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:15:16189)
    at Object.r [as acceptHash] (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:15:16075)
    at n (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:15:26102)
    at Object.a.inline (https://qa-integration.batterii.com/assets/vendor-69da94618271be1c4338db3f0e942865.js:15:26664)

That seems to point back to the lookup-helper. The few times I've been lucky enough to catch this at a breakpoint, I've observed that the minified owner parameter becomes undefined. The rest of the env looks correct. See:

image
image

I know that's very little to go on. Short of coming up with an easy way to reproduce this, is there any additional information about the stack that might helpful in debugging this?

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 9, 2016

What's even more odd is that you can see in the last screenshot, there is var s = "helper:" + e; on line 8786 ... but somehow on the very next line s is undefined. 😕 It's almost as if the stack is getting borked ... or the chrome debugger is just getting it wrong.

@raido

This comment has been minimized.

Contributor

raido commented Mar 9, 2016

I have seen something like this too. Happens rarely and i do not know how to reproduce. Also i am not sure if it is my app's problem or something else.

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 9, 2016

@raido is your stack the same as mine (IE the failure is on _lookupFactory)? Are you on Ember 2.4.1? Have you seen this issue on prior versions of ember?

We upgraded from Ember 2.2 => Ember 2.4 and started seeing this issue pretty heavily in our server logs within a few days.

@rlivsey

This comment has been minimized.

Contributor

rlivsey commented Mar 9, 2016

👍 seen this myself and confused the heck out of me, from my bugsnag logs:

11779      if (validateLazyHelperName(name, owner, env.hooks.keywords)) {
11780        var helperName = 'helper:' + name;
11781        if (owner.hasRegistration(helperName, options)) {
11782          helper = owner._lookupFactory(helperName, options);
11783        }
11784      }
11785    }

As you say, how can owner be undefined on line 11782 if it's got past the owner.hasRegistration the line before?

Likewise only seen it on production when minified (above comes from sourcemaps).

Logs show we've only seen it on Chrome so far.

@raido

This comment has been minimized.

Contributor

raido commented Mar 10, 2016

@workmanw Yes, my error is also related too _lookupFactory and is see lookupHelper in the stacktrace.

Logs from production build, happened just now with v2.3.0

TypeError: Cannot read property '_lookupFactory' of undefined
    at o (vendor-6292d0672068025de3c6d57c1fb505d0.js:7)
    at Object.a [as default] (vendor-6292d0672068025de3c6d57c1fb505d0.js:7)
    at Object.r [as lookupHelper] (vendor-6292d0672068025de3c6d57c1fb505d0.js:6)
    at Object.D [as inline] (vendor-6292d0672068025de3c6d57c1fb505d0.js:16)
    at Object.i.inline (vendor-6292d0672068025de3c6d57c1fb505d0.js:16)
    at l.populateNodes (vendor-6292d0672068025de3c6d57c1fb505d0.js:16)
    at l.render (vendor-6292d0672068025de3c6d57c1fb505d0.js:16)
    at i (vendor-6292d0672068025de3c6d57c1fb505d0.js:16)
    at vendor-6292d0672068025de3c6d57c1fb505d0.js:16
    at s (vendor-6292d0672068025de3c6d57c1fb505d0.js:16)
@jcbvm

This comment has been minimized.

jcbvm commented Mar 11, 2016

I can confirm this issue on 2.4.2, seeing it sometimes in production, not seen in development yet. Can this be caused by the ember-inspector maybe?

For more info: never had this on 2.3.x and the stack is the same (_lookupFactory)

EDIT: I can confirm that this is not a ember-inspector issue, error occured while inspector was disabled.

@gpoitch

This comment has been minimized.

Contributor

gpoitch commented Mar 11, 2016

Confirmed bug in 2.4.1 and 2.4.2. Happens only with minified js

@mixonic mixonic added the Bug label Mar 11, 2016

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 11, 2016

@jcbvm @gdub22 Have either of you been able to reproduce it consistently? I've tried and tried to find at least a consistent set of steps in our app with the hope of building an ember-twiddle, but I've had no luck.

This is completely anecdotal and probably a red herring, but it seems to happen while trying to render a helper which has an ancestor component rendered with the component helper ({{component componentName}}).

@rlivsey

This comment has been minimized.

Contributor

rlivsey commented Mar 11, 2016

@workmanw Likewise not been able to reproduce consistently, we do use the {{component}} helper a lot throughout our app.

@gpoitch

This comment has been minimized.

Contributor

gpoitch commented Mar 11, 2016

@workmanw not 100% consistently but I think I narrowed it down to a particular component which does happen to use the component helper in its own template.

@ef4

This comment has been minimized.

Contributor

ef4 commented Mar 11, 2016

Uglify transpiles the function above to:

function n(e,t,r,n){
  var i=r.helpers[e];
  if(!i){
    var o=r.owner;
    if (a(e,o,r.hooks.keywords)){
      var s="helper:"+e;
      o.hasRegistration(s,n) && (i=o._lookupFactory(s,n));
    }
  }
  return i;
}

Notice that both property accesses on owner have been compiled down to a single line. My guess is that the crash actually happens on the first one, and the sourcemap fidelity is not high enough to distinguish them correctly.

Edited to add: Ah, but the crash is definitely for a _lookupFactory property, so my speculation must be wrong. Curiouser and curiouser.

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 11, 2016

@ef4 Maybe ... but the few times I've caught this exception in the debugger, o (in your snippet) is undefined, but r.owner is a valid owner. In fact, I'm able to do r.owner.hasRegistration(s,n) && (r.owner._lookupFactory(s,n)); and get the class. Granted this is after Chrome has broken on a caught exception ... so the debugger could also be in a misleading state.

@rlivsey

This comment has been minimized.

Contributor

rlivsey commented Mar 11, 2016

@ef4 yep it's definitely a strange one.

Could V8 be optimising it away for some reason? Who knows most about the dark arts of V8, @stefanpenner maybe?

@wycats

This comment has been minimized.

Member

wycats commented Mar 12, 2016

If the line code that is crashing is:

o.hasRegistration(s,n) && (i=o._lookupFactory(s,n));

then:

  1. o.hasRegistration(s,n) has already returned a truthy value; this means
  2. o is neither null or undefined; therefore
  3. Cannot read property '_lookupFactory' of undefined is either wrong or a bug in v8

Am I missing something obvious?

@wycats

This comment has been minimized.

Member

wycats commented Mar 12, 2016

For people who have reproduced this problem, what version of Chrome are you running? Have you reproduced it in Firefox, IE, or Safari?

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 12, 2016

@wycats I don't see anything obvious you're missing. These are the same conclusions I arrived at. We have only seen this issue logged on Chrome latest (48). In my reproduction testing, I was using 48.0.2564.116. I personally did not try Firefox, IE or Safari, but I will try them out and report back.

EDIT: I cannot simplify the problem to the point of producing a twiddle. But if it would be helpful, I can queue up one or more failures paused at a breakpoint, and jump on a screenhero if someone wants to poke around in the chrome debugger. Sometimes I can reproduce it 3 times in a minute. Sometimes it takes 20 minutes or more.

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 12, 2016

@wycats Alright. I spent the last hour trying Chrome, Safari and Firefox. I was able to reproduce it several times in Chrome and not at all in Safari and Firefox. I'm not sure this is conclusive, but it is certainly pointing more and more toward a Chrome specific issue.

@wycats

This comment has been minimized.

Member

wycats commented Mar 12, 2016

@raido

This comment has been minimized.

Contributor

raido commented Mar 12, 2016

I have not been able to reproduce it either. For me it typically happens when app boots. Reloading app several times probably crash it, yet it is not consistent. It may crash 5 times in just minutes or take half an hour. I tested on latest Chrome.

@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Mar 12, 2016

@workmanw I would recommend forcing that function to stay in specific modes, this may help us find out more about the issue. At which point we should also ping our v8 friends. But for that, a reproduction (even a full app) is likely going to be required.

To keep functions in specific modes, you can slowly disable parts of v8, and see if the bug stops. This will help us get closer to the root cause.

first I would just disable inlining, by running chromes v8 with it disabled: --nouse_inlining (my guess is this may be sufficient)

/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --js-flags="--nouse_inlining" --user-data-dir=/tmp/foobar

On the flip side, try disabling crankshaft all together --crankshaft=false

/Applications/Google\ Chrome\ Canary.app/Contents/MacOS/Google\ Chrome\ Canary --js-flags="--crankshaft=false" --user-data-dir=/tmp/foobar
@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 12, 2016

@stefanpenner

I tested this using Chrome 48 (rather than canary). If you'd like me to try with Canary, I'd be happy to.

With --nouse_inlining I was not able to reproduce this issue.
With --crankshaft=false I was able to reproduce this issue.

At which point we should also ping our v8 friends. But for that, a reproduction (even a full app) is likely going to be required.

I completely understand. I've spent about 4 hours trying to "work forwards" and build an Ember-twiddle that reproduces this. I just don't understand enough of what's going on to do that. So now I'm going to start "working backwards" using our app to simplify the reproduction by reducing the reproduction steps and ripping out as much out of our app as I can.

@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Mar 12, 2016

With --nouse_inlining I was not able to reproduce this issue.

seems expected, so its most likely related to inlining bug of some kind.

With --crankshaft=false I was able to reproduce this issue.

this may be the wrong flag, i can't remember.

I completely understand. I've spent about 4 hours trying to "work forwards" and build an Ember-twiddle that reproduces this. I just don't understand enough of what's going on to do that. So now I'm going to start "working backwards" using our app to simplify the reproduction by reducing the reproduction steps and ripping out as much out of our app as I can.

@workmanw is it possible to share the app (or a URL to the app) as-is with steps to reproduce?
The reality is, reproducing this in isolation may be tricky.

@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Mar 12, 2016

A :sadpanda: work-around, is to force the function to exceed the max AST that can be inlined. This should allow you to 🚢

putting the following string in that function's body, should do the trick (right now, these limits/heuristics change overtime)

"Pork chop porchetta rump, bacon turducken filet mignon tri-tip drumstick picanha beef ribs sausage salami. Leberkas beef landjaeger bresaola, sausage meatloaf pastrami frankfurter ribeye jowl turducken drumstick flank. Pork loin shank tongue leberkas ham strip steak salami swine short ribs cupim. Strip steak sausage turkey tenderloin, alcatra turducken porchetta ribeye brisket spare ribs rump salami ground round tail frankfurter. Kielbasa cow porchetta, hamburger jowl salami turducken capicola beef. Corned beef meatloaf ball tip landjaeger shank pork belly. Short loin kielbasa pig tail, brisket cupim salami andouille hamburger sausage short ribs."
@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Mar 12, 2016

@workmanw if you can also check in canary, that would be handy.

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 12, 2016

@stefanpenner

putting the following string in that function, should do the trick (right now, these can change overtime)

Your knowledge of the internals never cease to amaze me.

@workmanw is it possible to share the app as-is with steps to reproduce?

Yea, I should be able to make that happen. If you give me a little bit I'll provision some credentials to one of our pre-production environments and get the data primed. I could probably get okay to share the source if need be, but would have to do that privately.

@workmanw if you can also check in canary, that would be handy.

Done. I checked canary "version 51.0.2673.0 canary (64-bit)" without either of the flags and unfortunately I could still reproduce it.

@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Mar 12, 2016

I could probably get okay to share the source if need be, but would have to do that privately.

It would help me personally try and reduce the problem further, although no guarantees. I more or less would love to do the following:

  • prepare a reproduction for the V8 folks
  • explore an interim [BUGFIX] solutions
  • get a V8 report in (today/this weekend)

With a app in-front of me (where I can change code + explore) discovering a proper work-around/reproduction might be possible.

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 12, 2016

EDIT: The application linked below is using a build of ember which contains the workaround from PR #13118. It is no longer be valid for reproduction of this issue. If anyone is interested in reproducing this issue using our app, contact me and I could probably make that happen.


@stefanpenner So I did whittle down the reproduction by injecting some code into the page. Believe me, otherwise it would have been a nightmare.

This is still not 100% reproducible. If you follow these steps and it still hasn't occurred, try restarting chrome all together.

  1. Visiting this URL: https://qa-integration.batterii.com/#/community/MTpDb21tdW5pdHksOTAwMQ/room/MTpSb29tLDE5NzQ2MzAwMQ/wall/MTpSb29tLDE5NzQ2MzAwMSxXYWxsLDEwMDAx

  2. Login with email: emberdev@batterii.com and password: tomster1. That should deeplink you into a page that looks like this:

image

  1. After logging in and landing on the above page, you'll need to refresh (so that you start clean from that page).

  2. Open your chrome debugger and run the following in the console:

(function() {
var room = 'MTpSb29tLDE5NzQ2MzAwMQ',
    wall = 'MTpSb29tLDE5NzQ2MzAwMSxXYWxsLDEwMDAx',
    wallitem = 'MTpXYWxsSXRlbSwxOTg0NjMwMDQ';

function promiseTimer(ms) {
  return new Ember.RSVP.Promise(function(resolve) {
    Ember.run.later(resolve, ms);
  });
}

function timedTransition() {
  return BC.router.transitionTo.apply(BC.router, arguments).then(function() {
    return promiseTimer(800);
  });
}

function takeActions() {
  var downloadUrl = window.wallitemRecord.get('downloadUrl');
  window.open(downloadUrl);
  promiseTimer(1400).then(function() {
    return timedTransition('wall.wallitem', room, wall, wallitem);
  }).then(function() {
    return timedTransition('wall', room, wall);
  }).then(function() {
    return timedTransition('wall.wallitem', room, wall, wallitem);
  }).then(function() {
    return timedTransition('wall', room, wall);
  }).then(function() {
    return timedTransition('wall.wallitem', room, wall, wallitem);
  }).then(function() {
    return timedTransition('wall', room, wall);
  }).then(function() {
    return timedTransition('wall.wallitem', room, wall, wallitem);
  }).then(function() {
    return timedTransition('wall', room, wall);
  }).then(function() {
    return timedTransition('wall.wallitem', room, wall, wallitem);
  }).then(function() {
    return timedTransition('wall', room, wall);
  });
}

BC.store.findRecord('wallitem', wallitem).then(function(wallitem) { window.wallitemRecord = wallitem; });
$('<button id="crash-reproduce">Crash Reproduce</button>').appendTo('.top-right-nav');
$('#crash-reproduce').on('click', takeActions);
})();
  1. Set Chrome Debugger to "Pause on Caught Exceptions".

  2. The injected code should have added a button in the upper right corner. Click that button. First a new tab will open and trigger a file to be downloaded, then a modal dialog should open and close several times. That modal is actually "routable" so you should also observe the route changing. On the second or third open of the modal, you should hit the exception. If this doesn't occur, refresh the browser and try again (starting with step 4).

image


I'll work on getting you source code in the meantime. We run on Google App Engine so you will still have to connect to a cloud server, but I should be able to arrange it so you can run the client app locally (proxying to a cloud server).

Lastly, I'm on slack now and will be until about 4PM EST. I'd be happy to screenhero if need be. I'll also be available all day tomorrow.

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 17, 2016

🎊 🎉 I tested @raido's PR and I cannot reproduce the issue. That does seem to be a successful workaround. 😄

Edit: Tested with Chrome 49 and Chrome 51.

@rwjblue

This comment has been minimized.

Member

rwjblue commented Mar 17, 2016

I have been trying to follow the reported scenarios closely, but I believe this code is in Ember 2.3 also. Does Ember 2.3 also suffer this issue?

@raido

This comment has been minimized.

Contributor

raido commented Mar 17, 2016

Yes, i can confirm that Ember v2.3 is affected by this too.

rwjblue added a commit that referenced this issue Mar 17, 2016

Merge pull request #13118 from raido/bug-fix-13071
[BUGFIX release] Fix for #13071 V8 inlining bug

rwjblue added a commit that referenced this issue Mar 17, 2016

rwjblue added a commit that referenced this issue Mar 17, 2016

rwjblue added a commit that referenced this issue Mar 17, 2016

[BUGFIX release] Fix for #13071 V8 inlining bug
(cherry picked from commit a6b09b3)
@rwjblue

This comment has been minimized.

Member

rwjblue commented Mar 17, 2016

OK, pulled #13118 into beta, release, and release-2-3 branches. The release and beta channels should get new builds shortly (via Travis) please bang on it for a while so we can confirm it does indeed fix this...

@2468ben

This comment has been minimized.

2468ben commented Mar 17, 2016

@workmanw @raido Thank you so much for spending so much time reproducing the bug, working with Chromium to find & fix this issue.
It's one of those Heisenbugs where, trying to be a responsible OSS user, I waited to file an issue for over a month because I couldn't make a stable gist/twiddle/bin/etc. It just defied so much logic that I figured it must be my fault. Next time I'll be more proactive and ping the Slack room for fellow victims.

@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Mar 17, 2016

Good job all!

@2468ben i wonder if we should maybe have a hesienbug/maybe vmbug label?

@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Mar 17, 2016

[fixed by #13118]

@workmanw

This comment has been minimized.

Contributor

workmanw commented Mar 17, 2016

:)

@rwjblue I've updated my bower.json now to use "ember": "components/ember#9c3e5820" and I'm going to send it through the QA cycle. I'll let you know if any issues popup.

@rwjblue

This comment has been minimized.

Member

rwjblue commented Mar 17, 2016

@cdoornink

This comment has been minimized.

cdoornink commented Mar 17, 2016

We've also been experiencing this issue and this appears to have fixed it. I've been hammering our site against the ember#9c3e5820 build for over an hour now and haven't seen any issues.

rwjblue added a commit that referenced this issue Mar 18, 2016

[BUGFIX release] Fix for #13071 V8 inlining bug
(cherry picked from commit a6b09b3)
@rwjblue

This comment has been minimized.

Member

rwjblue commented Mar 18, 2016

v2.4.3 has been released with the work around added in #13118.

stefanpenner added a commit that referenced this issue Mar 18, 2016

@courthead

This comment has been minimized.

courthead commented Mar 21, 2016

Oh man, the hours I spent trying to reproduce/fix this bug before finding this thread... ugh. Good work guys!

@Turbo87

This comment has been minimized.

Member

Turbo87 commented Jul 24, 2017

we're currently running an app on ember@2.4.6 and we're hitting this exact issue according to Sentry even though the code includes the workaround in #13118 😞

@workmanw

This comment has been minimized.

Contributor

workmanw commented Jul 24, 2017

@Turbo87 This issue was also fix in Chrome as well. So there should only be 1 or 2 versions of Chrome that could run into this.

Are you sure it's the same issue?

@Turbo87

This comment has been minimized.

Member

Turbo87 commented Jul 24, 2017

@workmanw yeah, pretty sure it's the same. apparently some of our users are still running old Chrome versions and in fact some derived browsers (Sogou Explorer, Opera, Chromium, Dragon) are showing similar behavior according to our Sentry logs

@workmanw

This comment has been minimized.

Contributor

workmanw commented Jul 24, 2017

:(. I feel your pain. Some of our customers are enterprises that lock users into specific versions of Chrome and never let them upgrade.

It's possible that this issue resurfaced in some way. I can say with 100% certainty that, at the time, this workaround did resolve the issue for us (v2.4.3).

@raido

This comment has been minimized.

Contributor

raido commented Jul 24, 2017

Same here.

@krisselden

This comment has been minimized.

Member

krisselden commented Jul 24, 2017

@Turbo87 do you have specific versions?

@Turbo87

This comment has been minimized.

Member

Turbo87 commented Jul 24, 2017

  • Chrome 49.0.2623
  • Opera 36.0.2130
  • Chromium 48.0.2564
  • Sogou Explorer 1.0
  • Chrome 48.0.2564
  • Chrome 50.0.2632

Chrome 49 is by far the most common one for some reason

@givanse

This comment has been minimized.

Contributor

givanse commented Nov 9, 2017

@Turbo87 did you ever found a way around this problem?

@Turbo87

This comment has been minimized.

Member

Turbo87 commented Nov 9, 2017

@givanse we've upgraded to Ember 2.12 by now and don't seem to have this issue anymore

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