Closed
Bug 1041425
Opened 10 years ago
Closed 10 years ago
Browser app crashes when sending touch event to nested child tab
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: kershaw, Assigned: kershaw)
References
Details
(Whiteboard: [nested-oop][FT:Stream3])
Attachments
(1 file, 2 obsolete files)
1.41 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
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().
Updated•10 years ago
|
Blocks: nested-oop
Updated•10 years ago
|
Whiteboard: [nested-oop][FT:Stream3]
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
QA Contact: kechang
Assignee | ||
Comment 2•10 years ago
|
||
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.
Attachment #8466978 -
Flags: review?(kchen)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → kechang
QA Contact: kechang
Comment 3•10 years ago
|
||
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.
Attachment #8466978 -
Flags: review?(kchen)
Attachment #8466978 -
Flags: review?(bugs)
Attachment #8466978 -
Flags: review+
Assignee | ||
Comment 4•10 years ago
|
||
>
> ::: 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•10 years ago
|
||
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.
Attachment #8466978 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 6•10 years ago
|
||
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.
Attachment #8466978 -
Attachment is obsolete: true
Attachment #8468205 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8468205 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 7•10 years ago
|
||
try |
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
Attachment #8468205 -
Attachment is obsolete: true
Attachment #8469863 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5caf65346c39
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5caf65346c39
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•