Bug 1520047 - Fix memory leak in places trees r=mak
authorRob Wu <rob@robwu.nl>
Wed, 23 Jan 2019 09:54:36 +0000
changeset 515084 10b57d0ee380c486e5d5e5a22b54f9d9da572577
parent 514974 1c2d27a85d17c93d89deeb1e0cd1bcfe2a7a97bf
child 515085 d646b0735ba3f0c8ff01bcc3745284dab9d95690
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1520047
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
browser/components/extensions/test/browser/browser_ext_contextMenus.js
browser/components/places/content/tree.xml
browser/components/places/content/treeView.js
browser/components/places/tests/browser/browser.ini
browser/components/places/tests/browser/browser_library_tree_leak.js
--- a/browser/components/extensions/test/browser/browser_ext_contextMenus.js
+++ b/browser/components/extensions/test/browser/browser_ext_contextMenus.js
@@ -646,21 +646,16 @@ add_task(async function test_organizer_c
   let leftTree = library.document.getElementById("placesList");
 
   let tests = [
     [mainTree, bookmarkContextMenuExtension],
     [mainTree, bookmarkFolderContextMenuExtension],
     [leftTree, bookmarkFolderContextMenuExtension],
   ];
 
-  if (AppConstants.DEBUG) {
-    // Avoid intermittent leak - bug 1520047
-    tests.pop();
-  }
-
   for (let [tree, makeExtension] of tests) {
     let extension = makeExtension();
     await extension.startup();
     let bookmarkGuid = await extension.awaitMessage("bookmark-created");
 
     tree.selectItems([bookmarkGuid]);
     let shown = BrowserTestUtils.waitForEvent(menu, "popupshown");
     synthesizeClickOnSelectedTreeCell(tree, {type: "contextmenu"});
--- a/browser/components/places/content/tree.xml
+++ b/browser/components/places/content/tree.xml
@@ -17,34 +17,29 @@
     <implementation>
       <constructor><![CDATA[
         // Force an initial build.
         if (this.place)
           this.place = this.place;
       ]]></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) {
           this._controller.terminate();
           this.controllers.removeController(this._controller);
         }
 
         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>
 
       <property name="controller"
                 readonly="true"
                 onget="return this._controller"/>
 
       <property name="disableUserActions"
--- a/browser/components/places/content/treeView.js
+++ b/browser/components/places/content/treeView.js
@@ -1458,16 +1458,17 @@ PlacesTreeView.prototype = {
     let hasOldTree = this._tree != null;
     this._tree = aTree;
 
     if (this._result) {
       if (hasOldTree) {
         // 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;
         }
       }
       if (aTree)
         this._finishInit();
     }
   },
--- a/browser/components/places/tests/browser/browser.ini
+++ b/browser/components/places/tests/browser/browser.ini
@@ -67,16 +67,17 @@ skip-if = (verify && debug && (os == 'ma
 [browser_library_left_pane_select_hierarchy.js]
 [browser_library_middleclick.js]
 [browser_library_new_bookmark.js]
 [browser_library_open_leak.js]
 [browser_library_openFlatContainer.js]
 [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_panelview_bookmarks_delete.js]
 [browser_paste_bookmarks.js]
 subsuite = clipboard
 [browser_paste_into_tags.js]
 [browser_paste_resets_cut_highlights.js]
new file mode 100644
--- /dev/null
+++ b/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");
+});
+