Merge pull request #13806 from atom/ns-as-element-pool-cleanup

Overhaul element pool and debug double-free errors

spec/atom-environment-spec.coffee

@@ -142,6 +142,11 @@ describe "AtomEnvironment", ->
           atom.assert(false, "a == b", (e) -> error = e)
           expect(error).toBe errors[0]
 
+      describe "if passed metadata", ->
+        it "assigns the metadata on the assertion failure's error object", ->
+          atom.assert(false, "a == b", {foo: 'bar'})
+          expect(errors[0].metadata).toEqual {foo: 'bar'}
+
     describe "if the condition is true", ->
       it "does nothing", ->
         result = atom.assert(true, "a == b")

spec/dom-element-pool-spec.coffee

@@ -1,60 +0,0 @@
-DOMElementPool = require '../src/dom-element-pool'
-{contains} = require 'underscore-plus'
-
-describe "DOMElementPool", ->
-  domElementPool = null
-
-  beforeEach ->
-    domElementPool = new DOMElementPool
-
-  it "builds DOM nodes, recycling them when they are freed", ->
-    [div, span1, span2, span3, span4, span5, textNode] = elements = [
-      domElementPool.buildElement("div")
-      domElementPool.buildElement("span")
-      domElementPool.buildElement("span")
-      domElementPool.buildElement("span")
-      domElementPool.buildElement("span")
-      domElementPool.buildElement("span")
-      domElementPool.buildText("Hello world!")
-    ]
-
-    div.appendChild(span1)
-    span1.appendChild(span2)
-    div.appendChild(span3)
-    span3.appendChild(span4)
-    span4.appendChild(textNode)
-
-    domElementPool.freeElementAndDescendants(div)
-    domElementPool.freeElementAndDescendants(span5)
-
-    expect(contains(elements, domElementPool.buildElement("div"))).toBe(true)
-    expect(contains(elements, domElementPool.buildElement("span"))).toBe(true)
-    expect(contains(elements, domElementPool.buildElement("span"))).toBe(true)
-    expect(contains(elements, domElementPool.buildElement("span"))).toBe(true)
-    expect(contains(elements, domElementPool.buildElement("span"))).toBe(true)
-    expect(contains(elements, domElementPool.buildElement("span"))).toBe(true)
-    expect(contains(elements, domElementPool.buildText("another text"))).toBe(true)
-
-    expect(contains(elements, domElementPool.buildElement("div"))).toBe(false)
-    expect(contains(elements, domElementPool.buildElement("span"))).toBe(false)
-    expect(contains(elements, domElementPool.buildText("unexisting"))).toBe(false)
-
-  it "forgets free nodes after being cleared", ->
-    span = domElementPool.buildElement("span")
-    div = domElementPool.buildElement("div")
-    domElementPool.freeElementAndDescendants(span)
-    domElementPool.freeElementAndDescendants(div)
-
-    domElementPool.clear()
-
-    expect(domElementPool.buildElement("span")).not.toBe(span)
-    expect(domElementPool.buildElement("div")).not.toBe(div)
-
-  it "throws an error when trying to free the same node twice", ->
-    div = domElementPool.buildElement("div")
-    domElementPool.freeElementAndDescendants(div)
-    expect(-> domElementPool.freeElementAndDescendants(div)).toThrow()
-
-  it "throws an error when trying to free an invalid element", ->
-    expect(-> domElementPool.freeElementAndDescendants(null)).toThrow()
-    expect(-> domElementPool.freeElementAndDescendants(undefined)).toThrow()

spec/dom-element-pool-spec.js

@@ -0,0 +1,112 @@
+const DOMElementPool = require ('../src/dom-element-pool')
+
+describe('DOMElementPool', function () {
+  let domElementPool
+
+  beforeEach(() => { domElementPool = new DOMElementPool() })
+
+  it('builds DOM nodes, recycling them when they are freed', function () {
+    let elements
+    const [div, span1, span2, span3, span4, span5, textNode] = Array.from(elements = [
+      domElementPool.buildElement('div', 'foo'),
+      domElementPool.buildElement('span'),
+      domElementPool.buildElement('span'),
+      domElementPool.buildElement('span'),
+      domElementPool.buildElement('span'),
+      domElementPool.buildElement('span'),
+      domElementPool.buildText('Hello world!')
+    ])
+
+    expect(div.className).toBe('foo')
+    div.textContent = 'testing'
+    div.style.backgroundColor = 'red'
+    div.dataset.foo = 'bar'
+
+    expect(textNode.textContent).toBe('Hello world!')
+
+    div.appendChild(span1)
+    span1.appendChild(span2)
+    div.appendChild(span3)
+    span3.appendChild(span4)
+    span4.appendChild(textNode)
+
+    domElementPool.freeElementAndDescendants(div)
+    domElementPool.freeElementAndDescendants(span5)
+
+    expect(elements.includes(domElementPool.buildElement('div'))).toBe(true)
+    expect(elements.includes(domElementPool.buildElement('span'))).toBe(true)
+    expect(elements.includes(domElementPool.buildElement('span'))).toBe(true)
+    expect(elements.includes(domElementPool.buildElement('span'))).toBe(true)
+    expect(elements.includes(domElementPool.buildElement('span'))).toBe(true)
+    expect(elements.includes(domElementPool.buildElement('span'))).toBe(true)
+    expect(elements.includes(domElementPool.buildText('another text'))).toBe(true)
+
+    expect(elements.includes(domElementPool.buildElement('div'))).toBe(false)
+    expect(elements.includes(domElementPool.buildElement('span'))).toBe(false)
+    expect(elements.includes(domElementPool.buildText('unexisting'))).toBe(false)
+
+    expect(div.className).toBe('')
+    expect(div.textContent).toBe('')
+    expect(div.style.backgroundColor).toBe('')
+    expect(div.dataset.foo).toBeUndefined()
+
+    expect(textNode.textContent).toBe('another text')
+  })
+
+  it('forgets free nodes after being cleared', function () {
+    const span = domElementPool.buildElement('span')
+    const div = domElementPool.buildElement('div')
+    domElementPool.freeElementAndDescendants(span)
+    domElementPool.freeElementAndDescendants(div)
+
+    domElementPool.clear()
+
+    expect(domElementPool.buildElement('span')).not.toBe(span)
+    expect(domElementPool.buildElement('div')).not.toBe(div)
+  })
+
+  it('does not attempt to free nodes that were not created by the pool', () => {
+    let assertionFailure
+    atom.onDidFailAssertion((error) => assertionFailure = error)
+
+    const foreignDiv = document.createElement('div')
+    const div = domElementPool.buildElement('div')
+    div.appendChild(foreignDiv)
+    domElementPool.freeElementAndDescendants(div)
+    const span = domElementPool.buildElement('span')
+    span.appendChild(foreignDiv)
+    domElementPool.freeElementAndDescendants(span)
+
+    expect(assertionFailure).toBeUndefined()
+  })
+
+  it('fails an assertion when freeing the same element twice', function () {
+    let assertionFailure
+    atom.onDidFailAssertion((error) => assertionFailure = error)
+
+    const div = domElementPool.buildElement('div')
+    div.textContent = 'testing'
+    domElementPool.freeElementAndDescendants(div)
+    expect(assertionFailure).toBeUndefined()
+    domElementPool.freeElementAndDescendants(div)
+    expect(assertionFailure.message).toBe('Assertion failed: The element has already been freed!')
+    expect(assertionFailure.metadata.content).toBe('<div>testing</div>')
+  })
+
+  it('fails an assertion when freeing the same text node twice', function () {
+    let assertionFailure
+    atom.onDidFailAssertion((error) => assertionFailure = error)
+
+    const node = domElementPool.buildText('testing')
+    domElementPool.freeElementAndDescendants(node)
+    expect(assertionFailure).toBeUndefined()
+    domElementPool.freeElementAndDescendants(node)
+    expect(assertionFailure.message).toBe('Assertion failed: The element has already been freed!')
+    expect(assertionFailure.metadata.content).toBe('testing')
+  })
+
+  it('throws an error when trying to free an invalid element', function () {
+    expect(() => domElementPool.freeElementAndDescendants(null)).toThrow()
+    expect(() => domElementPool.freeElementAndDescendants(undefined)).toThrow()
+  })
+})

src/atom-environment.coffee

@@ -821,12 +821,17 @@ class AtomEnvironment extends Model
   Section: Private
   ###
 
-  assert: (condition, message, callback) ->
+  assert: (condition, message, callbackOrMetadata) ->
     return true if condition
 
     error = new Error("Assertion failed: #{message}")
     Error.captureStackTrace(error, @assert)
-    callback?(error)
+
+    if callbackOrMetadata?
+      if typeof callbackOrMetadata is 'function'
+        callbackOrMetadata?(error)
+      else
+        error.metadata = callbackOrMetadata
 
     @emitter.emit 'did-fail-assertion', error
 

src/dom-element-pool.coffee

@@ -1,55 +0,0 @@
-module.exports =
-class DOMElementPool
-  constructor: ->
-    @freeElementsByTagName = {}
-    @freedElements = new Set
-
-  clear: ->
-    @freedElements.clear()
-    for tagName, freeElements of @freeElementsByTagName
-      freeElements.length = 0
-    return
-
-  build: (tagName, factory, reset) ->
-    element = @freeElementsByTagName[tagName]?.pop()
-    element ?= factory()
-    reset(element)
-    @freedElements.delete(element)
-    element
-
-  buildElement: (tagName, className) ->
-    factory = -> document.createElement(tagName)
-    reset = (element) ->
-      delete element.dataset[dataId] for dataId of element.dataset
-      element.removeAttribute("style")
-      if className?
-        element.className = className
-      else
-        element.removeAttribute("class")
-    @build(tagName, factory, reset)
-
-  buildText: (textContent) ->
-    factory = -> document.createTextNode(textContent)
-    reset = (element) -> element.textContent = textContent
-    @build("#text", factory, reset)
-
-  freeElementAndDescendants: (element) ->
-    @free(element)
-    @freeDescendants(element)
-
-  freeDescendants: (element) ->
-    for descendant in element.childNodes by -1
-      @free(descendant)
-      @freeDescendants(descendant)
-    return
-
-  free: (element) ->
-    throw new Error("The element cannot be null or undefined.") unless element?
-    throw new Error("The element has already been freed!") if @freedElements.has(element)
-
-    tagName = element.nodeName.toLowerCase()
-    @freeElementsByTagName[tagName] ?= []
-    @freeElementsByTagName[tagName].push(element)
-    @freedElements.add(element)
-
-    element.remove()

src/dom-element-pool.js

@@ -0,0 +1,89 @@
+module.exports =
+class DOMElementPool {
+  constructor () {
+    this.managedElements = new Set()
+    this.freeElementsByTagName = new Map()
+    this.freedElements = new Set()
+  }
+
+  clear () {
+    this.managedElements.clear()
+    this.freedElements.clear()
+    this.freeElementsByTagName.clear()
+  }
+
+  buildElement (tagName, className) {
+    const elements = this.freeElementsByTagName.get(tagName)
+    let element = elements ? elements.pop() : null
+    if (element) {
+      for (let dataId in element.dataset) { delete element.dataset[dataId] }
+      element.removeAttribute('style')
+      if (className) {
+        element.className = className
+      } else {
+        element.removeAttribute('class')
+      }
+      while (element.firstChild) {
+        element.removeChild(element.firstChild)
+      }
+      this.freedElements.delete(element)
+    } else {
+      element = document.createElement(tagName)
+      if (className) {
+        element.className = className
+      }
+      this.managedElements.add(element)
+    }
+    return element
+  }
+
+  buildText (textContent) {
+    const elements = this.freeElementsByTagName.get('#text')
+    let element = elements ? elements.pop() : null
+    if (element) {
+      element.textContent = textContent
+      this.freedElements.delete(element)
+    } else {
+      element = document.createTextNode(textContent)
+      this.managedElements.add(element)
+    }
+    return element
+  }
+
+  freeElementAndDescendants (element) {
+    this.free(element)
+    element.remove()
+  }
+
+  freeDescendants (element) {
+    while (element.firstChild) {
+      this.free(element.firstChild)
+      element.removeChild(element.firstChild)
+    }
+  }
+
+  free (element) {
+    if (element == null) { throw new Error('The element cannot be null or undefined.') }
+    if (!this.managedElements.has(element)) return
+    if (this.freedElements.has(element)) {
+      atom.assert(false, 'The element has already been freed!', {
+        content: element instanceof window.Text ? element.textContent : element.outerHTML
+      })
+      return
+    }
+
+    const tagName = element.nodeName.toLowerCase()
+    let elements = this.freeElementsByTagName.get(tagName)
+    if (!elements) {
+      elements = []
+      this.freeElementsByTagName.set(tagName, elements)
+    }
+    elements.push(element)
+    this.freedElements.add(element)
+
+    for (let i = element.childNodes.length - 1; i >= 0; i--) {
+      const descendant = element.childNodes[i]
+      this.free(descendant)
+    }
+  }
+}