Closed Bug 1446904 Opened 6 years ago Closed 6 years ago

nsBaseDragService::InvokeDragSession function - freeze when using touch input

Categories

(Core :: Widget: Win32, defect, P2)

Product:
Core
Component:
Widget: Win32
Version:
58 Branch
Platform:
All
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla61
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 + disabled
firefox60 + verified
firefox61 + verified

People

(Reporter: marcin.miodek, Assigned: johannh)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1446904 - Do not generate drag gestures from synthesized/mouse events while in touch drag mode.
59 bytes, text/x-review-board-request
kats
: review+
smaug
: review+
RyanVM
: approval-mozilla-beta+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180315233128
Firefox for Android

Steps to reproduce:

Reporting based on forum thread: https://support.mozilla.org/en-US/questions/1209873
Issue has been identified as hang in nsBaseDragService::InvokeDragSession function by philipp.


Lenovo Yoga 2 10 Tablet (1051f) - Bad performance when using touch screen, good performance when using mouse+keyboard. Firefox 59.0.1.

MAIN ISSUE: When using touch display as input method, the performance in Firefox (59.0.1) is very bad - there are multiple freezes and various other issues. The issues go away when using an external mouse+keyboard via USB.

FURTHER DETAILS: The most frustrating issues appear when: 
-switching between various open tabs in FF 
-closing and opening new tabs 
-right clicking on a tab > selecting "pin tab" - instead of pinning the tab, FF opens the website in a new window - it can be then dragged-into the original FF window, and pinned without problem 
-The general performance seems very bad and not what you would expect it to be

WHAT HELPS: 
-Connecting a mouse+keyboard through USB - then the performance is what I would expect from this hardware

VIDEOS SHOWING THE ISSUE: 
1) Recording showing bad performance when using touch input: 
https://www.youtube.com/watch?v=mq1QYrXUlYE 
Note how clicking on the "multitasking" button at the bottom of the screen (I don't know the real name) interrupts the freeze, and then performance is back to normal... for some time.

2) Freeze when using touch input: 
https://www.youtube.com/watch?v=_4csda5Kdl0 
What this video is showing: 
-touching the display - everything is frozen, tablet does not react to touches 
-then the physical mouse is moved (just moved! no clicking!) 
-you can then see sudden "update" of what was done using touch input, and everything returns to normal

3) Another instance of the same thing as in point 2: 
https://www.youtube.com/watch?v=9cHMTv0A2gE

4) Recording showing good performance when using mouse+keyboard as input devices: 
https://www.youtube.com/watch?v=2czwhvmdcAM


Captured files:
https://drive.google.com/open?id=1L8ug5arwgFnUgro2HVmUfULredoRW6l2

right-click-freeze.gz  - triggered the freeze by right clicking (touch and hold) on an opened tab. Waited a few seconds, then moved the physical mouse to fix the freeze. Then captured the profile.

right-click-freeze2.gz - same as above

pin-tab-opens-in-new-window.gz - opened new tab, right clicked (touch and hold), selected "pin tab". Instead of pinning, the website opened in a new window.

This is identified as a problem with Firefox only. Other browsers and Windows in general work perfectly fine on the same device. 




Actual results:

1) Freezes when using touch display, mostly when touch-and-hold'ing in tabs to bring up the right-click menu.
2) Tabs open in a new window instead of being pinned.


Expected results:

1) Fluid work with touch screen
2) Tabs being pinned.
Any feedback here? :/
hi, could you find someone to look into the three captured profiles in the google drive link?
Flags: needinfo?(past)
(In reply to [:philipp] from comment #2)
> hi, could you find someone to look into the three captured profiles in the
> google drive link?

Is this a question to me? How could I find someone like that?
no, sorry for the confusion - it's a question meant for the triage owner of this category in bugzilla.
I'm tagging the bug for performance triage, but unfortunately, there are no symbols in the profile. Did the symbolication process not finish before saving? If you could post direct links to perf-html.io with a (shared) profile that has the proper symbols it would be much more useful in diagnosing this.
Component: Untriaged → Event Handling
Flags: needinfo?(past)
Product: Firefox → Core
Whiteboard: [qf][fxperf]
(In reply to Panos Astithas [:past] (please ni?) from comment #5)
> I'm tagging the bug for performance triage, but unfortunately, there are no
> symbols in the profile. Did the symbolication process not finish before
> saving? If you could post direct links to perf-html.io with a (shared)
> profile that has the proper symbols it would be much more useful in
> diagnosing this.

I will redo the tests today. Didn't know that you have to wait for symbols to load.
If you have any further hints how to report properly, please do share them. I am not familiar with reporting Firefox bugs.
"Waiting for symbol tables for library xul.pdb..." is taking a lot of time. Is it normal? (over 15 minutes I guess)
Here is the link where the freeze caused by touch screen should be visible:
https://perfht.ml/2HP4aFh
Hey jimm, it looks like (even from the unsymbolicated profile) that we're blocked on some OLE system call here... do you have any idea what might be happening here? (Since this sounds pretty bad)
Component: Event Handling → Widget: Win32
Flags: needinfo?(jmathies)
Whiteboard: [qf][fxperf] → [qf:p1][qf:f64][fxperf]
(In reply to Marcin from comment #7)
> "Waiting for symbol tables for library xul.pdb..." is taking a lot of time.
> Is it normal? (over 15 minutes I guess)

It's normal for it to take a few minutes (15 minutes seems long, though).

Unfortunately, your latest profile is still missing symbols for some reason. But at the lowest level, there are enough symbols to be useful here, I think!

jimm, here's a profile that's zoomed to the key section:
https://perfht.ml/2unrJDj

That shows that the Main Thread is spending 2575ms in the OS "DoDragDrop" method (and nearly all of that time is in "CDragOperation::RuntimeClassInitialize" calling into "KiFastSystemCallRet", whatever that means.
another user at https://support.mozilla.org/en-US/questions/1211335 is reporting the same issue with a surface pro device (unfortunately the profile there is unsymbolized again).
i have also seen it on my own device: https://perfht.ml/2GV3aA9
i can somewhat reliably reproduce the issue on a win10 touchscreen device by opening two new tabs and then very quickly tapping on their tab titles to switch back and forth between them (using both hands or two fingers to be quick).

the problem seems to have regressed in firefox 58 by bug 1362065.
Blocks: 1362065
status-firefox59: --- → affected
status-firefox60: --- → affected
status-firefox61: --- → affected
status-firefox-esr52: --- → unaffected
Keywords: regression
Version: 59 Branch → 58 Branch
there are various duplicate or similar sounding bugs on file for this as well: bug 1430724, bug 1436879, bug 1439417, bug 1441239, bug 1447738
Hm, I can look into this when I have my touchscreen laptop on Tuesday, but this might not be isolated to the touchdownstartsdrag hack we added in bug 1362065 (we have a similar mechanism for double-tapping as well), so kats may know more about this than me...

Thanks for finding this!
Flags: needinfo?(jhofmann)
Flags: needinfo?(bugmail)
Flags: needinfo?(jmathies)
Priority: -- → P2
OS: Unspecified → Windows
Hardware: Unspecified → All
Windows 10 Pro
Surface Pro 2017 8GB/256Gb RAM
Leaving this for platform/[qf] work for now, as it doesn't seem to be a frontend issue.
Whiteboard: [qf:p1][qf:f64][fxperf] → [qf:p1][qf:f64]
according to https://www.reddit.com/r/firefox/comments/88kq0w/firefox_ui_freezing_when_using_touchscreen/ the same symptoms are present on linux as well.
Based on the information so far it seems like we're entering a drag session during a tap, and then getting stuck inside that drag session. Starting the drag session on touchdown was the intended effect of bug 1362065, so the problem must be exiting the drag session properly.

Johann, were you able to reproduce this? If so we should find out why we're not hitting https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/dom/events/EventStateManager.cpp#612 to exit the drag once the finger is lifted.
Flags: needinfo?(bugmail)
I'm not sure this addresses my issue. My issue is with opening ("+") and then closing ("-") tabs . After so many tries (often just one or 2 sequences) closing a recently opened tab causes a freeze. Once Firefox gets a refresh (by opening another external window), all clicks that were executed on the frozen Firefox get processed in order.

This is easily reproducible on my 2017 Surface Pro running Windows 10 Pro. The issue does not occur with a mouse. 

The problem was so bad I had to switch to Chrome. I will return to Firefox once this problem is fixed. Developers can PM me if they wish for me to test.
Just a quick update that I can reproduce this (I think) and will look into it more tomorrow, after my build finishes. I agree that it sounds most interesting to find out whether we hit the line mentioned in comment 19 and stop dragging...

From my testing (and since it reproduces on Linux according to some people) I think bug 1362065 is not actually ultimately responsible for this. I'll try to verify that tomorrow as well by backing it out locally.
Comment on attachment 8965388 [details]
Bug 1446904 - Do not generate drag gestures from synthesized/mouse events while in touch drag mode.

https://reviewboard.mozilla.org/r/234132/#review239748

::: dom/events/EventStateManager.cpp:726
(Diff revision 1)
>      // a bookmark, for example).  If this is the case, however, we know
>      // that ClearFrameRefs() has been called and it cleared out
>      // |mCurrentTarget|. As a result, we should pass |mCurrentTarget|
>      // into UpdateCursor().
> +    if (!mInTouchDrag) {
> -    GenerateDragGesture(aPresContext, mouseEvent);
> +        GenerateDragGesture(aPresContext, mouseEvent);

nit, MozReview says there is 4 spaces indentation when it should be just 2.


(Oh, I think I see why pointer events are handled too - we want to prevent DOM dispatch for those if we start dragging. We may want to simplify this a bit though.)
Attachment #8965388 - Flags: review?(bugs) → review+
Comment on attachment 8965388 [details]
Bug 1446904 - Do not generate drag gestures from synthesized/mouse events while in touch drag mode.

https://reviewboard.mozilla.org/r/234132/#review239778

This seems safe enough
Attachment #8965388 - Flags: review?(bugmail) → review+
Comment on attachment 8965388 [details]
Bug 1446904 - Do not generate drag gestures from synthesized/mouse events while in touch drag mode.

https://reviewboard.mozilla.org/r/234132/#review239748

> nit, MozReview says there is 4 spaces indentation when it should be just 2.
> 
> 
> (Oh, I think I see why pointer events are handled too - we want to prevent DOM dispatch for those if we start dragging. We may want to simplify this a bit though.)

Ah, damn, that's what I get for using the VS editor instead of Vim...
Assignee: nobody → jhofmann
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(jhofmann)
I'm going to trigger a try run and land this tomorrow. Anyone who can reproduce the issue should feel free to download the builds from try to check them (I can post links later).

We should probably give it a bit of Nightly bake time until uplift, too. The issue seems serious enough for me to consider a dot release ride-along if everyone agrees that the fix is good. The patch does not seem very risky...
I've tried the Win64 build you posted and cannot reproduce the issue :)  Thanks.
i've tested the 32bit version and it seems to address the issue as well.
Hi guys, I am the OP here, can you please send me the download link for the new build (32-bit)? The thread is a bit confusing for me. :) I would check if it works fine on my device. Thanks.
Yeah, I'm also landing this now so that it will be in Nightly tomorrow.

Thanks for testing everyone!
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83a2d9c67bb6
Do not generate drag gestures from synthesized/mouse events while in touch drag mode. r=kats,smaug
Just tested this and all is fine. Awesome! Any ETA when this will be included in a stable release?
This seems to have fixed my problem as well.
https://hg.mozilla.org/mozilla-central/rev/83a2d9c67bb6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Please fill the uplift requests so that we are ready when we want to take them in beta.
Thanks!
Flags: needinfo?(jhofmann)
Comment on attachment 8965388 [details]
Bug 1446904 - Do not generate drag gestures from synthesized/mouse events while in touch drag mode.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1299286 and bug 1362065
[User impact if declined]: Severe hangs when using Firefox with a touchscreen on elements that support touch-dragging (such as the tab bar or the customization menu).
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: On a Windows touch device, open a lot of tabs and close them by tapping the [x] button on the tab. You should not experience intermittent long hangs.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: The only thing this patch does is add an if-condition that prevents code from running while we are inside a touch drag operation, so in the worst case touch dragging could break. Arguably touch usage is already quite broken and several people have confirmed that this patch works for them.
[String changes made/needed]: None
Flags: needinfo?(jhofmann)
Attachment #8965388 - Flags: approval-mozilla-beta?
(In reply to Johann Hofmann [:johannh] from comment #44)
> ...steps to reproduce

A much more repeatable (100% for me) test for me is to use touch or pen to attempt close a tab, but deliberately miss the "X" and click on a different part of the tab. This then renders the tab uncloseable with a subsequent click on the "X" until a screen refresh happens. This behaviour is not observed in the builds from this fix.
Flags: qe-verify+
Comment on attachment 8965388 [details]
Bug 1446904 - Do not generate drag gestures from synthesized/mouse events while in touch drag mode.

Approved for 60.0b11. Let's get this verified by QE as well.
Attachment #8965388 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've reproduced this bug on FF 59.0.1 (following the steps from comment 0), using a Dell XPS 13 with a touch display and running Windows 10 x64.

I can't reproduce anymore neither of the behaviors mentioned in this report on Beta 60.0b12 (20180412172954) and latest Nightly 61.0a1 (20180413100443).
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: fixed → verified
Flags: qe-verify+
Too late to fix in 59. Great that it will be fixed in the 60 release!
status-firefox59: affected → disabled
Performance Impact: --- → P1
Whiteboard: [qf:p1][qf:f64]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: