Bug 1520047 - Fix memory leak in places trees r=mak

The places-tree destructor lacks a `result.removeObserver(this.view);`
call before the assignment to `result.root.containerOpen`, which is
causing a memory leak.

This patch fixes the leak by removing the `result.root.containerOpen`
assignment, since the correct logic already exists in the `setTree`
method of `PlacesTreeView`, which is called upon `this.view = null;`
(XULTreeElement::SetView -> nsTreeBodyFrame::SetView -> setTree).

Differential Revision: https://phabricator.services.mozilla.com/D17241

--HG--
extra : moz-landing-system : lando

browser/components/extensions/test/browser/browser_ext_contextMenus.js

@@ -651,11 +651,6 @@ add_task(async function test_organizer_contextmenu() {
     [leftTree, bookmarkFolderContextMenuExtension],
   ];
 
-  if (AppConstants.DEBUG) {
-    // Avoid intermittent leak - bug 1520047
-    tests.pop();
-  }
-
   for (let [tree, makeExtension] of tests) {
     let extension = makeExtension();
     await extension.startup();

browser/components/places/content/tree.xml

@@ -22,14 +22,6 @@
       ]]></constructor>
 
       <destructor><![CDATA[
-        // Break the treeviewer->result->treeviewer cycle.
-        // Note: unsetting the result's viewer also unsets
-        // the viewer's reference to our tree.
-        var result = this.result;
-        if (result) {
-          result.root.containerOpen = false;
-        }
-
         // Unregister the controllber before unlinking the view, otherwise it
         // may still try to update commands on a view with a null result.
         if (this._controller) {
@@ -40,6 +32,9 @@
         if (this.view) {
           this.view.uninit();
         }
+        // view.setTree(null) will be called upon unsetting the view, which
+        // breaks the reference cycle between the PlacesTreeView and result.
+        // See the "setTree" method of PlacesTreeView in treeView.js.
         this.view = null;
       ]]></destructor>
 

browser/components/places/content/treeView.js

@@ -1463,6 +1463,7 @@ PlacesTreeView.prototype = {
         // detach from result when we are detaching from the tree.
         // This breaks the reference cycle between us and the result.
         if (!aTree) {
+          // Balances the addObserver call from the load method in tree.xml
           this._result.removeObserver(this);
           this._rootNode.containerOpen = false;
         }

browser/components/places/tests/browser/browser.ini

@@ -72,6 +72,7 @@ skip-if = (verify && debug && (os == 'mac' || os == 'linux'))
 [browser_library_open_bookmark.js]
 [browser_library_panel_leak.js]
 [browser_library_search.js]
+[browser_library_tree_leak.js]
 [browser_library_views_liveupdate.js]
 [browser_library_warnOnOpen.js]
 [browser_markPageAsFollowedLink.js]

browser/components/places/tests/browser/browser_library_tree_leak.js

@@ -0,0 +1,22 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+
+"use strict";
+
+add_task(async function bookmark_leak_window() {
+  // A library window has two trees after selecting a bookmark item:
+  // A left tree (#placesList) and a right tree (#placeContent).
+  // Upon closing the window, both trees are destructed, in an unspecified
+  // order. In bug 1520047, a memory leak was observed when the left tree
+  // was destroyed last.
+
+  let library = await promiseLibrary("BookmarksToolbar");
+  let tree = library.document.getElementById("placesList");
+  tree.selectItems(["toolbar_____"]);
+
+  await synthesizeClickOnSelectedTreeCell(tree, {type: "mousedown"});
+  await promiseLibraryClosed(library);
+
+  Assert.ok(true, "Closing a window after selecting a node in the tree should not cause a leak");
+});
+