1520047 - context menu + bookmark removal in bookmarks library = leaked 1 window(s) until shutdown [url = chrome://browser/content/places/places.xul]

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Rob Wu [:robwu]]

Patches for bug 1419195 were backed out because of leaks in TV on debug builds.

I created a minimal test case, consisting of the following steps:

1. Open bookmarks Library.
2. Create new bookmark folder via the places API.
3. Open and close context menu.
4. Remove bookmark.
5. Close library window.

After these steps (automated test case below), I get the following leak messages:

0:12.26 ERROR TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_library_menu_leak.js | leaked 1 window(s) until shutdown [url = about:blank]
0:12.26 ERROR TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_library_menu_leak.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/places/places.xul]
0:12.26 INFO TEST-INFO | browser/components/places/tests/browser/browser_library_menu_leak.js | windows(s) leaked: [pid = 9604] [serial = 14], [pid = 9604] [serial = 13]
0:12.26 ERROR TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_library_menu_leak.js | leaked 1 docShell(s) until shutdown
0:12.26 INFO TEST-INFO | browser/components/places/tests/browser/browser_library_menu_leak.js | docShell(s) leaked: [pid = 9604] [id = {fbb9f854-668e-46ca-ac7b-5534ac3430bf}]

When I remove step 3 (opening and closing context menu) or step 4 (removing bookmark), the leaks do not occur. With both steps present, I usually see leaks being reported when the test is run (at least on Linux, debug builds).

Full test case:

browser/components/places/tests/browser/browser_library_menu_leak.js

/* -*- 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() {                                   
  let library = await promiseLibrary("BookmarksToolbar");                          
  let menu = library.document.getElementById("placesContext");                     
  let tree = library.document.getElementById("placesList");                        
                                                                                   
  let {guid} = await PlacesUtils.bookmarks.insert({                                
    title: "Example",                                                              
    type: PlacesUtils.bookmarks.TYPE_FOLDER,                                       
    parentGuid: "toolbar_____",                                                    
  });                                                                              
                                                                                   
  tree.selectItems([guid]);                                                        
                                                                                   
  let shown = BrowserTestUtils.waitForEvent(menu, "popupshown");                   
  synthesizeClickOnSelectedTreeCell(tree, {type: "contextmenu"});                  
  await shown;                                                                     
                                                                                   
  // Waiting for 5 seconds here seems to reduce the frequency of the leak,         
  // but the leak still occurs somehow.                                            
                                                                                   
  let hidden = BrowserTestUtils.waitForEvent(menu, "popuphidden");                 
  menu.hidePopup();                                                                
  await hidden;                                                                    
                                                                                   
  // Waiting for a few seconds here does not seem to have a significant impact. 
                                                                                   
  await PlacesUtils.bookmarks.remove({guid});                                      
                                                                                   
  Assert.ok(true, "Should be able to remove a bookmark after closing a menu."); 
                                                                                   
  await promiseLibraryClosed(library);                                             
});                                                                                

Add to browser/components/places/tests/browser/browser.ini:

[browser_library_menu_leak.js]

Then create a debug build (artifacts are fine) and run the test:

mach test browser/components/places/tests/browser/browser_library_menu_leak.js --verify

Comment 1

[Rob Wu [:robwu]]

Smaller test case; context menus do not appear to be relevant:

1. Open bookmarks Library.
2. Click on a (top-level?) bookmark folder in the left panel of the bookmarks library.
3. Close library window.

add_task(async function bookmark_leak_window() { 
  let library = await promiseLibrary("BookmarksToolbar");
  let tree = library.document.getElementById("placesList");
  tree.selectItems(["toolbar_____"]);

  await synthesizeClickOnSelectedTreeCell(tree, {type: "mousedown"});
  // ^ Populates the right panel, which causes a leaked window.

  Assert.ok(true, "Dummy assertion for test runner");

  await promiseLibraryClosed(library);
});

Upon clicking a "selected" tree cell, the right panel is populated:
This is called: https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/browser/components/places/content/places.js#296
and ultimately triggers https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/browser/components/places/content/tree.xml#297
and causes a leaked window.

Comment 2

[Rob Wu [:robwu]]

This is triggered by a race condition in the destruction of the <tree> elements.

places.xul has a left panel (#placesList) and a right panel (#placeContent).
If the left panel is destructed before the right panel, everything is fine.
However, if the right panel is destructed first, and the left panel destructer thereafter, the leak occurs. I haven't finished investigating why exactly this happens, but it seems correlated to the selection of tree nodes.

I found a way to get rid of the leak, either by clearing the selection or by suppressing selection events at the start of the destructor.

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1520047 - Fix memory leak in places trees

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).

Comment 4

[Rob Wu [:robwu]]

This bug was likely introduced by https://hg.mozilla.org/mozilla-central/rev/95804c2b56e4#l5.2 (part of bug 543444).

Try result for fix in comment 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9ea05630844aa257b1e6d9f684dd02565e9aa56

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10b57d0ee380
Fix memory leak in places trees r=mak

Comment 6

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/10b57d0ee380