Closed Bug 1073250 Opened 5 years ago Closed 5 years ago

[Browser][APZ] Browser is unresponsive when multiple touch events occur

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla35
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: jschmitt, Assigned: botond, NeedInfo)

References

()

Details

(Keywords: regression, Whiteboard: [2.1-exploratory-2])

Attachments

(3 files)

Attached file log.txt
Description:
Browser is unresponsive when user scrolls and touches the screen causing two touch inputs.

Note:

Issue only occurs with APZ on.
   
Repro Steps:
1) Update a Flame device to BuildID: 20140925000204
2) Connect to a Data or Wifi Network
3) Open the Browser app
4) Touch the screen while scrolling
  
Actual:
The browser becomes unresponsive.
  
Expected: 
The user can interact with the browser, and is responsive.
  
Environmental Variables:
Device: Flame 2.1
BuildID: 20140925000204
Gaia: 8061ab487d42cbc49b329fd68b9ca90e0fe477e6
Gecko: e970bc96f8b5
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Notes:
Repro frequency: 100%
See attached: https://www.youtube.com/watch?v=RrXYgxIQgUw and logcat
Issue DOES repro on 2.2 Flame 319MB KK.

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20140925040206
Gaia: c5d2e2f4ebf5f370d6003517057dcd47493dec90
Gecko: 32acbe1d64dc
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Issue does NOT repro on 2.0 Flame 319MB KK.

Environmental Variables:
Device: Flame 2.0 
BuildID: 20140924183011
Gaia: 87ee41fcb3f9a14d7a8bb67f1dd7fd95a6bcd0f0
Gecko: b1cb27078909
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(dharris)
Whiteboard: [2.1-exploratory-2]
[Blocking Requested - why for this release]:

It seems very likely that the user will run into this bug if they are trying to zoom in on the awesome page. Nominating this bug as a 2.1 blocker for being a likely use case, and a blocker
blocking-b2g: --- → 2.1?
QA Whiteboard: [QAnalyst-Triage?]
Component: Gaia::Browser → Gaia::System::Browser Chrome
Flags: needinfo?(dharris)
Component: Gaia::System::Browser Chrome → Panning and Zooming
Product: Firefox OS → Core
I'm able to reproduce. Based on a cursory investigation I think what's happening is that the first finger scrolling puts stuff into overscroll, and then we don't enter the block at [1] when the second finger goes down. I suspect this is because the overscroll is actually higher up in the handoff chain, so that condition might need adjusting.

The simple STR I used are: load the browser (I cleared history from device settings and loaded a single page so that i had just one history entry), scroll down into overscroll with one finger, then add a second finger in a pinch motion. This gets the page stuck in overscroll.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=a8b326379790#628
Assignee: nobody → botond
QA Contact: pcheng
mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20140904140137
Gaia: de59e0c3614dd0061881fe284e9f2d74fa0d1d5d
Gecko: 1c0889637e60
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20140904140640
Gaia: de59e0c3614dd0061881fe284e9f2d74fa0d1d5d
Gecko: eb7169690808
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1c0889637e60&tochange=eb7169690808

Caused by Bug 1062437.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Caused by Bug 1062437? Can you take a look Botond?
Blocks: 1062437
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(botond)
The actual bug is caused by the code related to overscrolling (bug 1020045). The wrong layer tree that bug 1062437 fixed just masked the problem.

I'm investigating.
No longer blocks: 1062437
Flags: needinfo?(botond)
Attached patch FixSplinter Review
Kats' analysis in comment 3 was accurate, and the suggested fix works.
Attachment #8499931 - Flags: review?(bugmail.mozilla)
This gtest was trickier to write than I thought it would be. I had to introduce an ApzctmPan() function to allow us to get into the state we want to test, and in turn it required a different approach to overcoming the touch start tolerance than the one used by ApzcPan().
Attachment #8499933 - Flags: review?(bugmail.mozilla)
Attachment #8499931 - Flags: review?(bugmail.mozilla) → review+
Unrelated, but should the |new| at http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=e4165ab15b4e#2017 have () on it? Since it's a non-POD type with no default constructor it might end up with members not initialized correctly (according to http://stackoverflow.com/a/620402).
Comment on attachment 8499933 [details] [diff] [review]
bug1073250-gtest.patch

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

Kinda hacky, but ok for now. If we end up reusing that function anywhere else we should clean it up a bit if possible.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +2200,5 @@
> +  EXPECT_TRUE(rootApzc->IsOverscrolled());
> +
> +  // Put a second finger down.
> +  MultiTouchInput secondFingerDown(MultiTouchInput::MULTITOUCH_START, 0, TimeStamp(), 0);
> +  // Use the same touch identifier for the first touch (0) as ApzcPan(). (A bit hacky.)

s/ApzcPan/ApzctmPan/
Attachment #8499933 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> Unrelated, but should the |new| at
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/
> AsyncPanZoomController.cpp?rev=e4165ab15b4e#2017 have () on it? Since it's a
> non-POD type with no default constructor it might end up with members not
> initialized correctly (according to http://stackoverflow.com/a/620402).

I had a look at what the standard says about this:

If the parentheses are ommitted, the object is "default-initialized". For an object of class type, this means the default constructor is called. Here, OverscrollHandoffChain doesn't have a user-declared default constructor, so the compiler generates one which calls the default constructors of all the members - in this case, the vector<nsRefPtr<AsyncPanZoomController>>. This is correct behaviour for us.

If you put in the parentheses, the object is "value-initialized". This is slightly different in that if the class has a user-declared default constructor, that is called; if it doesn't, then rather than calling the compiler-generated default constructor, the object is "zero-initialized", which is basically the same thing except data members of primitive types are initialized to 0 rather than an indeterminate value.

So it doesn't matter for OverscrollHandoffChain, because it doesn't have any data members of primitive types. If it did, I would argue that it should have a user-declared default constructor that explicitly zero-initializes said members, so that we know the members always get a value 0 rather than relying on allocation via 'new' having a particular syntax.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> Kinda hacky, but ok for now. If we end up reusing that function anywhere
> else we should clean it up a bit if possible.

Do you mean sharing code with ApzcPan()? Or allowing the user to pass in the touch identifier to be used? Or do you have some other clean-up in mind?
(In reply to Botond Ballo [:botond] from comment #11)
> 
> I had a look at what the standard says about this:
> 
> [snip]

Cool, thanks for checking! Another dusty corner of C++...

(In reply to Botond Ballo [:botond] from comment #12)
> 
> Do you mean sharing code with ApzcPan()? Or allowing the user to pass in the
> touch identifier to be used? Or do you have some other clean-up in mind?

Mostly the code reuse bit if that's doable. For the record I like your scooped-pref approach more than the old "big delta" approach and it would be good to convert ApzcPan to use the pref as well.
https://hg.mozilla.org/mozilla-central/rev/a19ccad6a934
https://hg.mozilla.org/mozilla-central/rev/42904535d23a
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
blocking-b2g: 2.1? → 2.1+
Please request Aurora approval on these patches when you get a chance.
Flags: needinfo?(botond)
Comment on attachment 8499931 [details] [diff] [review]
Fix

Botond is away until next week so requesting approval for him...

Approval Request Comment
[Feature/regressing bug #]: overscroll
[User impact if declined]: In some cases (such as on the browser start screen) it's possible to get stuck in an overscroll state, which causes all touch events to get ignored until the process is killed
[Describe test coverage new/current, TBPL]: new gtest added to cover this problem
[Risks and why]: low risk of regressions; patch is pretty straightforward. Only affects B2G and metro
[String/UUID change made/needed]: none
Attachment #8499931 - Flags: approval-mozilla-aurora?
Flags: needinfo?(botond)
Attachment #8499931 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8499933 [details] [diff] [review]
bug1073250-gtest.patch

Josh,

can you please help verify this bug once it lands on 2.1? Thanks!
Attachment #8499933 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(jschmitt)
Keywords: verifyme
This bug is verified fixed on the Flame 2.1 (319mb) and the Flame 2.2 (319mb)


Flame 2.2 Master KK (319mb) (Full Flash)

Device: Flame 2.2
BuildID: 20141010040202
Gaia: 1036b544b7e102592bd9fab95cd9317329ac1293
Gecko: 50b689feab5f
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1 KK (319mb) (Full Flash)

Environmental Variables:
Device: Flame 2.1
BuildID: 20141010000201
Gaia: bc8eb493311c58f1f311a56b8b645b52bfbd2f71
Gecko: 72c13d8631ff
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Result: The user can interact with the browser, and is responsive
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
when using multiple touch events, user can interact with the browser, and is responsive
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.