From 2e88d171759210b4e47aa17680d602b4faa046fa Mon Sep 17 00:00:00 2001 From: Ian MacLeod Date: Fri, 13 Jun 2014 14:12:27 -0700 Subject: [PATCH] Protect against infinite recursion in focusOverlay. An easy way to repro the infinite recursion is to create multiple _opened_ overlays, and dismiss them at the same time (toasts). Here's what's happening. Say there are two overlays (A, and B), that were dismissed at the same time - and that their transitions end within the same run loop tick: * `A.transitionend`: * Removed from `overlays`. * Calls `applyFocus`, which calls `focusOverlay`. * `focusOverlay` calls `B.applyFocus`: * `B.closed == false`, so `focusOverlay` is called again. * ...recurse. The main problem is that the overlay's state (`opened`) is set before the transition completes; but `focusOverlay` relies on it to determine what to do next. So, we defer work until after transitions are complete. --- core-overlay.html | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/core-overlay.html b/core-overlay.html index da72bf6..72d615b 100644 --- a/core-overlay.html +++ b/core-overlay.html @@ -302,6 +302,7 @@

Dialog

}, openedChanged: function() { + this.transitioning = true; this.ensureTargetSetup(); this.prepareRenderOpened(); // continue styling after delay so display state can change @@ -371,6 +372,7 @@

Dialog

if (e && e.target !== this.target) { return; } + this.transitioning = false; if (!this.opened) { this.resetTargetDimensions(); this.target.style.display = 'none'; @@ -437,7 +439,11 @@

Dialog

focusNode.focus(); } else { focusNode.blur(); - focusOverlay(); + if (currentOverlay() == this) { + console.warn('Current core-overlay is attempting to focus itself as next! (bug)'); + } else { + focusOverlay(); + } } }, @@ -637,7 +643,15 @@

Dialog

function focusOverlay() { var current = currentOverlay(); - if (current) { + // We have to be careful to focus the next overlay _after_ any current + // transitions are complete (due to the state being toggled prior to the + // transition). Otherwise, we risk infinite recursion when a transitioning + // (closed) overlay becomes the current overlay. + // + // NOTE: We make the assumption that any overlay that completes a transition + // will call into focusOverlay to kick the process back off. Currently: + // transitionend -> applyFocus -> focusOverlay. + if (current && !current.transitioning) { current.applyFocus(); } }