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

@@ -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
 	*

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',

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;
 	}
 };
 

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

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