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 lts] Fix double container destroy #15207

Merged

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented May 4, 2017

This code on the Engine Instance is not needed as it includes the
ContainerProxyMixin, which already takes care of destroying the
container.

This duplicated destroy step was causing ember-cp-validations to
attempt to destroy validators twice. This destroy process tries to
nullify some property with o.set(‘model’, null) on an object that is
already destroyed.

This code on the Engine Instance is not needed as it includes the
`ContainerProxyMixin`, which already takes care of destroying the 
container.

This duplicated destroy step was causing ember-cp-validations to
attempt to destroy validators twice. This destroy process tries to 
nullify some property with `o.set(‘model’, null)` on an object that is
already destroyed.
@cibernox
Copy link
Contributor Author

cibernox commented May 4, 2017

This is the code in the ContainerProxyMixin that already takes care of destroying the container.
https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/mixins/container_proxy.js#L128-L134

@rwjblue
Copy link
Member

rwjblue commented May 4, 2017

This seems like the correct change, but I would like to understand why it only started affecting things on Ember 2.14 beta before removing it...

@cibernox
Copy link
Contributor Author

cibernox commented May 4, 2017

I cannot pinpoint the exact point where things go wrong, but I've noticed that on the test I'm using to trace this, on Ember 2.13 the list of keys to destroy contains three items (["controller:application", "controller:users", "controller:users/login"]) and in 2.14 it contains more controllers (["controller:application", "controller:users/login", "controller:users", "controller:users/signup/index", "controller:passwords/index"]).

It's in controller:passwords/index where validations are mixed:

const emailValidations = buildValidations({
  'model.email': email
});

export default Controller.extend(emailValidations, {

so I only see the error in 2.14.

I think that the different behaviour is because of the changes in the way transition.retry works. There is an open bug for that.

Either way, destroying the container twice seems accidental and the fix make sense still.

@rwjblue
Copy link
Member

rwjblue commented May 4, 2017

After chatting with @cibernox about this in slack a bit more, I believe that the reason for the additional controllers being instantiated is #15178 (removal of controller.proto() shenanigans). Now we owner.lookup the controllers needed for QPs whereas before we did a bunch of hoop jumping to avoid that .lookup.

@rwjblue
Copy link
Member

rwjblue commented May 4, 2017

@cibernox - Can you also remove https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/system/engine-instance.js#L137?

Nevermind me, I just linked to the same thing that is being removed in this PR 😝 ...

@rwjblue rwjblue changed the title [BUGFIX beta] Fix double container destroy [BUGFIX lts] Fix double container destroy May 4, 2017
@rwjblue
Copy link
Member

rwjblue commented May 4, 2017

Updated description to be [BUGFIX lts] as this bugfix will need to be backported to 2.12 (once released in a 2.13 point release).

@rwjblue rwjblue merged commit 28892b8 into emberjs:master May 4, 2017
@cibernox cibernox deleted the BUGFIX/fix-double-container-destroy branch May 4, 2017 16:26
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.

2 participants