================================================================================================ PROPOSAL - [MINOR] functional - p2j.sso_reauth.js handleSsoReauth The expiry path leaves the IdP popup open as an orphan ================================================================================================ REVIEW FINDING (abridged) "The expiry branch no longer calls cleanupReauthUi, which the deleted showSessionExpiredPage did as its first statement (await cleanupReauthUi(true)), so the reauthPopup.close() it performs is skipped [...] popupCloseCheckInterval and reauthMessageListener die with the page but the popup window does not, and the reloaded page holds no reference to it." FILES src/com/goldencode/p2j/ui/client/driver/web/res/p2j.sso_reauth.js ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ cleanupReauthUi (p2j.sso_reauth.js:436) is the only place that closes the popup: if (reauthPopup && !reauthPopup.closed) { reauthPopup.close(); } reauthPopup = null; The deleted showSessionExpiredPage called it as its first statement, so every pre-change expiry path closed the popup. The replacement branch reaches window.location.reload() with no cleanup call of any kind. The grace period does not prevent this. The guard is if (reauthInFlight() && (Date.now() - reauthDeadline) < REAUTH_GRACE_MS) whose second conjunct is time-bounded, so once 10 s have elapsed past the deadline the branch falls through regardless of reauthInFlight() still being true from an open popup. The tick then reloads with reauthPopup non-null and unclosed. A popup opened by window.open is an independent browsing context; reloading the opener does not close it. reauthPopup is module-private and referenced nowhere else, and the reloaded page starts with reauthPopup = null, so nothing can ever close it afterwards. The user is left with a stray window sitting on the IdP page. Concrete trigger: the user clicks Re-login, the IdP popup opens, and they abandon it without closing it - a realistic outcome, since the popup is where they would go if they could not remember their credentials. ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ 2.1 Close the popup before navigating --------------------------------------- The full cleanupReauthUi is async and removes the overlay, which is not wanted here - the overlay should stay visible until the page actually goes away, and awaiting a dialog close before a navigation invites the same cancelled-navigation problem as proposal-issue-minor-functional-3. A synchronous, targeted close is the right size: countdownDiv.textContent = 'Confirming sign-in...'; + // the popup is an independent browsing context and would survive this navigation with + // nothing left holding a reference to it + if (reauthPopup && !reauthPopup.closed) + { + reauthPopup.close(); + } + reauthPopup = null; p2j.socket.beginInternalReload(); window.location.reload(); Placed before beginInternalReload so that a cancelled navigation still leaves the popup closed - the popup is dead either way at this point, since the deadline plus grace has passed. 2.2 Alternative: extract the popup teardown --------------------------------------------- If the duplication with cleanupReauthUi is unwelcome, extract those five lines: + /** Close the re-login popup if one is open. Synchronous, unlike cleanupReauthUi. */ + function closeReauthPopup() + { + if (reauthPopup && !reauthPopup.closed) + { + reauthPopup.close(); + } + reauthPopup = null; + } and call it from both the expiry branch and cleanupReauthUi. There are already two further inline copies of the same three lines at :346 and :360 (the success and failure message handlers), so the extraction pays for itself four times over. This is the better change if the file is being touched anyway. ------------------------------------------------------------------------------------------------ 3. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-minor-functional-2 introduces p2j.socket.beginInternalReload, shown in the hunk above. If that proposal is not taken, drop that one line; this fix is independent of it. proposal-issue-minor-functional-3 reorders the same branch (reloadRequested guard). If both are taken, the popup close belongs inside the "first time through" path, not on every tick. ------------------------------------------------------------------------------------------------ 4. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: negligible. window.close() on an already-closed or never-opened window is guarded, and the popup is unusable at this point by definition. One caveat: a popup that navigated cross-origin to the IdP may not be closeable by the opener in every browser (window.close() is honoured for script-opened windows, but a cross-origin document can restrict what the opener may do). The call is best-effort; where it is refused the outcome is today's behaviour, no worse. Verification 1. Trigger SSO re-auth, click Re-login, and leave the IdP popup open without signing in. Wait out the countdown plus REAUTH_GRACE_MS. Expected: the popup closes as the opener reloads. Pre-fix it remains open on the IdP page. 2. Same, but sign in successfully - confirm the existing success path (which closes the popup at :346) is unchanged. 3. Same, but close the popup manually before the deadline - confirm no error from closing an already-closed window. 4. Confirm the Logout button path still closes the popup via cleanupReauthUi(true).