From 53bcd30a5fac0aaef3f54298ab788ea9c22505f2 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Tue, 25 Aug 2026 23:34:46 -0400 Subject: [PATCH] Don't report a window's observers as leaked while it's still closing A window is marked as closed before its unload handlers run, so the tab events fired from ZoteroPane.destroy() warned about every observer that the closing window hadn't torn down yet. The leak check now also requires the window to be detached from its docShell, and it covers functions. It caught an observer that outlived its window -- the sync-reminder API key observer, which is now unregistered on teardown. --- chrome/content/zotero/xpcom/notifier.js | 9 ++++ chrome/content/zotero/xpcom/prefs.js | 17 +++++++ .../zotero/xpcom/utilities_internal.js | 11 +++-- chrome/content/zotero/zoteroPane.js | 8 +++- test/tests/tabsTest.js | 46 +++++++++++++++++++ 5 files changed, 85 insertions(+), 6 deletions(-) diff --git a/chrome/content/zotero/xpcom/notifier.js b/chrome/content/zotero/xpcom/notifier.js index 2d7af1969d..44f48b24f1 100644 --- a/chrome/content/zotero/xpcom/notifier.js +++ b/chrome/content/zotero/xpcom/notifier.js @@ -110,6 +110,15 @@ Zotero.Notifier = new function () { }; + /** + * @return {String[]} - Ids of registered observers that belong to a closed window + */ + this.getLeakedObserverIDs = function () { + return Object.keys(_observers) + .filter(id => Zotero.Utilities.Internal.isObjectLeakingWindow(_observers[id].ref)); + }; + + /** * Trigger a notification to the appropriate observers * diff --git a/chrome/content/zotero/xpcom/prefs.js b/chrome/content/zotero/xpcom/prefs.js index 2629eec421..caf1fbf58e 100644 --- a/chrome/content/zotero/xpcom/prefs.js +++ b/chrome/content/zotero/xpcom/prefs.js @@ -512,6 +512,23 @@ Zotero.Prefs = new function () { } + /** + * @return {String[]} - Name of the observed pref for each registered observer that belongs to + * a closed window, with one entry per observer + */ + this.getLeakedObserverNames = function () { + var names = []; + for (let [name, handlers] of Object.entries(_observers)) { + for (let handler of handlers) { + if (Zotero.Utilities.Internal.isObjectLeakingWindow(handler)) { + names.push(name); + } + } + } + return names; + }; + + this.getVirtualCollectionState = function (type) { const prefKeys = { duplicates: 'duplicateLibraries', diff --git a/chrome/content/zotero/xpcom/utilities_internal.js b/chrome/content/zotero/xpcom/utilities_internal.js index 5eda717f7f..c259bcc6b2 100644 --- a/chrome/content/zotero/xpcom/utilities_internal.js +++ b/chrome/content/zotero/xpcom/utilities_internal.js @@ -2649,18 +2649,21 @@ Zotero.Utilities.Internal = { }, /** - * Check whether an object belongs to a closed window, and is therefore - * keeping it alive. + * Check whether an object or function belongs to a closed window, and is + * therefore keeping it alive. + * + * A window only counts as leaked once it's closed and detached from its + * docShell, which happens after its 'unload' handlers have run. * * @param {any} obj * @returns {boolean} */ isObjectLeakingWindow(obj) { - if (typeof obj !== 'object' || obj === null) { + if (obj === null || (typeof obj !== 'object' && typeof obj !== 'function')) { return false; } let global = Cu.getGlobalForObject(obj); - return global.constructor.name === 'Window' && global.closed; + return global.constructor.name === 'Window' && global.closed && !global.docShell; } }; diff --git a/chrome/content/zotero/zoteroPane.js b/chrome/content/zotero/zoteroPane.js index 0d7ab0b0ef..bf86cf5f4c 100644 --- a/chrome/content/zotero/zoteroPane.js +++ b/chrome/content/zotero/zoteroPane.js @@ -791,6 +791,9 @@ var ZoteroPane = new function () { if (_syncRemindersObserverID) { Zotero.Notifier.unregisterObserver(_syncRemindersObserverID); } + if (_apiKeyObserverID) { + Zotero.Notifier.unregisterObserver(_apiKeyObserverID); + } this.uninitContainers(); @@ -3279,9 +3282,10 @@ var ZoteroPane = new function () { var _syncRemindersObserverID = null; + var _apiKeyObserverID = null; this.initSyncReminders = function (startup) { - if (startup) { - Zotero.Notifier.registerObserver( + if (startup && !_apiKeyObserverID) { + _apiKeyObserverID = Zotero.Notifier.registerObserver( { notify: (event) => { // When the API Key is deleted we need to add an observer diff --git a/test/tests/tabsTest.js b/test/tests/tabsTest.js index b7ef45bb29..3d91882c0a 100644 --- a/test/tests/tabsTest.js +++ b/test/tests/tabsTest.js @@ -40,4 +40,50 @@ describe("Zotero_Tabs", function() { assert.notOk(tab.querySelector('img')); }); }); + + describe("Window teardown", function () { + it("should not leave observers registered after a window is closed", async function () { + this.timeout(60000); + let collection = await createDataObject('collection'); + let item = await createDataObject('item', { collections: [collection.id] }); + let notifierBefore = new Set(Zotero.Notifier.getLeakedObserverIDs()); + let prefsBefore = Zotero.Prefs.getLeakedObserverNames(); + + // Render the library item pane in a second window + let win2 = await loadZoteroPane(); + await selectCollection(win2, collection); + await win2.ZoteroPane.selectItem(item.id); + + // The window's observers are still registered while it tears down, and shouldn't be + // reported as leaked before it's actually gone + let leakedWhileUnloading = null; + win2.addEventListener('pagehide', () => { + leakedWhileUnloading = Zotero.Notifier.getLeakedObserverIDs() + .filter(id => !notifierBefore.has(id)); + }); + // An object that nothing unregisters, so that it starts reporting as leaked as soon + // as the window has finished tearing down + let root = win2.document.documentElement; + win2.close(); + await waitForCallback( + () => Zotero.Utilities.Internal.isObjectLeakingWindow(root), 50, 20 + ); + + assert.deepEqual( + leakedWhileUnloading, [], 'no observers are reported as leaked while unloading' + ); + let notifierAfter = Zotero.Notifier.getLeakedObserverIDs() + .filter(id => !notifierBefore.has(id)); + assert.isEmpty(notifierAfter, 'no notifier observers belong to a closed window'); + + let prefsAfter = Zotero.Prefs.getLeakedObserverNames(); + for (let name of prefsBefore) { + let index = prefsAfter.indexOf(name); + if (index != -1) { + prefsAfter.splice(index, 1); + } + } + assert.isEmpty(prefsAfter, 'no pref observers belong to a closed window'); + }); + }); });