Skip to content

Commit

Permalink
[BUGFIX beta] Avoid rerendering outlet state during router destruction.
Browse files Browse the repository at this point in the history
During each `Ember.Route`'s `willDestroy` we trigger a
`run.once(this.router, '_setOutlets');`. This is so that the routes
views are destroyed properly (by removing them from the outlet state).

During `Ember.Router`'s `willDestroy` we clear
`this._toplevelView` (along with a bunch of other cleanup).

These two things combined can mean that `this._toplevelView` is `null`
when `_setOutlets` is called again (during the `Router`'s destruction).
In that scenario we are actually creating another
`this._toplevelView` and setting up another rendered root (since one
doesn't exist). When this happens the newly created root is never
cleaned up, since the Router's `willDestroy` has already ran and can no
longer clean up after itself.
  • Loading branch information
Robert Jackson committed Aug 24, 2016
1 parent 5f5b9ae commit 890fb04
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 0 deletions.
9 changes: 9 additions & 0 deletions packages/ember-routing/lib/system/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ const EmberRouter = EmberObject.extend(Evented, {
this._engineInstances = new EmptyObject();
this._engineInfoByRoute = new EmptyObject();
}

// avoid shaping issues with checks during `_setOutlets`
this.isDestroyed = false;
this.isDestroying = false;
},

/*
Expand Down Expand Up @@ -266,6 +270,11 @@ const EmberRouter = EmberObject.extend(Evented, {
},

_setOutlets() {
// This is triggered async during Ember.Route#willDestroy.
// If the router is also being destroyed we do not want to
// to create another this._toplevelView (and leak the renderer)
if (this.isDestroying || this.isDestroyed) { return; }

let handlerInfos = this.router.currentHandlerInfos;
let route;
let defaultParentState;
Expand Down
28 changes: 28 additions & 0 deletions packages/ember/tests/application_lifecycle_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,34 @@ QUnit.test('Destroying the application resets the router before the appInstance
equal(controllerFor(appInstance, 'application').get('selectedMenuItem'), null);
});

QUnit.test('Destroying a route after the router does create an undestroyed `toplevelView`', function() {
App.Router.map(function() {
this.route('home', { path: '/' });
});

setTemplates({
index: compile('Index!'),
application: compile('Application! {{outlet}}')
});

App.IndexRoute = Route.extend();
run(App, 'advanceReadiness');

handleURL('/');

let router = appInstance.lookup('router:main');
let route = appInstance.lookup('route:index');

run(router, 'destroy');
equal(router._toplevelView, null, 'the toplevelView was cleared');

run(route, 'destroy');
equal(router._toplevelView, null, 'the toplevelView was not reinitialized');

run(App, 'destroy');
equal(router._toplevelView, null, 'the toplevelView was not reinitialized');
});

QUnit.test('initializers can augment an applications customEvents hash', function(assert) {
assert.expect(1);

Expand Down
44 changes: 44 additions & 0 deletions packages/ember/tests/routing/basic_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3742,4 +3742,48 @@ if (isEnabled('ember-application-engines')) {

throws(() => router.transitionTo('blog.post'), /Defining a custom serialize method on an Engine route is not supported/);
});

QUnit.test('App.destroy does not leave undestroyed views after clearing engines', function() {
expect(4);

let engineInstance;
// Register engine
let BlogEngine = Engine.extend();
registry.register('engine:blog', BlogEngine);
let EngineIndexRoute = Route.extend({
init() {
this._super(...arguments);
engineInstance = getOwner(this);
}
});

// Register engine route map
let BlogMap = function() {
this.route('post');
};
registry.register('route-map:blog', BlogMap);

Router.map(function() {
this.mount('blog');
});

bootApplication();

let engine = container.lookup('engine:blog');
engine.register('route:index', EngineIndexRoute);
engine.register('template:index', compile('Engine Post!'));

handleURL('/blog');

let route = engineInstance.lookup('route:index');

run(router, 'destroy');
equal(router._toplevelView, null, 'the toplevelView was cleared');

run(route, 'destroy');
equal(router._toplevelView, null, 'the toplevelView was not reinitialized');

run(App, 'destroy');
equal(router._toplevelView, null, 'the toplevelView was not reinitialized');
});
}

0 comments on commit 890fb04

Please sign in to comment.