Closed Bug 1061142 Opened 5 years ago Closed 5 years ago

[GAIA][Contacts]Scroll contact list very fast and stop suddenly by taping on the screen, the list item highlighted is not matching the tapped position/lisitem.

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1074691
Tracking Status
b2g-v2.0 --- affected
b2g-v2.1 --- affected
b2g-v2.2 --- affected

People

(Reporter: ashayb2g, Assigned: kats)

Details

(Whiteboard: [LibGLA,TD92131,QE1, B])

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140828030205

Steps to reproduce:

Have some good number of contacts stored in memory.
=>Go to Contacts app and scroll fast
=>Tap suddenly on the screen.


Actual results:

The list item highlighted is not matching the tapped position.


Expected results:

It should highlight the same item of tapped position.
Whiteboard: [LibGLA,TD92131,QE1, B]
Component: General → Gaia::Contacts
Somehow I cannot reproduce this issue here. Could you provide a video showing how to reproduce this issue?

Thanks

Vance
Flags: needinfo?(ashayb2g)
Attached video ScrollIssue.mp4
Hi,

PFA Scroll issue video.

Thanks
Flags: needinfo?(ashayb2g)
Attached image IMAG3060.jpg
Screenshots for this issue
ni Evelyn, Luke

Could you help to check this issue? I can show you how to reproduce this one if necessary

Thanks

Vance
Flags: needinfo?(lchang)
Flags: needinfo?(ehung)
Group: kddi-confidential
I feel it's not a gaia issue, looks like the touch event is calculated to a wrong position.
Flags: needinfo?(vwang)
Flags: needinfo?(lchang)
Flags: needinfo?(ehung)
I can find this bug like 1/10 or less reproduce rate now.
Since we have no code for Madai, I would like to see if we can find this bug in flame first.
Flags: needinfo?(vwang)
Hi Viral, this bug is also reproducible on Flame.


Hi Mike, could you please double confirm this ?


Thank you for your help.
Flags: needinfo?(mlien)
verify with Flame KK v180 with today's v2.0 gaia/gecko
It has the highlight incorrect problem but reproduce rate is much lower: 2/11

Gaia-Rev        6449cc35a8f0704d95acac374ba857bde4b86d6c
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4edd7f40672b
Build-ID        20140923160206
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  27
FW-Date         Thu Sep  4 14:59:02 CST 2014
Bootloader      L1TC10011800

I also check the touch event when problem happens and its position is correct
tap position to stop scroll:
/dev/input/event0: EV_SYN       SYN_REPORT           00000000            
/dev/input/event0: EV_ABS       ABS_MT_POSITION_Y    000001ed            
/dev/input/event0: EV_SYN       SYN_REPORT           00000000            
/dev/input/event0: EV_ABS       ABS_MT_POSITION_Y    000001ee            
/dev/input/event0: EV_SYN       SYN_REPORT           00000000            
/dev/input/event0: EV_ABS       ABS_MT_POSITION_X    000000f1            
/dev/input/event0: EV_SYN       SYN_REPORT           00000000            
/dev/input/event0: EV_ABS       ABS_MT_POSITION_Y    000001ef

highlight position while stoping scroll:
/dev/input/event0: EV_ABS       ABS_MT_POSITION_X    000000e3            
/dev/input/event0: EV_SYN       SYN_REPORT           00000000            
/dev/input/event0: EV_ABS       ABS_MT_POSITION_Y    000000bc            
/dev/input/event0: EV_SYN       SYN_REPORT           00000000            
/dev/input/event0: EV_ABS       ABS_MT_POSITION_X    000000e2            
/dev/input/event0: EV_SYN       SYN_REPORT           00000000            
/dev/input/event0: EV_ABS       ABS_MT_POSITION_Y    000000bb
Flags: needinfo?(mlien)
I can reproduce in flame also and have some analysis about this issue.

once Communications receive the touch event, it will choose the position we click and find the target(element) to send the event. When issue happened, it seems like the target gecko choose can not always being same as the finger stays.

looks like driver report correct position in this case.
probably need to check nsPresShell.cpp and nsViewManager.cpp for more detail analysis
just figure out this issue can NOT reproduce if we disable APZC, but not sure who can help on this.

Hi Rachelle,
Maybe you can help on this? Thanks!
Flags: needinfo?(ryang)
Hi Keven, Could you please help find someone to look into the correlation of APZC and contact, and to see if we can make any improvement on this?
Thank you very much!
Flags: needinfo?(ryang) → needinfo?(kkuo)
[Blocking Requested - why for this release]: According to comment 8, this case can be duplicated on Flame.

Hi! Peter,

Is this an APZC related issue? Need your help to confirm. Thanks

--
Keven
blocking-b2g: --- → 2.0?
Flags: needinfo?(kkuo) → needinfo?(pchang)
:kats, would definitely know if this is a known issue or not and unless this is a new regression we should not block 2.0 on this. but NI :kats to get his thoughts.
Flags: needinfo?(bugmail.mozilla)
There was a very similar issue that was fixed recently in bug 1068802, and was uplifted to 2.0 yesterday. Can we re-check on a build that includes that fix to see if the problem still happens? If it does I can take a look, it does look like an APZ issue.
Flags: needinfo?(bugmail.mozilla)
Clear the needinfo since kats already gave the suggestion in comment 14.
Flags: needinfo?(pchang)
I can still reproduce it in flame with following info:
Gaia-Rev        95a51689acd0488b3ba79abe2423cdcc132fff4a
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c1d4b8a89b10
Build-ID        20140925160203
Version         32.0
Device-Name     flame

reproduce rate is around 1/20 or less

If you want to add contacts in your device, please try 
$ cd gaia
$ make reference-workload-medium

Hi Kats, any suggestion about this?
Flags: needinfo?(bugmail.mozilla)
Oh, sorry I got confused earlier and thought this was about 2.1 when it's actually about 2.0. Bug 1068802 which I referenced earlier in comment 14 was also uplifted to 2.1, not 2.0. It might however fix this issue. To verify that we should test 2.1 builds just before and just after that patch was uplifted, to see if it happens before but not after. If so then we can uplift that patch further to 2.0. Or, if you are willing to do a local build, you can just apply the patch from that bug to 2.0 (it should apply without too much rebasing work) and see if the problem still happens.

Sorry again for the confusion.
Flags: needinfo?(bugmail.mozilla)
I can still reproduce this bug in latest PVT build :(

Gaia-Rev        2834baf4c7e34fe6ef335f0469f6d0f593c5922b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/6a63bcb6e0d3
Build-ID        20140928160202
Version         35.0a1
I am able to reproduce this on master, although it's very flaky. It becomes much easier to reproduce if you set the apz.fling_repaint_interval pref to 1000. The reason it's happening on master is that when we do a touch-start, the APZ code flushes any pending repaint requests to the content. The touch-start therefore doesn't need to be untransformed because the APZ and gecko are in sync. (This is the change that was made to fix bug 973105).

However, the problem is that the repaint request doesn't actually arrive at the content process before the touch-start. Because the input event arrives on the root process main thread, the code at [1] causes the repaint request to get bumped to a later spin of the root process main thread, which means it arrives at the content process after the input event.

Changing the code at [1] to run DoRequestContentRepaint directly instead of redispatching to the UI loop solves this bug, but that code was that way for a reason (see bug 798194) so I'll have to think about this a bit more and figure out how to fix it properly.

This same problem is the likely cause of the bug in B2G versions older than 2.2, although the exact codepaths are going to be a little different because they don't have bug 973105. In particular the event will have an untransform but that may be computed using an incorrect last-dispatched paint metrics, for the same reason as above.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp?rev=3a582ed49923#89
Assignee: nobody → bugmail.mozilla
Status: UNCONFIRMED → NEW
Component: Gaia::Contacts → Panning and Zooming
Ever confirmed: true
Product: Firefox OS → Core
This issue likely affects 1.4 as well but I haven't confirmed that.
After thinking about this a bit more I think the right fix here is to ensure the APZC always dispatches the repaint request from the same thread. It seems kind of silly to allow dispatching it from either the compositor or the controller thread, and that behaviour probably results in other races too. If we make sure all the repaint requests are sent on the controller thread, then we can undo some of the code in bug 798194 while maintaining the repaint request order, and that should fix this bug.

It's probably worth making sure that all the other functions invoked on GeckoContentController are not split across threads as well.
Attached patch PatchSplinter Review
This fixes the problem, but it's really ugly and I would like to find a better solution. Any ideas? I could probably have done more rewriting of the RequestContentRepaint codepaths but at the same time I'd rather keep the patch as simple as possible in case we need to uplift it, and then do any additional cleanup in bug 1074401.

On the other hand, I'm not sure this patch is upliftable without bug 973105 anyway, so maybe it's not worth trying to uplift it at all?
Attachment #8497174 - Flags: feedback?(botond)
Triage:2.0+. Since this is strongly requested by partner. Thanks!
Hi Mike,

Can you please open a public clone of this bug? Since there is a device image attached to this bug so we cannot make this bug public. Once you do please nominate it for 2.0? and duplicate this one. Thank you very much!

Hi Kats,

Apologies for the trouble, but once Mike has that new bug can you shift your work to the new open bug in order to land that patch?
Flags: needinfo?(mlien)
Flags: needinfo?(bugmail.mozilla)
Blocks: 1074691
Duplicate of this bug: 1074691
(In reply to Wayne Chang [:wchang] from comment #24)
> Hi Mike,
> 
> Can you please open a public clone of this bug? Since there is a device
> image attached to this bug so we cannot make this bug public. Once you do
> please nominate it for 2.0? and duplicate this one. Thank you very much!
> 
> Hi Kats,
> 
> Apologies for the trouble, but once Mike has that new bug can you shift your
> work to the new open bug in order to land that patch?

please keep tracking with 1074691
Flags: needinfo?(mlien)
Mike closed the new tracking bug as a dupe of this one. I don't really have a problem using whichever bug you guys decide on.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27)
> Mike closed the new tracking bug as a dupe of this one. I don't really have
> a problem using whichever bug you guys decide on.

I made a big mistake, duplicate the wrong way
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1074691
I moved the blocking-b2g nomination for 2.0 to bug 1074691.
blocking-b2g: 2.0? → ---
Comment on attachment 8497174 [details] [diff] [review]
Patch

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

I haven't fully thought through the problem and this patch's approach for solving it, but while looking at it I noticed that the B2G implementation of GeckoContentController::PostDelayedTask() [1] posts the task to the _current_ thread's message loop, which means that if we call it from the compositor thread then it will be run on the compositor thread.

Therefore, insofar as DispatchRepaintRequest just posts a call to DispatchRepaintRequestOnControllerThread via PostDelayedTask, it doesn't actually dispatch the request on the controller thread if the DispatchRepaintRequest call itself is on the compositor thread (which can happen, e.g. via NotifyLayersUpdated -> RequestContentRepaint -> /* paint throttler doesn't have an outstanding request */ -> DispatchRepaintRequest).

(There's also a matter of GeckoCC::PostDelayedTask() being either specified incorrectly (if it's meant to only be called from the controller thread to begin with), or implemented incorrectly on B2G (if it's meant to be called from any thread). In practice this hasn't a problem because there aren't any existing calls to it from the compositor thread.)

Dropping the f? for now; I'll have another look at this once this issue is addressed.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp?rev=3a582ed49923#214
Attachment #8497174 - Flags: feedback?(botond)
Group: kddi-confidential
You need to log in before you can comment on or make changes to this bug.