1245068 - [e10s] Ctrl+mouse wheel to change zoom level doesn't work if web page is not focused

Status: VERIFIED FIXED

(Core :: DOM: Content Processes, defect, P1)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20160201030241
STR:
1. Open http://example.org/
2. Press Ctrl+F to open and focus Findbar
3. Place mouse pointer over the page content, hold Ctrl and rotate mouse wheel up and down

AR:  Tab crashes
ER:  Tab should change zoom level

Comment 1

[arni2033]

Sorry, the actual result is "No visible action"

AR:  No visible action
ER:  Tab should change zoom level

Comment 2

[Tracy Walker [:tracy]]

This is quite easily reproducible using steps from comment 0, in e10s. I get this content crash https://crash-stats.mozilla.com/report/index/6ccc4d4c-0936-4d9d-859d-4c8d32160202

Works as expected in a new non-e10s window.

Win 7, Nightly 47.0a1, 20160201030241 - FAIL (content crash)
Mac 10.9.5, Nightly 47.0a1, 20160202030232 - FAIL (content crash)
Ubuntu 14.04.2, Nightly	47.0a1, 20160202030232 - n/a (ctrl + mouse wheel doesn't do anything, in this scenario, in e10s nor non-e10s)

This only happens with mouse wheel control of zoom size. using keyboard (ctrl/cmd + +/-) shorts cuts produces expected results.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

The crash was fixed in bug 1244582, I believe. The ctrl+mousewheel code is not handled in APZ, so I'm not sure where this bug belongs. It is e10s-specific though, and I can repro even on DevEdition.

Comment 4

[Ting-Yu Chou [:ting] (away)]

Can not reproduce after commenting out this line:

  https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/dom/base/nsFocusManager.cpp#1669

It was added by bug 583976, https://hg.mozilla.org/mozilla-central/rev/238b9a9479ed.

Comment 5

[Ting-Yu Chou [:ting] (away)]

Do you know why are Activate/Deactivate (WindowRaised/WindowLowered) required for a remote browser when nsFocusManager::Focus() and nsFocusManager::Blur()? Thanks.

Comment 6

[Neil Deakin]

To indicate to the child process when it is focused or blurred. That isn't the problem here.

Likely, some code somewhere is checking if the window is active so that the zoom action doesn't apply when the window is in the background. This is correct, but should be checking this state in the parent process, or this state could be exposed in the focus manager.

Comment 7

[Ting-Yu Chou [:ting] (away)]

(In reply to Neil Deakin from comment #6)
> Likely, some code somewhere is checking if the window is active so that the
> zoom action doesn't apply when the window is in the background. This is

The checking is in EventStateManager::ChangeFullZoom():

https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/dom/events/EventStateManager.cpp#2049

In this case, EventStateManager::GetContentViewer() returns error because nsFocusManager::GetFocusedWindow() returns null. mFocusedWindow is cleared by RecvDeactivate() (WindowLowered()) when Blur() gets called:

https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/dom/base/nsFocusManager.cpp#780
https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/dom/base/nsFocusManager.cpp#1717

Child process gets RecvDeactivate() called when parent calls Blur(), but later when parent calls Focus() on the input field, child does not RecvActivate() because it can't QI to nsIFrameLoaderOwner:

https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/dom/base/nsFocusManager.cpp#1892-1893

Comment 8

[Ting-Yu Chou [:ting] (away)]

Both isElementinFocusedWindow and isElementInActiveWindow are true when the input field gets focused:

https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/dom/base/nsFocusManager.cpp#1200
https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/dom/base/nsFocusManager.cpp#1232

The problem seems to be:

a. How we check whether the new focused content belong to a remote browser, or
b. Should we SendDectivate() when Blur() in parent has aIsLeavingDocument false? Because RecvDeactivate() goes to WindowLowered() which calls Blur() with aIsLeavingDocument true.

Comment 9

[Neil Deakin]

> https://dxr.mozilla.org/mozilla-central/rev/
> 6adc822f5e27a55551faeb6c47a9bd8b0859a23b/dom/events/EventStateManager.
> cpp#2049

OK so that code is a little unusual. It tries to get the focused window but then effectively ignores it and just gets the root window a few lines down. It does however have the effect of breaking out when the window isn't active, except that it dumps warnings on the console (even in non-e10s!)

For non-e10s, it should just have just used something like mDocument->GetWindow()->GetPrivateRoot()

then compared it to nsIFocusManager::GetActiveWindow.

For e10s however, you would need to take that root window and, if it is a TabChild, check if TabChild::ParentIsActive returns true to see if the window is active.

Comment 10

[Ting-Yu Chou [:ting] (away)]

Findbar is attached to the browser container:

https://dxr.mozilla.org/mozilla-central/rev/77cead2cd20300623eea2416bc9bce4d5021df09/browser/base/content/tabbrowser.xml#185-188

Comment 11

[Ting-Yu Chou [:ting] (away)]

(In reply to Neil Deakin from comment #9)
> For non-e10s, it should just have just used something like
> mDocument->GetWindow()->GetPrivateRoot()
> 
> then compared it to nsIFocusManager::GetActiveWindow.
> 
> For e10s however, you would need to take that root window and, if it is a
> TabChild, check if TabChild::ParentIsActive returns true to see if the
> window is active.

I am not sure I understand this. Does that mean EventStateManager::GetContentViewer() should return error if the root window is a TabChild and TabChild::ParentIsActive() returns false?

The problem from what I understand now is when ctrl+f, nsFocusManager::SetFocus() triggers Blur() for the browser element and Focus() for the input element. Blur() will SendDeactivate() to the child process and clear the mFocusedWindow, but Focus() will not SendActivate(). That's why EventStateManager::GetContentViewer() returns error and EventStateManager::ChangeFullZoom() does nothing.

Comment 12

[Ting-Yu Chou [:ting] (away)]

Attachment: wip

Tried to fix it as comment 9, I am not sure I am doing it correctly though it fixes the issue.

I guess you meant to skip GetFocusedWindow() for both e10s and non-e10s cases and just get root window by mDocument->GetWindow()->GetPrivateRoot()?

Comment 13

[Neil Deakin]

> I am not sure I understand this. Does that mean
> EventStateManager::GetContentViewer() should return error if the root window
> is a TabChild and TabChild::ParentIsActive() returns false?

It shouldn't return an error at all in those cases. Not having a focused window is a normal state, not an error state.


> The problem from what I understand now is when ctrl+f,
> nsFocusManager::SetFocus() triggers Blur() for the browser element and
> Focus() for the input element. Blur() will SendDeactivate() to the child
> process and clear the mFocusedWindow, but Focus() will not SendActivate().

Are you sure? Do you have some steps I could use here to look?

Comment 14

[Neil Deakin]

Comment on attachment 8748069 [details]
wip

># HG changeset patch
># User Ting-Yu Chou <[e-mail]>
># Date 1462258332 -28800
>#      Tue May 03 14:52:12 2016 +0800
># Node ID 237a0a675532a08edc014cb585e12f9e891ea2a4
># Parent  77cead2cd20300623eea2416bc9bce4d5021df09
>[mq]: bug-1245068
>
>diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
>--- a/dom/events/EventStateManager.cpp
>+++ b/dom/events/EventStateManager.cpp
>@@ -13,16 +13,17 @@
> #include "mozilla/MathAlgorithms.h"
> #include "mozilla/MouseEvents.h"
> #include "mozilla/TextComposition.h"
> #include "mozilla/TextEvents.h"
> #include "mozilla/TouchEvents.h"
> #include "mozilla/dom/ContentChild.h"
> #include "mozilla/dom/DragEvent.h"
> #include "mozilla/dom/Event.h"
>+#include "mozilla/dom/TabChild.h"
> #include "mozilla/dom/TabParent.h"
> #include "mozilla/dom/UIEvent.h"
> 
> #include "ContentEventHandler.h"
> #include "IMEContentObserver.h"
> #include "WheelHandlingHelper.h"
> 
> #include "nsCOMPtr.h"
>@@ -2010,21 +2011,27 @@ EventStateManager::GetContentViewer(nsIC
> {
>   *aCv = nullptr;
> 
>   nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>   if(!fm) return NS_ERROR_FAILURE;
> 
>   nsCOMPtr<mozIDOMWindowProxy> focusedWindow;
>   fm->GetFocusedWindow(getter_AddRefs(focusedWindow));
>-  if (!focusedWindow) return NS_ERROR_FAILURE;
>-
>-  auto* ourWindow = nsPIDOMWindowOuter::From(focusedWindow);
>-
>-  nsCOMPtr<nsPIDOMWindowOuter> rootWindow = ourWindow->GetPrivateRoot();
>+
>+  nsCOMPtr<nsPIDOMWindowOuter> rootWindow
>+  if (focusedWindow) {
>+    rootWindow = nsPIDOMWindowOuter::From(focusedWindow)->GetPrivateRoot()

Does GetContentViewer get called from the EventStateManager associated with the window where the wheel event occurred? I would expect that to be the case. If so, then you don't need the focused window at all (which, for example, could be in a window on another screen) mDocument->GetWindow()->GetPrivateRoot() will return the toplevel window for that window.


>+  } else if (XRE_IsContentProcess()) {
>+    rootWindow = mDocument->GetWindow()->GetPrivateRoot();
>+    TabChild* tabChild = TabChild::GetFrom(rootWindow);
>+    if (tabChild && !tabChild->ParentIsActive()) {
>+      return NS_ERROR_FAILURE;

You don't need to check for XRE_IsContentProcess(), and instead assume that if a TabChild is null that it isn't a child process.

Comment 15

[Ting-Yu Chou [:ting] (away)]

(In reply to Neil Deakin from comment #13)
> > The problem from what I understand now is when ctrl+f,
> > nsFocusManager::SetFocus() triggers Blur() for the browser element and
> > Focus() for the input element. Blur() will SendDeactivate() to the child
> > process and clear the mFocusedWindow, but Focus() will not SendActivate().
> 
> Are you sure? Do you have some steps I could use here to look?

Quite sure, just follow the STR of the bug description.

Comment 16

[Neil Deakin]

Well that's correct then. The findbar should be focused after step 3 and not the content.

Comment 17

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v1

Comment 18

[Neil Deakin]

Comment on attachment 8749518 [details] [diff] [review]
patch v1

>+    nsCOMPtr<mozIDOMWindowProxy> activeWindow;
>+    fm->GetActiveWindow(getter_AddRefs(activeWindow));
>+    if (rootWindow != activeWindow) return NS_ERROR_FAILURE;
>+  } else {
>+    if (!tabChild->ParentIsActive()) return NS_ERROR_FAILURE;
>+  }
> 
Both of these should return NS_OK as not having a focused window isn't an error. Otherwise, this will dump errors on the console when NS_ENSURE_SUCCESS is then invoked by the caller.

Comment 19

[Ting-Yu Chou [:ting] (away)]

(In reply to Neil Deakin from comment #18)
> Both of these should return NS_OK as not having a focused window isn't an
> error. Otherwise, this will dump errors on the console when
> NS_ENSURE_SUCCESS is then invoked by the caller.

Then a null checking in the caller is going to be needed, both ChangeTextSize() and ChangeFullZoom() expect |cv| a valid pointer after NS_ENSURE_SUCCESS.

Also does that mean when GetContentViewer() returns OK and a null pointer, ChangeTextSize() and ChangeFullZoom() will return OK?

Thank you for your help and reviewing. :)

Comment 20

[Neil Deakin]

> Then a null checking in the caller is going to be needed, both
> ChangeTextSize() and ChangeFullZoom() expect |cv| a valid pointer after
> NS_ENSURE_SUCCESS.

Yes, just add a null-check.
 
> Also does that mean when GetContentViewer() returns OK and a null pointer,
> ChangeTextSize() and ChangeFullZoom() will return OK?

Yes.

Comment 21

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v2

Addressed review comments. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ebe7b8635c0

Comment 22

[Ting-Yu Chou [:ting] (away)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=717fb8de99e4

Comment 23

[Ting-Yu Chou [:ting] (away)]

Attachment: patch v3

Fixed failed try cases.

Comment 24

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #22)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=717fb8de99e4

Try looks good.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a95651b07928

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a95651b07928

Comment 27

[Jim Mathies [:jimm]]

ting, is this fix worth uplifting to 48?

Comment 28

[Ting-Yu Chou [:ting] (away)]

I think no, because I expect there won't be a big portion of users that run into this bug. But if you want, we need to take bug 1274888 as well, because my patch here lacks a null checker.

Comment 29

[Chris Peterson [:cpeterson]]

wontfix for 49 based on Ting's comment 28.

Comment 30

[Camelia Badau [:cbadau], Desktop QA]

I've reproduced the initial issue on Nightly 47.0a1 (02-02-2016). 

Verified fixed on Windows 7 x64 and Mac OSX 10.9.5 using Firefox 49 Beta 5 (buildID: 20160818050015) with/without e10s.