DOM Node memory leak with phx-update=prepend/append

[mveytsman]

Environment

• Elixir version (elixir -v): 1.10.3
• Phoenix version (mix deps): 1.5.3
• Phoenix LiveView version (mix deps): https://github.com/phoenixframework/phoenix_live_view/commit/254bd86f708dd1ea7087270b4f1ade59bfa6609c
• NodeJS version (node -v): v12.18.0
• NPM version (npm -v): 6.14.4
• Operating system: Linux
• Browsers you attempted to reproduce this bug on (the more the merrier): Firefox & Chrome
• Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: Yes

Issue

I found what I think is a pretty major DOM memory leak when using phx-update="prepend" and phx-update="append" along with a list comprehension.

TL;DR When using phx-update="append"/phx-update="prepend" in a container containing a comprehension, for every element updated we will add an extra TextNode containing only whitespace. This means our DOM will grow unbounded with every element updated.

Below I include a sample LiveView demonstrating the issue, and discussion of what's going on. I also have a PR fixing the bug ready to go, but am creating this issue separately as I'm not sure if my approach to fixing the issue is the right one.

By the way, I do not believe this is related to https://github.com/phoenixframework/phoenix_live_view/issues/978

Demonstration

Consider this sample LiveView. We render 20 divs inside a comprehension with phx-update="append" on the parent. Each div has a unique id. We can click a button to randomly update one of the divs, and another button to update all of them.

defmodule PixelPartyWeb.TestLive do
  use Phoenix.LiveView

  @impl true
  def render(assigns) do
    ~L"""
    <a href="#" phx-click="update-random">Update a random div</a>
    <a href="#" phx-click="update-all">Update all</a>
    <div id="list" phx-update="append"  phx-hook="CountChildNodes">
      <%= for {i, item} <- @list do %>
        <div id="<%= i %>"><%= item %></div>
      <% end %>
    </div>
    """
  end

  @impl true
  def mount(_params, _session, socket) do
    length = 20
    list = for i <- 0..(length - 1), do: {i, 0}

    {:ok,
     socket
     |> assign(:counter, 1)
     |> assign(:length, length)
     |> assign(:list, list), temporary_assigns: [list: []]}
  end

  @impl true
  def handle_event("update-random", _, %{assigns: %{length: length, counter: counter}} = socket) do
    {:noreply,
     socket |> assign(:list, [{Enum.random(0..length-1), counter}]) |> assign(:counter, counter + 1)}
  end

  @impl true
  def handle_event("update-all", _, %{assigns: %{length: length}} = socket) do
    list = for i <- 0..(length - 1), do: {i, 0}

    {:noreply,
     socket |> assign(:list, list) |> assign(:counter, 1)}
  end
end

To demonstrate the issue we also include the following hook which prints out the number of childnodes of the container on each update:

let Hooks = {}
Hooks.CountChildNodes = {
  updated() {
    console.log(this.el.childNodes.length)
  }
}

let liveSocket = new LiveSocket("/live", Socket, {hooks: Hooks, params: {_csrf_token: csrfToken}})

Here's my console after I click "update a random div" 4 times and then click "update all" 4 times:

I found this can have a significant impact on performance when I have a non-trivial number of elements. For example, on my computer when I set length to 1000 and enable profiling, updating a random div will take ~100ms. If I click "update all" 20 times I end up with ~20k childnodes and ~1000ms for a full patch.

Discussion

To make things clearer, I set length=2 for the following html samples.

Here's what's going on:

1) The EEX comprehension as written above will generate newlines around the divs:

<div id="list" phx-update="append"  phx-hook="CountChildNodes">

    <div id="0">0</div>

    <div id="1">0</div>

</div>

This means that our initial DOM tree looks like this

Div id="list"
  TextNode contents ="\n"
  Div id="0"
  TextNode contents ="\n"
  Div id="1"
  TextNode contents ="\n"

This is why in the console log screenshot I included above our initial mount had 41 childNodes, when we only had 20 div children.

2) When doing phx-update="append"/phx-update="prepend" we don't discard nodes during the morphdom patching:

https://github.com/phoenixframework/phoenix_live_view/blob/254bd86f708dd1ea7087270b4f1ade59bfa6609c/assets/js/phoenix_live_view.js#L1353-L1355

Then, after we're done patching we re-order them back to their original order:

https://github.com/phoenixframework/phoenix_live_view/blob/254bd86f708dd1ea7087270b4f1ade59bfa6609c/assets/js/phoenix_live_view.js#L1425-L1439

So, after the socket is mounted and we do a re-render, we will end up pushing the divs to the top and have the following DOM tree:

Div id="list"
  Div id="0"
  Div id="1"
  TextNode contents ="\n"
  TextNode contents ="\n"
  TextNode contents ="\n"

3) If I'm updating just the div with id 0, I'm patching in this tree:

Div id="list"
  TextNode contents ="\n"
  Div id="0"
  TextNode contents ="\n"

We didn't discard any nodes, so before re-arranging divs, my tree looks like this:

Div id="list"
  Div id="1"
  TextNode contents ="\n"
  TextNode contents ="\n"
  TextNode contents ="\n"
  Div id="0"
  TextNode contents ="\n"

Then I do the post morphdom re-ordering and I get

Div id="list"
  Div id="0"
  Div id="1"
  TextNode contents ="\n"
  TextNode contents ="\n"
  TextNode contents ="\n"
  TextNode contents ="\n"

This is how we grew from 3 TextNodes to 4. If we update N elements we will add N extra empty TextNodes!

Solutions

I'm come up with a few ways to fix this, and I've made a PR for the one I think is cleanest. I'm relatively new to this project, so I'm not sure if I'm missing something obvious solution-wise, and would appreciate feedback on my approach / if there's a better one!

1) Only skip discarding elements in phx-update="append"/"prepend" when they have ids. I think this is the cleanest way to do this. 2) Prevent adding new nodes if they are whitespace-only TextNodes 3) Prune whitespace-only text nodes from the container before we do the post-morphdom re-arranging. 4) Prune whitespace-only text nodes from inside comprehensions, similar to how we do it inside components: https://github.com/phoenixframework/phoenix_live_view/blob/254bd86f708dd1ea7087270b4f1ade59bfa6609c/assets/js/phoenix_live_view.js#L304-L313

[josevalim]

Thanks for the detailed report! :heart: Awesome job! :100:

Note the docs already say this:

When using phx-update, a unique DOM ID must always be set in the container. If using "append" or "prepend", a DOM ID must also be set for each child.

So we are clear to adopt solution 1. Although we should probably warn if we are discarding an actual tag, as users may be surprised to why their elements are not appearing on the page (no need to warn for text nodes). If you can add a test (we should have a reasonable coverage for this that you can copy and adapt), even better!

[mveytsman]

Thanks! I'll add a test & a warning to my PR.