Skip to content
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

[BUGFIX release] Fix for #13071 V8 inlining bug #13118

Merged
merged 1 commit into from
Mar 17, 2016

Conversation

raido
Copy link
Contributor

@raido raido commented Mar 17, 2016

I have made changes based on a comment here - https://bugs.chromium.org/p/v8/issues/detail?id=4839#c9

I will prefix the commit once it is proven that this solves the problem.

@workmanw
Copy link

As mentioned on #13071, as far as I can tell, this does successfully work around the V8 bug.

Given the nature of this issue, I would advocate this be merged and a new version of ember 2.4 be cut. I'm not sure, but it might be worth also back porting for 2.3 users given that 2.3 was the first version that exposed this V8 bug.

Also since it's not at all obvious why the code is this way, it might make sense to add a quick code comment referencing either this PR or issue #13071. Maybe that would help to avoid this regressing in the future. I'm not sure what ember's code standards are in this area, that might be overkill. Just a thought, maybe an ember core member could weigh in on the value of that.

@rwjblue
Copy link
Member

rwjblue commented Mar 17, 2016

I'd be fine with a small comment, but I also think that Chrome should fix it :stuck_out_tongue_closed_eyes.

I completely agree that this should be backported to 2.4.x, as such the commit should be prefixed with [BUGFIX release].

@raido
Copy link
Contributor Author

raido commented Mar 17, 2016

Prefixed and added issue links as comments.

@rwjblue
Copy link
Member

rwjblue commented Mar 17, 2016

Once green I'll pull into release branch and y'all can use it from there. I'd love it if folks could test that on the release channel before doing an official 2.4.3 just to get a bit more testing time into it and really confirm things are fixed...

@workmanw
Copy link

@rwjblue Hehe. I am building Chrome from source presently to test their fix. (https://codereview.chromium.org/1811693002).

My main concern is that we unfortunately have enterprise customers who lock their users into arbitrary versions of Chrome. One customer only permits their employees to use Chrome 42 . Obviously ember can't support every version of Chrome forever. But having an ember side workaround for a period of time should help smooth out the impact of this issue.

I have no idea why enterprises insist on locking their users into versions of Chrome. I've asked this repeatedly and gotten no good answers :(.

@raido
Copy link
Contributor Author

raido commented Mar 17, 2016

@rwjblue Sure, i will test release branch once it's merged.

@raido raido changed the title Possible fix for #13071 [BUGFIX release] Fix for #13071 V8 inlining bug Mar 17, 2016
rwjblue added a commit that referenced this pull request Mar 17, 2016
[BUGFIX release] Fix for #13071 V8 inlining bug
@rwjblue rwjblue merged commit 92e3160 into emberjs:master Mar 17, 2016
@stefanpenner
Copy link
Member

@raido / @workmanw good job guys!

@jakobkummerow
Copy link

Just to note: the Chrome-side fix will show up in Canary in the next 1-2 days, and then in versions 49 and 50 over the next 1-3 weeks or so (whenever Stable/Beta channels get refreshed).
Of course if you want to support older revisions (as workmanw noted), you have no choice but to also deploy the ember-side workaround.

Running outdated browser versions (of any browser!) is indeed a pretty bad idea, especially in an enterprise environment where security presumably matters. All outdated browsers have security bugs (as well as other bugs, like this one) that simply won't get fixed.

@jaro-sevcik
Copy link

Indeed, good job; thank you for your hard work on the repro! Glad to hear the suggested workaround worked.

This kind of bugs is really hard to discover for us because such bugs do not result in crashes, so we do not see them in statistics. This particular one was especially tricky because it relies on deoptimization triggering at a very specific point for a very specific reason (I spent embarrassing amount of time debugging this).

@stefanpenner
Copy link
Member

glad ember can be a smoke test for you guys :)

@rwjblue
Copy link
Member

rwjblue commented Mar 18, 2016

Released in v2.4.3.

@stefanpenner
Copy link
Member

@rwjblue <3

@raido raido deleted the bug-fix-13071 branch March 18, 2016 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants