Desktop: Fixes #14663: Fix crash when rapidly closing secondary windows (#14702)

.eslintignore

@@ -563,6 +563,7 @@ packages/app-desktop/integration-tests/models/NoteEditorScreen.js
 packages/app-desktop/integration-tests/models/NoteList.js
 packages/app-desktop/integration-tests/models/SettingsScreen.js
 packages/app-desktop/integration-tests/models/Sidebar.js
+packages/app-desktop/integration-tests/multiWindow.spec.js
 packages/app-desktop/integration-tests/noteList.spec.js
 packages/app-desktop/integration-tests/pluginApi.spec.js
 packages/app-desktop/integration-tests/resizableLayout.spec.js
@@ -584,6 +585,7 @@ packages/app-desktop/integration-tests/util/setMessageBoxResponse.js
 packages/app-desktop/integration-tests/util/setSettingValue.js
 packages/app-desktop/integration-tests/util/test.js
 packages/app-desktop/integration-tests/util/waitForNextOpenPath.js
+packages/app-desktop/integration-tests/util/waitForNextWindowMatching.js
 packages/app-desktop/integration-tests/wcag.spec.js
 packages/app-desktop/main-html.js
 packages/app-desktop/main.js

.gitignore

@@ -536,6 +536,7 @@ packages/app-desktop/integration-tests/models/NoteEditorScreen.js
 packages/app-desktop/integration-tests/models/NoteList.js
 packages/app-desktop/integration-tests/models/SettingsScreen.js
 packages/app-desktop/integration-tests/models/Sidebar.js
+packages/app-desktop/integration-tests/multiWindow.spec.js
 packages/app-desktop/integration-tests/noteList.spec.js
 packages/app-desktop/integration-tests/pluginApi.spec.js
 packages/app-desktop/integration-tests/resizableLayout.spec.js
@@ -557,6 +558,7 @@ packages/app-desktop/integration-tests/util/setMessageBoxResponse.js
 packages/app-desktop/integration-tests/util/setSettingValue.js
 packages/app-desktop/integration-tests/util/test.js
 packages/app-desktop/integration-tests/util/waitForNextOpenPath.js
+packages/app-desktop/integration-tests/util/waitForNextWindowMatching.js
 packages/app-desktop/integration-tests/wcag.spec.js
 packages/app-desktop/main-html.js
 packages/app-desktop/main.js

packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/utils/useContextMenu.ts

@@ -113,6 +113,9 @@ const useContextMenu = (props: ContextMenuProps) => {
 	// It might be buggy, refer to the below issue
 	// https://github.com/laurent22/joplin/pull/3974#issuecomment-718936703
 	useEffect(() => {
+		const targetWindow = bridge().windowById(windowId);
+		if (!targetWindow) return ()=> {};
+
 		const isAncestorOfCodeMirrorEditor = (elem: Element) => {
 			for (; elem.parentElement; elem = elem.parentElement) {
 				if (elem.classList.contains(props.editorClassName)) {
@@ -179,8 +182,6 @@ const useContextMenu = (props: ContextMenuProps) => {
 			return getResourceIdFromMarkup(line.text, clickPos - line.from);
 		};
 
-		const targetWindow = bridge().windowById(windowId);
-
 		const showResourceContextMenu = async (resourceId: string, type: ResourceMarkupType) => {
 			const menu = new Menu();
 			const baseType = type === 'image' ? ContextMenuItemType.Image : ContextMenuItemType.Resource;

packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/utils/useContextMenu.ts

@@ -35,8 +35,10 @@ export default function(editor: Editor, plugins: PluginStates, dispatch: Dispatc
 	useEffect(() => {
 		if (!editor) return () => {};
 
-		const contextMenuItems = menuItems(dispatch);
 		const targetWindow = bridge().windowById(windowId);
+		if (!targetWindow) return () => {};
+
+		const contextMenuItems = menuItems(dispatch);
 
 		const makeMainMenuItems = async (element: Element) => {
 			let itemType: ContextMenuItemType = ContextMenuItemType.None;

packages/app-desktop/integration-tests/models/MainScreen.ts

@@ -7,6 +7,7 @@ import setFilePickerResponse from '../util/setFilePickerResponse';
 import NoteList from './NoteList';
 import { expect } from '../util/test';
 import ChangeAppLayoutScreen from './ChangeAppLayoutScreen';
+import waitForNextWindowMatching from '../util/waitForNextWindowMatching';
 
 export default class MainScreen {
 	public readonly newNoteButton: Locator;
@@ -64,6 +65,13 @@ export default class MainScreen {
 		await activateMainMenuItem(electronApp, /^(Preferences\.\.\.|Options)$/);
 	}
 
+	public async openNewWindow(electronApp: ElectronApplication) {
+		const pagePromise = waitForNextWindowMatching(/^Joplin -/, electronApp);
+
+		await activateMainMenuItem(electronApp, 'Open in new window');
+		return pagePromise;
+	}
+
 	public async search(text: string) {
 		const searchBar = this.page.getByPlaceholder('Search...');
 		await searchBar.fill(text);

packages/app-desktop/integration-tests/models/NoteEditorScreen.ts

@@ -4,7 +4,7 @@ import activateMainMenuItem from '../util/activateMainMenuItem';
 import EditorCodeDialog from './EditorCodeDialog';
 import setSettingValue from '../util/setSettingValue';
 
-export default class NoteEditorPage {
+export default class NoteEditorScreen {
 	public readonly codeMirrorEditor: Locator;
 	public readonly noteViewerContainer: Locator;
 	public readonly editorPluginFrame: Locator;
@@ -26,7 +26,8 @@ export default class NoteEditorPage {
 	private readonly containerLocator: Locator;
 
 	public constructor(private page_: Page) {
-		this.containerLocator = page_.locator('.rli-editor');
+		// .rli-editor is used in the main window, .note-editor-wrapper in secondary windows
+		this.containerLocator = page_.locator('.rli-editor, .note-editor-wrapper');
 		this.codeMirrorEditor = this.containerLocator.locator('.cm-editor');
 		this.richTextEditor = this.containerLocator.locator('iframe[title="Rich Text Area"]');
 		this.editorPluginFrame = this.containerLocator.locator('iframe[id^="plugin-view-"]');

packages/app-desktop/integration-tests/multiWindow.spec.ts

@@ -0,0 +1,31 @@
+import { test, expect } from './util/test';
+import MainScreen from './models/MainScreen';
+import NoteEditorScreen from './models/NoteEditorScreen';
+
+test.describe('multiWindow', () => {
+	// Disabled: This test often hangs when closing secondary windows (see https://github.com/laurent22/joplin/issues/14628):
+	test.fixme('should support quickly creating, then closing secondary windows', async ({ mainWindow, electronApp }) => {
+		const mainPage = await new MainScreen(mainWindow).setup();
+		await mainPage.createNewNote('Test');
+
+		const windows = [];
+		for (let i = 0; i < 4; i++) {
+			const window = await mainPage.openNewWindow(electronApp);
+
+			// Should load successfully
+			const screen = new NoteEditorScreen(window);
+			await screen.waitFor();
+
+			windows.push(window);
+		}
+
+		// Close them all, very quickly.
+		for (const window of windows) {
+			await window.close();
+		}
+
+		// Should not have crashed
+		await expect(await mainPage.noteEditor.contentLocator()).toBeVisible();
+	});
+});
+

packages/app-desktop/integration-tests/util/waitForNextWindowMatching.ts

@@ -0,0 +1,52 @@
+import { Second } from '@joplin/utils/time';
+import { ElectronApplication, Page } from '@playwright/test';
+
+const waitForNextWindowMatching = (titlePattern: RegExp, electronApp: ElectronApplication) => {
+	return new Promise<Page>((resolve, reject) => {
+		let timeout: NodeJS.Timeout|null = null;
+		const clearListenersAndTimeouts = () => {
+			if (timeout) {
+				clearTimeout(timeout);
+				timeout = null;
+			}
+			electronApp.off('window', onWindowAdded);
+			electronApp.off('close', onClose);
+		};
+
+		const onWindowAdded = async (page: Page) => {
+			const title = await page.title();
+			if (title.match(titlePattern)) {
+				clearListenersAndTimeouts();
+				resolve(page);
+			}
+		};
+		const onClose = () => {
+			clearListenersAndTimeouts();
+			reject(new Error('Target application closed.'));
+		};
+
+		electronApp.on('window', onWindowAdded);
+		electronApp.on('close', onClose);
+
+		timeout = setTimeout(async () => {
+			timeout = null;
+			clearListenersAndTimeouts();
+
+			const windowTitles = await getOpenWindowTitles(electronApp);
+			reject(new Error(`Opening a window timed out. Open window titles: ${JSON.stringify(windowTitles)}.`));
+		}, 30 * Second);
+	});
+};
+
+export default waitForNextWindowMatching;
+
+const getOpenWindowTitles = (electronApp: ElectronApplication) => {
+	const windows = electronApp.windows();
+	return Promise.all(windows.map(async w => {
+		try {
+			return await w.title();
+		} catch (error) {
+			return `(Error: ${error})`;
+		}
+	}));
+};