Bypass isConnected checks when destroying FIE #17890
Merged
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
This may be the cause of all the "window is null" bugs...
dreamofabear
approved these changes
Sep 5, 2018
src/custom-element.js
Outdated
| * Called when an element is disconnected from DOM, or when an ampDoc is | ||
| * being disconnected (the element itself may still be connected to ampDoc). | ||
| * | ||
| * This does not guard against connected elements. |
There was a problem hiding this comment.
This is confusing since we still check this.isConnected_. Maybe rename that variable to connectedCallbackWasCalled_?
src/custom-element.js
Outdated
| * | ||
| * This does not guard against connected elements. | ||
| */ | ||
| disconnect() { |
There was a problem hiding this comment.
I think using opt_force = false or opt_checkDom = true param in disconnectedCallback is more readable. Then add a comment to resource.js about why we pass false in that case.
|
@jridgewell @choumx is this PR important enough to cherry pick into the current release before promoting to production? All of these referenced issues are present in current canary at relatively high error rates. |
Enriqe
pushed a commit
to Enriqe/amphtml
that referenced
this pull request
Nov 28, 2018
* Bypass isConnected checks when destroying FIE This may be the cause of all the "window is null" bugs... * Tests and reviews
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.
This may be the cause of all the "window is null" bugs...
Introduced in #13945, we started to ignore
disconnectedCallbacks when the node itself was still connected to a document. But, FIE's destroy themselves and try to clean upResources even though those resources are still connected to the FIE's document. So, we would end up ignoring the disconnect, leaving all of them in memory and active.Fixes #17839.
Fixes #15883.
Fixes #17604.
Probably more... Anything that touches FIE.
Fixes (maybe) #9177?
Fixes (maybe) #17840?
Working on tests now.