Closed
Bug 1073250
Opened 10 years ago
Closed 10 years ago
[Browser][APZ] Browser is unresponsive when multiple touch events occur
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
People
(Reporter: jschmitt, Assigned: botond, NeedInfo)
References
()
Details
(Keywords: regression, Whiteboard: [2.1-exploratory-2])
Attachments
(3 files)
110.75 KB,
text/plain
|
Details | |
3.87 KB,
patch
|
kats
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
5.65 KB,
patch
|
kats
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•10 years ago
|
||
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)
Keywords: regression,
regressionwindow-wanted
Updated•10 years ago
|
Component: Gaia::System::Browser Chrome → Panning and Zooming
Product: Firefox OS → Core
Comment 3•10 years ago
|
||
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
Blocks: apz-overscroll
Updated•10 years ago
|
QA Contact: pcheng
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
Caused by Bug 1062437? Can you take a look Botond?
Blocks: 1062437
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell) → needinfo?(botond)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
Kats' analysis in comment 3 was accurate, and the suggested fix works.
Attachment #8499931 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8499931 -
Flags: review?(bugmail.mozilla) → review+
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
(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?
Comment 13•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19ccad6a934 https://hg.mozilla.org/integration/mozilla-inbound/rev/42904535d23a
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a19ccad6a934 https://hg.mozilla.org/mozilla-central/rev/42904535d23a
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•10 years ago
|
Updated•10 years ago
|
blocking-b2g: 2.1? → 2.1+
Comment 16•10 years ago
|
||
Please request Aurora approval on these patches when you get a chance.
Flags: needinfo?(botond)
Comment 17•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8499933 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8499931 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•10 years ago
|
||
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+
Comment 19•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/cecf4403ba9d https://hg.mozilla.org/releases/mozilla-aurora/rev/cf80dc75fc51
Comment 20•10 years ago
|
||
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
Comment 21•10 years ago
|
||
when using multiple touch events, user can interact with the browser, and is responsive
Updated•10 years ago
|
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.
Description
•