[BUGFIX beta] Avoid rerendering outlet state during router destruction. #14123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@rwjblue this may be tricky, but can we test this? Testing some observable "We did rerender" would help prevent regressions in this future. |
|
@stefanpenner - I don't know. This was the result of a gnarly debugging session when running Ember's own tests in randomized order (where there were a large number of failures since leaking a root view in Glimmer is worse). I am happy to try to add a specific test, but I think the only way to do it would be to force the router to be destroyed before a route instance. |
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.
rwjblue
force-pushed
the
fix-router-teardown
branch
from
c3a4dac to
890fb04
Compare
|
@stefanpenner - Added two tests (one with an engine and one without). Tests fail before this change. |
|
tests look legit |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During each
Ember.Route'swillDestroywe trigger arun.once(this.router, '_setOutlets');. This is so that the routes views are destroyed properly (by removing them from the outlet state).During
Ember.Router'swillDestroywe clearthis._toplevelView(along with a bunch of other cleanup).These two things combined can mean that
this._toplevelViewisnullwhen_setOutletsis called again (during theRouter's destruction). In that scenario we are actually creating anotherthis._toplevelViewand setting up another rendered root (since one doesn't exist). When this happens the newly created root is never cleaned up, since the Router'swillDestroyhas already ran and can no longer clean up after itself.This fixes a number of test failures that crop up when doing fuzz testing (via
seedURL param with QUnit).