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'); + }); + }); });