Fix window id plumbing, and restore native windows across a reload - #435
Merged
Conversation
Three id-plumbing bugs, all in how a window's id is resolved rather than in what it is used for. `iframeContent` native windows baked the id the plugin registered into their synthesised render callback. `manager.open()` reassigns that id whenever an instance of the same baseId is already open (`chat` -> `chat-2`, e.g. opening the same window on a second virtual desktop), so the second instance registered its synthetic iframe, marked content ready, routed bridge messages, and dispatched window channels all under the first instance's id: its loading overlay never cleared, `connect()` dropped every message, and `Window.on()` never fired. The render now resolves the live instance id from the window root it mounts into. The same function's cleanup chain never ran. `onClose` was wrapped only when `cleanups.length` was non-zero, but that array is populated by the render callback, which doesn't run until `manager.open()` hydrates the window — so the check always saw an empty array. Every `iframeContent` window leaked a stale `_syntheticIframes` entry pointing at a detached iframe (which the next instance of that id would then find instead of its own) plus a `message` listener that stayed on `window` for the rest of the session. Gate the wrapper on `def.iframeContent` instead. Session restore replayed each saved window through `open()`, which matches on baseId. A session holding two instances of one page (`edit-php` + `edit-php-2`) collapsed to a single window on reload, and when the two had been navigated apart the URL-aware reuse check dragged the survivor to the second window's URL, losing the first page as well. Restore now goes through `openNew()`, which always constructs; `openNew()` in turn honours a free caller-supplied instance id verbatim so saved ids come back unchanged.
Opening OS Settings and hitting F5 lost the window. Same for Bug Report and for every window a plugin registers through `desktop_mode_register_window()`. Two places dropped them. `snapshot()` filtered out `native: true` before persisting, on the grounds that a native window's `render` callback is a JS closure and can't be serialized. That's true and beside the point — the closure never needed to be serialized, because every native window is addressable by id: the shell can ask its owner to reopen it. And even had the client sent them, the server would have discarded them: native windows carry a `#slug` marker instead of an admin URL, which fails the same-admin check that guards the restore path against foreign origins. Native entries now persist with `native: true`, and the server rebuilds the `#slug` marker from the sanitized id rather than storing the client's string — nothing navigates to it, so there is no reason to round-trip a client-controlled value through user meta. The same-admin gate is untouched for iframe windows. Restore reopens native windows through a single `openNativeWindowById` dispatcher in the shell (built-in openers first, then the native-window registry), and skips ids nothing answers to — a plugin deactivated since the session was saved. Because those openers build their own `manager.open()` config from the registry and have nowhere to put restore-time values, `restoreSession` stages the saved geometry, desktop, and state via the new `WindowManager.seedWindowRestoreState()`; the manager merges each entry into the first window claiming that id, then forgets it. A lifecycle-event barrier between opens keeps the saved stacking order and the focused-window restore deterministic despite the openers being fire-and-forget. Ephemeral windows (editor previews, whose URLs carry single-use nonces) remain the one category that is never persisted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four bugs in how a window's id is resolved and persisted. All live on trunk.
1.
iframeContentnative windows bake the registered idcreateRegisterWindowpassesdef.idinto the synthesised render callback, butmanager.open()reassigns the id whenever an instance of the same baseId is already open (chat→chat-2— e.g. opening the same registered window on a second virtual desktop).The second instance then registers its synthetic iframe, marks content ready, routes bridge messages, and dispatches window channels under the first instance's id:
connect( 'chat-2' )finds no iframe and drops every messageWindow.on( channel )never firesThe render now resolves the live instance id from the window root (
wp-window-<id>) it mounts into.2.
iframeContentcleanups never runonClosewas wrapped only whencleanups.lengthwas non-zero — but that array is populated by the render callback, which doesn't run untilmanager.open()hydrates the window. The check always saw an empty array, so the wrapper was never installed.Every
iframeContentwindow leaked a stale_syntheticIframesentry pointing at a detached iframe (which the next instance of that id would then find instead of its own) plus amessagelistener that stayed onwindowfor the rest of the session. Now gated ondef.iframeContent.3. Session restore collapses duplicate instances
restoreSessionreplayed each saved window throughopen(), which matches on baseId. A session holding two instances of one page (edit-php+edit-php-2, both baseIdedit-php) came back as one window, and when the two had been navigated apart the URL-aware reuse check dragged the survivor to the second window's URL — losing the first page too.Restore now goes through
openNew(), which always constructs.openNew()in turn honours a free caller-supplied instance id verbatim, so saved ids come back unchanged and anything keyed by window id (the saved focused-window pointer, per-window plugin state,wp.desktop.onWindow( id )) still lines up.4. Native windows don't survive a reload
Open OS Settings, hit F5, it's gone. Same for Bug Report and for every window registered through
desktop_mode_register_window().Two places dropped them:
snapshot()filtered outnative: truebefore persisting, because a native window'srendercallback is a JS closure and can't be serialized. True, and beside the point — the closure never needed serializing, since every native window is addressable by id and the shell can ask its owner to reopen it.#slugmarker instead of an admin URL, which fails the same-admin check guarding the restore path against foreign origins.Native entries now persist with
native: true. The server rebuilds the#slugmarker from the sanitized id rather than storing the client's string — nothing navigates to it, so there's no reason to round-trip a client-controlled value through user meta. The same-admin gate is untouched for iframe windows.Restore reopens them through a single
openNativeWindowByIddispatcher (built-in openers first, then the native-window registry) and skips ids nothing answers to — a plugin deactivated since the session was saved. Those openers build their ownmanager.open()config from the registry and have nowhere to put restore-time values, sorestoreSessionstages the saved geometry / desktop / state via the newWindowManager.seedWindowRestoreState(); the manager merges each entry into the first window claiming that id, then forgets it. A lifecycle-event barrier between opens keeps the saved stacking order and the focused-window restore deterministic despite the openers being fire-and-forget.Ephemeral windows (editor previews, whose URLs carry single-use nonces) remain the one category that's never persisted.
Tests
tests/vitest/native-window-iframe-content-id.test.ts— 5 tests, 4 fail without the fixtests/vitest/session-restore-duplicates.test.ts— 16 tests covering duplicate restore,openNewid allocation, native restore, and snapshot shapetests/phpunit/tests/desktopModeSession.php— 4 new: native entries survive sanitization, the marker is rebuilt from the id (a foreignurlon a native entry is discarded), iframe windows keep the strict same-admin gate, and thenativekey stays absent for iframe windowslint,typecheck,buildclean.Docs
architecture.md— new "What comes back, and how" under Session persistencejavascript-reference.md—openNew()'s id-allocation contract,seedWindowRestoreState()bridge-protocol.md—windowIdin the synthetic-iframe section is the live instance id, not the registered one