Disconnect loaded Frame Element while rendering #454
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
seanpdoyle
commented
Nov 18, 2021
| @@ -29,6 +29,7 @@ export class FrameRenderer extends Renderer<FrameElement> { | |||
| if (sourceRange) { | |||
| sourceRange.selectNodeContents(frameElement) | |||
| this.currentElement.appendChild(sourceRange.extractContents()) | |||
| frameElement.disconnectedCallback() | |||
There was a problem hiding this comment.
I'm not sure of a way to add a test to guard against a regression.
The getEventListeners() function used to uncover this leak is defined in the console's suite of tools, and isn't available to the document.
I'm open to ideas!
There was a problem hiding this comment.
Maybe attach a listener that increments a global you can check?
There was a problem hiding this comment.
I don't think that the code within the connectedCallback() and disconnectedCallback() fire any events directly. It's also worth noting that we're manually invoking these methods within Turbo itself, so they're kind of "synthetic" connections, which makes a MutationObserver similarly unpredictable, since the underlying element itself might not be attached to the browser's document.
|
@dhh this feels like an important fix to ship as part of |
Closes [hotwired#453] --- While loading a matching `<turbo-frame>` element from a response body, the `FrameController` "activates" it by invoking `FrameElement.connectedCallback()` after importing it into the document. During that activation, several event listeners are attached, including some on the `document`. Unfortunately, that `FrameElement.connectedCallback()` invocation is never mirrored with a `FrameElement.disconnectedCallback()` invocation, so the attached listeners are never detached, causing an accumulation. This commit invokes the `disconnectedCallback()` after loaded element's contents are extracted into the browser's document. [hotwired#453]: hotwired#453
seanpdoyle
force-pushed
the
frame-leaking-event-listeners
branch
from
666429f to
da43934
Compare
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 18, 2021
Add test coverage to [hotwired#454][]. While the original fix was important to ship, it was unclear how to guard against re-introducing the leaks. This commit introduces the `withoutChangingEventListenersCount()` test helper to wrap a block within the test harness. While executing within the block, the driven page's `document.addEventListener` and `document.removeEventListener` functions are replaced with decorated functions that increment and decrement a counter. At the start of the block, capture and return how many times `addEventListener` has been invoked without a matching `removeEventListener`. At the end, capture and return the differential. [hotwired#454]: hotwired#454
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 18, 2021
The fix introduced in [hotwired#454][] resolved the issue, but introduced a matching `disconnectedCallback()` far-off in the `FrameRenderer`. This commit moves that call to occur _immediately_ after it's paired `element.connectedCallback()`. [hotwired#454]: hotwired#454
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 18, 2021
Add test coverage to [hotwired#454][]. While the original fix was important to ship, it was unclear how to guard against re-introducing the leaks. This commit introduces the `withoutChangingEventListenersCount()` test helper to wrap a block within the test harness. While executing within the block, the driven page's `document.addEventListener` and `document.removeEventListener` functions are replaced with decorated functions that increment and decrement a counter. At the start of the block, capture and return how many times `addEventListener` has been invoked without a matching `removeEventListener`. At the end, capture and return the differential. [hotwired#454]: hotwired#454
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 18, 2021
The fix introduced in [hotwired#454][] resolved the issue, but introduced a matching `disconnectedCallback()` far-off in the `FrameRenderer`. This commit moves that call to occur _immediately_ after it's paired `element.connectedCallback()`. [hotwired#454]: hotwired#454
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 18, 2021
The fix introduced in [hotwired#454][] resolved the issue, but introduced a matching `disconnectedCallback()` far-off in the `FrameRenderer`. This commit moves that call to occur _immediately_ after it's paired `element.connectedCallback()`. [hotwired#454]: hotwired#454
dhh
added a commit
that referenced
this pull request
Nov 19, 2021
* Add test coverage for event listener leaking Add test coverage to [#454][]. While the original fix was important to ship, it was unclear how to guard against re-introducing the leaks. This commit introduces the `withoutChangingEventListenersCount()` test helper to wrap a block within the test harness. While executing within the block, the driven page's `document.addEventListener` and `document.removeEventListener` functions are replaced with decorated functions that increment and decrement a counter. At the start of the block, capture and return how many times `addEventListener` has been invoked without a matching `removeEventListener`. At the end, capture and return the differential. [#454]: #454 * Move the `disconnectedCallback()` call The fix introduced in [#454][] resolved the issue, but introduced a matching `disconnectedCallback()` far-off in the `FrameRenderer`. This commit moves that call to occur _immediately_ after it's paired `element.connectedCallback()`. [#454]: #454 Co-authored-by: David Heinemeier Hansson <david@basecamp.com>
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.
Closes #453
While loading a matching
<turbo-frame>element from a response body,the
FrameController"activates" it by invokingFrameElement.connectedCallback()after importing it into the document.During that activation, several event listeners are attached, including
some on the
document.Unfortunately, that
FrameElement.connectedCallback()invocation isnever mirrored with a
FrameElement.disconnectedCallback()invocation,so the attached listeners are never detached, causing an accumulation.
This commit invokes the
disconnectedCallback()after loaded element'scontents are extracted into the browser's document.