New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Browser Client] Minor memory leak: Text nodes in notification area #1931

Closed
jvilk opened this Issue Sep 21, 2017 · 1 comment

Comments

Projects
None yet
2 participants
@jvilk
Contributor

jvilk commented Sep 21, 2017

Problem

When Mailpile adds a notification to the notification area, it accidentally adds three DOM nodes instead of one. You can see this happening by examining document.getElementById('notification-bubbles').childNodes while navigating between individual emails and the inbox.

Explanation

When mailpile adds a notification to the notification area, it produces a document fragment using the following HTML template:

"
  <div id="event-<%= event_id %>" class="notification-bubble <%= status %> hide">
    <span class="icon <%= icon %>"></span>
    <span class="text">
      <span class="message"><%= message %></span><br>
      <% if (message2) { %>
      <span class="action"><%= message2 %>
      <%   if (action_js) { %>
        - <a <%= action_js %> data-event_id="<%= event_id %>" class="action <%= action_cls %>"><%= action_text %></a>
      <%   } else if (action_url) { %>
        - <a href="<%= action_url %>" data-event_id="<%= event_id %>" class="action <%= action_cls %>"><%= action_text %></a>
      <%   } %>
      </span>
      <% } else if (type === 'nagify') { %>
      <span class="action">Want to <a href="<%= action %>" data-event_id="<%= event_id %>" class="action notification-nag <%= action_cls %>">do it now?</a></span>
      <% } else if (undo) { %>
      <span class="action">A mistake? <a href="#" data-event_id="<%= event_id %>" class="action notification-undo <%= action_cls %>">undo</a></span>
      <% } %>
    </span>
    <a href="#" data-type="<%= type %>" class="notification-close"><span class="icon-x"></span></a>
  </div>
"

...and then uses jQuery to replace an existing notification or prepend it to the notifications area.

Due to the newlines in the above template, jQuery inserts three nodes into the DOM:

  • A text node with a newline.
  • The notification div.
  • A text node with a newline.

Later on, when the notification finishes, it animates the notification sliding up and removes the div with the event ID from the DOM. However, this leaves two stray text nodes in the DOM.

Suggested Fixes

  • Remove the newlines from the template.
  • trim() the template string before passing it to jQuery.
@BjarniRunar

This comment has been minimized.

Member

BjarniRunar commented Sep 22, 2017

Thank you for reporting this and suggesting a fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment