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)
Tracking
()
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: robwu, Assigned: robwu)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
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:
- Open bookmarks Library.
- Create new bookmark folder via the places API.
- Open and close context menu.
- Remove bookmark.
- 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
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Smaller test case; context menus do not appear to be relevant:
- Open bookmarks Library.
- Click on a (top-level?) bookmark folder in the left panel of the bookmarks library.
- 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.
Assignee | ||
Comment 2•6 years ago
|
||
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 | ||
Comment 3•6 years ago
|
||
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).
Assignee | ||
Comment 4•6 years ago
|
||
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 6•6 years ago
|
||
bugherder |
Description
•