1334572 - Focus manager touches multiple tabs synchronously

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

One of the main tasks in the Quantum DOM project is to ensure that each runnable only touches the content of one tab. We're running into a problem with the focus manager where it dispatches events to multiple tabs synchronously. Here's an example stack:
https://treeherder.mozilla.org/logviewer.html#?job_id=72417010&repo=try&lineNumber=2096

Here's an excerpt from the stack:

 8  libxul.so!nsFocusManager::Blur [nsFocusManager.cpp:31bf4daf154e : 1754 + 0x1a]
 9  libxul.so!nsFocusManager::WindowLowered [nsFocusManager.cpp:31bf4daf154e : 799 + 0x12]
10  libxul.so!nsFocusManager::RaiseWindow [nsFocusManager.cpp:31bf4daf154e : 2217 + 0x9]
11  libxul.so!nsFocusManager::SetActiveWindow [nsFocusManager.cpp:31bf4daf154e : 401 + 0xa]
12  libxul.so!nsGlobalWindow::FocusOuter [nsGlobalWindow.cpp:31bf4daf154e : 7252 + 0x1d]
13  libxul.so!mozilla::dom::WindowBinding::focus [WindowBinding.cpp:31bf4daf154e : 2011 + 0xb]

Basically, calling focus() on one window is synchronously causing a blur event to be sent to a different window. Neil, can you help figure out what to do here? The most obvious solution is to dispatch the focus and blur events in separate runnables. However, I'm not sure what would break based on that. Presumably we would want to dispatch the blur event first, followed by the focus event. Would any state get messed up in the focus manager because there would now be a window of time where no window is in focus? I don't know this code very well.

Comment 1

[Neil Deakin]

I'm not understanding what the issue is here, but when the focus changes we blur the old content first, fire the blur event in FocusBlurEvent (which inherits from Runnable), then we focus the new content and fire an event in a different FocusBlurEvent. In between, the blur and focus, no content is considered focused, but the window still is.

There is also two separate FocusInOutEvent objects for the focusout and focusin events.

As a note, the stack above suggests someone is calling window.focus() on a top-level window, and the special test mode preference is enabled. In the test mode, attempts to switch window order are ignored and simulated, so that tests can be run in a background window. Does the same issue occur when running with test mode disabled?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Neil Deakin from comment #1)
> I'm not understanding what the issue is here, but when the focus changes we
> blur the old content first, fire the blur event in FocusBlurEvent (which
> inherits from Runnable), then we focus the new content and fire an event in
> a different FocusBlurEvent. In between, the blur and focus, no content is
> considered focused, but the window still is.

Would it be okay to dispatch these events using AsyncEventDispatcher instead of AddScriptRunner? That way they would run in separate turns of the event loop no matter what.

> There is also two separate FocusInOutEvent objects for the focusout and
> focusin events.

Could I do the same thing for those (use AsyncEventDispatcher)?

> As a note, the stack above suggests someone is calling window.focus() on a
> top-level window, and the special test mode preference is enabled. In the
> test mode, attempts to switch window order are ignored and simulated, so
> that tests can be run in a background window. Does the same issue occur when
> running with test mode disabled?

I don't know. I'll check.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Making blur/focus more asynchronous in such way which is visible to the web pages is no go.
But sure, if we can detect that events are going to different tab groups, then we could use async dispatching. I guess the tab group gaining focus should get it synchronously (using script runner) but the one losing it could live with async blur.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Olli Pettay [:smaug] from comment #3)
> Making blur/focus more asynchronous in such way which is visible to the web
> pages is no go.
> But sure, if we can detect that events are going to different tab groups,
> then we could use async dispatching. I guess the tab group gaining focus
> should get it synchronously (using script runner) but the one losing it
> could live with async blur.

In that case the blur would happen after the focus. Is that okay?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

That could be problematic yes in some cases, but would break only our internal code (which we can fix), but making something async which currently isn't async from web page point of view is pretty much guaranteed to break pages.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: move Activate/Deactivate messages to PContent

This patch moves the Activate/Deactivate messages from PBrowser to PContent. This means that they won't be labeled with a particular TabGroup. The problem with these messages now is that, for example, calling Activate on window A can fire a blur event on window B. If A and B are in different TabGroups, then we will assert.

Eventually we'll need to split the blur event into a separate runnable so that we can label these messages. But this is a pretty rare message type, so it's not very important right now.

I also moved ParentActivated since it seems related. That one is a little more common, but not much.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: run raised/lowered functions in separate runnables in test mode

Neil pointed out that the assertions I was seeing in comment 0 are specific to test mode (sTestMode = true). It doesn't seem to break anything if I run the raising and lowering in a separate unlabeled runnable. So let's do that.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8834636 [details] [diff] [review]
move Activate/Deactivate messages to PContent

Why the methods in TabChild need to be virtual?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8834637 [details] [diff] [review]
run raised/lowered functions in separate runnables in test mode


>   if (sTestMode) {
>     // In test mode, emulate the existing window being lowered and the new
>-    // window being raised.
>-    if (mActiveWindow)
>-      WindowLowered(mActiveWindow);
>-    WindowRaised(aWindow);
>+    // window being raised. This happens in a separate runnable to avoid
>+    // touching multiple windows in the current runnable.
>+    nsCOMPtr<nsPIDOMWindowOuter> active(mActiveWindow);
>+    nsCOMPtr<nsPIDOMWindowOuter> window(aWindow);
>+    RefPtr<nsFocusManager> self(this);
>+    NS_DispatchToCurrentThread(NS_NewRunnableFunction([self, active, window] () -> void {
>+          if (active) {
>+            self->WindowLowered(active);
>+          }
>+          self->WindowRaised(window);
>+        }));
Some weird indentation here. Maybe something like
NS_DispatchToCurrentThread(
  NS_NewRunnableFunction([self, active, window] () -> void {
    if (active) {


tiny bit risky change, hopefully not too many marionette tests or such aren't broken.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8c605570c1b
Move Activate/Deactivate messages from PBrowser to PContent (r=smaug)
https://hg.mozilla.org/integration/mozilla-inbound/rev/100f3d6ec589
Dispatch WindowLowered/Raised events asynchronously in test mode (for marionette) (r=smaug)

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8c605570c1b
https://hg.mozilla.org/mozilla-central/rev/100f3d6ec589