1041425 - Browser app crashes when sending touch event to nested child tab

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Kershaw Chang [:kershaw]]

Steps to reproduce:
1. Follow the steps in Bug 812403 Comment 3 and Bug 812403 Comment 4 to enable nested oop.
2. Open two browser tabs and load two random pages.
3. Click some random elements in the first tab.
4. Switch to the other tab and click any element.
5. Browser app crashes due to the assertion failure in TabParent::SendRealTouchEvent().

Comment 1

[Kershaw Chang [:kershaw]]

Root cause:
Since TabParent::TryCapture[1] is only called from nsAppShell, this function has no chance to be executed if the TabParent is in content process. It means that fast-path touch event dispatch cannot work in nested oop iframe.


[1]http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?from=TryCapture&case=true#1014

Comment 2

[Kershaw Chang [:kershaw]]

Attachment: Enable fast-path dispatch when sending touch event to nested oop iframe - v1

Hi Kan-Ru,

I am not sure who should I ask to review this patch. Maybe you can help or suggest a correct one?

Thanks.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8466978 [details] [diff] [review]
Enable fast-path dispatch when sending touch event to nested oop iframe - v1

Review of attachment 8466978 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I think we only have to change TabChild.

smaug, could you help review this patch?

::: dom/ipc/TabChild.cpp
@@ +534,5 @@
> +  if (TabParent* capturer = TabParent::GetEventCapturer()) {
> +    bool captured = capturer->TryCapture(event);
> +    if (captured) {
> +      return nsEventStatus_eConsumeNoDefault;
> +      }

nit: indent.

::: dom/ipc/TabParent.cpp
@@ +980,5 @@
> +    else {
> +      NS_WARNING("Fast-path dispatch is disabled.");
> +      sEventCapturer = nullptr;
> +      mEventCaptureDepth = 0;
> +    }

I don't think we need this change.

Comment 4

[Kershaw Chang [:kershaw]]

> 
> ::: dom/ipc/TabParent.cpp
> @@ +980,5 @@
> > +    else {
> > +      NS_WARNING("Fast-path dispatch is disabled.");
> > +      sEventCapturer = nullptr;
> > +      mEventCaptureDepth = 0;
> > +    }
> 
> I don't think we need this change.

Let me explain more why I made this change. Consider the following special case, a content process P has two grandchildren A and B.
+------------------+
|Content Process P |
|                  |
| +--------------+ |
| |              | |
| | Grandchild A | |
| |              | |
| +--------------+ |
|                  |
| +--------------+ |
| |              | |
| | Grandchild B | |
| |              | |
| +--------------+ |
|                  |
+------------------+
If I use two fingers to touch A and B simultaneously, P will receive two consecutive NS_TOUCH_EVENT events (one for A and the other one for B). After processing the first NS_TOUCH_EVENT event, sEventCapturer is A and mEventCaptureDepth in TabParent A is 1. The second NS_TOUCH_EVENT event goes to TabParent B, so we have sEventCapturer is A and mEventCaptureDepth in TabParent B is still 0. This makes the assertion fail in TabParent::SendRealTouchEvent().
I think the fast-path dispatch can not work in this special case. That's why I made this change.

Comment 5

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

Comment on attachment 8466978 [details] [diff] [review]
Enable fast-path dispatch when sending touch event to nested oop iframe - v1

 
>-    MOZ_ASSERT((!sEventCapturer && mEventCaptureDepth == 0) ||
>-               (sEventCapturer == this && mEventCaptureDepth > 0));
>-    // We want to capture all remaining touch events in this series
>-    // for fast-path dispatch.
>-    sEventCapturer = this;
>-    ++mEventCaptureDepth;
>+    if ((!sEventCapturer && mEventCaptureDepth == 0) ||
>+               (sEventCapturer == this && mEventCaptureDepth > 0)) {
Odd indentation.

But I don't understand this change.
If we have sEventCapturer, we should just dispatch all the events to that, IMHO.
(See also https://bugzilla.mozilla.org/show_bug.cgi?id=790454 )
And once the touch-session is done, sEventCapturer should be set to null.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Enable fast-path dispatch when sending touch event to nested oop iframe - v2

Ok, I will dig deeper to understand the case I've mentioned in comment 4.
However, would you please review the change in TabChild first?

Thanks.

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: [Final] Enable fast-path dispatch when sending touch event to nested oop iframe, r=smaug

Update the final patch includes:
1. Carry the reviewer's name
2. Rebase to the latest code

Try server:
https://tbpl.mozilla.org/?tree=Try&rev=f560e13d7b4a

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5caf65346c39

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/5caf65346c39