Closed Bug 1520047 Opened 11 months ago Closed 11 months ago

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

Categories

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

63 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox66 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

Details

Attachments

(1 file)

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
Priority: -- → P3

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.

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.

Assignee: nobody → rob
Status: NEW → ASSIGNED

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

Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/10b57d0ee380
Fix memory leak in places trees r=mak
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
You need to log in before you can comment on or make changes to this bug.