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-1-13] backports V8 memory leak bug through registry / resolver access. #13220

Closed

Conversation

calderas
Copy link

Related to #12618

@rwjblue
Copy link
Member

rwjblue commented Mar 31, 2016

Hmm, I'm somewhat concerned about this change being backported. It is a fairly large set of changes, and due to that it poses a level of risk that I'm not sure we should take on...

@rwjblue
Copy link
Member

rwjblue commented Apr 10, 2016

We discussed this in detail during the most recent core team meeting (see notes here), and we generally agree that their is a bit more risk in backporting than we are comfortable with. It is likely that other changes are required (there were when the original PR landed) in various parts of the ecosystem (like ember-test-helpers, ember-qunit, etc) to support this change.

We would absolutely consider a fix that is less risky (ala making these functions too large to inline by adding a large string)...

@rwjblue
Copy link
Member

rwjblue commented Apr 10, 2016

See https://github.com/emberjs/ember.js/pull/12652/files for an example of a very low risk change that prevents the leak also.

@rwjblue
Copy link
Member

rwjblue commented Jun 2, 2016

Closing based on response to last few comments.

@rwjblue rwjblue closed this Jun 2, 2016
@micky2be
Copy link

I tried using this patch on Ember 1.13.13 and still get PhantomJS to crash.
I'm trying to update my app from 1.12.2 to 2.x and going through 1.13.x is kinda mandatory for us to make sure we are not breaking anything on the way (it's a big app).

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

Successfully merging this pull request may close these issues.

4 participants