980679 - [B2G][Youtube] The screen will jump up and down when scrolling through the Youtube app

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: youtubelog.txt

Description:
If the user scrolls the Youtube app as soon as they open it, the screen will jump up and down.

Repro Steps:
1)  Updated Buri to Build ID: 20140306040204
2)  Install the Youtube app from the Marketplace.
3)  Open the Youtube app.
4)  As soon as the app loads, try to scroll through the videos.

Actual:
The screen will appear to be jumping up and down as the user scrolls down through the Youtube app.

Expected:
The screen scrolls smoothly without issue. 

Environmental Variables
Device: Buri v 1.4.0 Mozilla RIL
Build ID: 20140306040204
Gecko: https://hg.mozilla.org/mozilla-central/rev/8122ffa9e1aa
Gaia: 9cb35e701df44766d9b3560b0defe0a401a0ecdd
Platform Version: 30.0a1
Firmware Version: v1.2-device.cfg

Notes:
Repro frequency: 100%
See attached: videoclip, logcat

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue also occurs on the Buri v 1.3.0 Mozilla RIL

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140306004002
Gaia: 8aed4fafbaeb86d6884d31ce7d3cbeb87bcbf837
Gecko: 3d2d84d52141
Version: 28.0
Firmware Version: v1.2-device.cfg

The screen jumps up and down when scrolling the Youtube app.

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: Youtubevideo.mp4

Comment 3

[Jason Smith [:jsmith]]

Does this happen on 1.1?

Comment 4

[Roland Kunkel [:RolandK]]

(In reply to Jason Smith [:jsmith] from comment #3)
> Does this happen on 1.1?

Following the STR in comment #0, the page will flash white then will jump to where the user would have scrolled normally while the page is loading. After the page fully loads the user can scroll smoothly.

Environmental Variables:
Device: Buri 1.1 MOZ
BuildID: 20140306041201
Gaia: 44a2ddf63373f8e95c784faf4ed4d60081699c61
Gecko: 1421a6b7fc51
Version: 18.0
Firmware Version: V1.2-device.cfg

Comment 5

[Preeti Raghunath(:Preeti)]

1.3+ for youtube

Comment 6

[Kartikaya Gupta (email:[e-mail])]

From my initial investigation on this bug it seems like what happens is that the content does a scrollTo(0,1). Layout sends a layers update to the APZC with mUpdateScrollOffset=true, and continues to do so until the scroll offset update is acknowledged. The APZ code dispatches such an update, but it takes a while to actually get processed because it has to do an IPC jump and also Gecko is busy with doing other things. Until the scroll offset update is acknowledged, every layers update that is received by the APZ (and there's a bunch of them, because the page is loading) has the mUpdateScrollOffset flag set to true, and so all of these layer updates keep clobbering the async scroll offset. Therefore, as the user scrolls, the scroll position keeps getting reset back to (0,1) causing the visual jumps.

Not sure what the right fix here is. There might be something else at play here too because it feels like the latency for processing the acknowledgement is much larger than I would expect.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> There might be something else at play
> here too because it feels like the latency for processing the
> acknowledgement is much larger than I would expect.

Nothing else at play, it seems. Just a really big latency with parent -> child messaging.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Also for the record in the test I just ran the message is sent from the APZC on the parent side at 16:02:07.349 and isn't received in TabChild until 16:02:09.109, which is 1.760 seconds (according to logcat timestamps). During this time APZC receives another 19 NotifyLayersUpdated calls with mUpdateScrollOffset=true; it receives an additional 5 for inflight calls dispatched before the acknowledgement is processed.

Comment 9

[Brogan Zumwalt [Inactive]]

Last working Build:
Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140110004009
Gaia: c606b129a2d1647c0fc7bfb197555026e9b27f9e
Gecko: c5109884ae3a
Version: 28.0a2
Firmware Version: v1.2-device.cfg


First Broken Build:
Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140111004005
Gaia: 1f029aa4c924d6c59fd7e8c6d9786a7370755d49
Gecko: 89c68fcb1448
Version: 28.0a2
Firmware Version: v1.2-device.cfg


Last Working Gaia/First Broken Gecko: Issue DOES Reproduce
Gaia: c606b129a2d1647c0fc7bfb197555026e9b27f9e
Gecko: 89c68fcb1448

First Broken Gaia/Last Working Gecko: Issue Does NOT Reproduce
Gaia: 1f029aa4c924d6c59fd7e8c6d9786a7370755d49
Gecko: c5109884ae3a

http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=c5109884ae3a&tochange=89c68fcb1448

Appears to be Gecko issue. Unable to narrow regression window further due to lack of Tinderbox and Inbound windows for this date range.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Bug 949132 is in that range, which fits based on what I described in comments 6-8.

Comment 11

[Timothy Nikkel (:tnikkel)]

Maybe if we did the scroll offset update acknowledgement from the refresh driver it would get processed sooner on the content side?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Can you elaborate on that? I'm not sure when the refresh driver would do the acknowledgement.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

One possible solution I think is to use a tri-state flag. State 1 would be when some non-apz code has set the scroll offset (this corresponds to originOfLastScroll != nullptr && originOfLastScroll != nsGkAtoms::apz). State 2 would be after RecordFrameMetrics has been called while in State 1 (that is, the mUpdateScrollOffset flag has been sent to the APZ side). During State 2 the code in APZCCallbackHelper would still not set scroll offset updates fromthe APZ. We would move into State 3 once the acknowledgement arrives, and originOfLastScroll is reset back to null.

This would prevent RecordFrameMetrics from sending multiple mUpdateScrollOffset=true metrics to the APZ side, which would fix this problem. However it should not regress any existing behaviour because the APZ code still can't set a scroll offset until the acknowledgement is processed.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Actually putting the tri-state flag in the RecordFrameMetrics code doesn't seem to work, because each call to RecordFrameMetrics can still be associated with multiple layer transactions (not sure why exactly). Even with tiling disabled I see multiple NotifyLayersUpdated with mUpdateScrollOffset=true even though there was only one RecordFrameMetrics that set it.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Filter duplicate scroll update messages in the APZ

Since we can get multiple NotifyLayersUpdated calls where aLayerMetrics.mUpdateScrollOffset is true (but they all have the same scroll generation) we can deduplicate the scroll update using the generation counter. This prevents us from constantly clobbering the APZ's frame metrics (we will only do it once per scroll generation) and also reduces the number of acknowledgement messages we send back to the child process (one per generation rather than one per NotifyLayersUpdated).

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> Can you elaborate on that? I'm not sure when the refresh driver would do the
> acknowledgement.

I guess we need to back up a little. How does the scroll update acknowledgement get from the APZC thread to layout?

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #16)
> I guess we need to back up a little. How does the scroll update
> acknowledgement get from the APZC thread to layout?

The acknowledgement originates at [1] which is on the compositor thread in the parent process. That call jumps over to the UI thread of the parent process at [2], and is sent to the child process at [3]. The gap between this send call and the corresponding receive call at [4] is where the latency is. I don't think we can get it to the child process any faster if the child process is busy. The only thing we could is make it a higher priority message so that it somehow jumps over whatever else is waiting in the child process' event queue and gets processed first, but that's probably a bad idea.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp?rev=fb97d6af06af#1780
[2] http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp?rev=aae54a60278d#530
[3] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?rev=aae54a60278d#515
[4] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=aae54a60278d#1500

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8389965 [details] [diff] [review]
Filter duplicate scroll update messages in the APZ

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

LGTM.

I also took this opportunity to have a look at the patch that introduced scroll generations to begin with, which I hadn't reviewed but have been meaning to. Very nice overall!

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1551,5 @@
>    , mResizerBox(nullptr)
>    , mOuter(aOuter)
>    , mAsyncScroll(nullptr)
>    , mOriginOfLastScroll(nsGkAtoms::other)
> +  , mScrollGeneration(1)

Please add a comment explaining why this starts at 1 instead of 0.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #18)
> Please add a comment explaining why this starts at 1 instead of 0.

Done and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8764e9f061d0

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8764e9f061d0

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)

Ok, that's not where I thought the latency was.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Since kats is on PTO, can you please request approval for b2g28 when this is ready? Thanks :)

Comment 23

[Timothy Nikkel (:tnikkel)]

Botond reviewed this, so I'll bump the needinfo over to him.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8389965 [details] [diff] [review]
Filter duplicate scroll update messages in the APZ

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 963278
User impact if declined: Jumps in scrolling while a page is loading
Testing completed: I just tested it locally. Presumably Kats tested it as well.
Risk to taking this patch (and alternatives if risky): It's no more risky than the bug 963278 patch whose regression it fixes, which was approved for uplift in bug 980041. The alternative would be backing out the patches uplifted in bug 980041, which would regress the bugs that those patches fixed.
String or UUID changes made by this patch: None

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply cleanly to b2g28.

Comment 26

[Botond Ballo [:botond]]

Attachment: Patch for uplifting to 1.3

Rebased the patch to apply to 1.3.

Comment 27

[Jason Smith [:jsmith]]

Comment on attachment 8389965 [details] [diff] [review]
Filter duplicate scroll update messages in the APZ

Moving a- - this caused a smoketest regression.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out of m-c in http://hg.mozilla.org/mozilla-central/rev/e84a391b604b
And from Aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/9b482d6994fd

Comment 29

[Kartikaya Gupta (email:[e-mail])]

The regression in bug 985529 happened because there is a case that falls through when this patch is applied.

Say a scroll frame is created, and so sends a scroll update to the APZC instance with scroll generation = 1. This is acknowledged by the APZC. Then, the scroll frame is destroyed and re-created, but is mapped to the same APZC (this happens on occasion). The new scroll frame will again send a scroll update with scroll generation = 1, but this time it will be ignored by the APZC because it already acknowledged an update with that generation. So things go out of sync and stay that way.

The best way I can think of to fix this is to start the ScrollFrameHelper::mScrollGeneration at an always-increasing initial value (using a static variable in nsGfxScrollFrame.cpp). This way when the scroll frame is re-created it will have a higher scroll generation than the old one so the APZC will accept the update.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #29)
> The best way I can think of to fix this is to start the
> ScrollFrameHelper::mScrollGeneration at an always-increasing initial value
> (using a static variable in nsGfxScrollFrame.cpp). This way when the scroll
> frame is re-created it will have a higher scroll generation than the old one
> so the APZC will accept the update.

Actually even this doesn't work in all cases, because the scroll frame might be scrolled once before being destroyed and re-created in which case it will end up with the same scroll generation with and without the destruction.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Attachment: Filter duplicate scroll update messages in the APZ (v2)

Updated version, needs more testing. The only difference is the deletion of the RemoteId in the ScrollFrameHelper destructor - this ensures we use a new APZC if the ScrollFrameHelper is destroyed and re-created, and so avoids the problem where we can't tell if the ScrollFrameHelper is destroyed and re-created.

Comment 32

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> Created attachment 8394409 [details] [diff] [review]
> Filter duplicate scroll update messages in the APZ (v2)
> 
> Updated version, needs more testing. The only difference is the deletion of
> the RemoteId in the ScrollFrameHelper destructor - this ensures we use a new
> APZC if the ScrollFrameHelper is destroyed and re-created, and so avoids the
> problem where we can't tell if the ScrollFrameHelper is destroyed and
> re-created.

I wonder if this risks regressing bug 934420, where we took care to preserve an APZC for a given ScrollableLayerGuid even if its layer was re-created. Do we know whether, in that case, it was just the layer being re-created, and not the scroll frame?

Comment 33

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #32)
> I wonder if this risks regressing bug 934420, where we took care to preserve
> an APZC for a given ScrollableLayerGuid even if its layer was re-created. Do
> we know whether, in that case, it was just the layer being re-created, and
> not the scroll frame?

That's a good point. I tested the test case attached to that bug on the B2G browser and it still behaves fine with this patch, so that's a good sign. But yeah we'll need to make sure this doesn't regress behaviour before we can uplift it anywere. I might even consider it too risky for uplift to 1.3.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8394409 [details] [diff] [review]
Filter duplicate scroll update messages in the APZ (v2)

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

tn, any objections to the line of code I added in the ScrollFrameHelper destructor?

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Attachment: Filter duplicate scroll update messages in the APZ (v3)

Botond suggested using a global counter for the scroll generation which also works and is less risky. It solves the problem outlined in comment 29 without incurring the problem in comment 30.

Comment 36

[Timothy Nikkel (:tnikkel)]

Another option would be to store the scroll generation on the content node, not sure if that is practically any better though.

Comment 37

[Kartikaya Gupta (email:[e-mail])]

I did consider that briefly but figured it would be more expensive to keeping updating a property every time it's incremented.

Comment 38

[Kartikaya Gupta (email:[e-mail])]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f36dfbb383

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e5f36dfbb383

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Ready to go with uplift requests? :)

Comment 41

[Kartikaya Gupta (email:[e-mail])]

Yup, thanks for reminding me! :)

Comment 42

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8394883 [details] [diff] [review]
Filter duplicate scroll update messages in the APZ (v3)

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 949132
User impact if declined: If the user scrolls the Youtube app as soon as they open it, the screen will jump up and down.
Testing completed (on m-c, etc.): on m-c for a week, no reports of regressions so far
Risk to taking this patch (and alternatives if risky): medium risk since this it's hard to ensure all cases are covered. this code is also not covered by tests so it's easy to regress. Affects B2G and metro.
String or IDL/UUID changes made by this patch: none

Comment 43

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8394883 [details] [diff] [review]
Filter duplicate scroll update messages in the APZ (v3)

accepting for aurora so it's in b2g 1.4 - not concerned about risk to metro, as we are not shipping that.

Comment 44

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c54582ca278

Comment 45

[bhavana bajaj [:bajaj]]

Flagging this for QA to verify before uplifting it to 1.3 and also some exploratory tests given we have no tests. :kats any other testcases you can through in here other than youtube for QA to do some exploratory testing here.

Comment 46

[Kartikaya Gupta (email:[e-mail])]

It would be good to test the behaviour while on a page that is driving scrolling via script. So create a test page or app that is doing scrollTo calls every half a second or so and scroll around and lock/unlock the device. Make sure the behavious is as expected.

Comment 47

[bhavana bajaj [:bajaj]]

:njpark is helping verify this so passing this onto him.

Comment 48

[No-Jun Park [:njpark]]

created a test javascript autoscroll page based on this script: http://www.brownielocks.com/autoscroll1.html
The page is: http://pageeasy.com/Autoscrolltest/.

The lock/unlock of device while scrolling does not cause any unexpected behavior.  It picks up where it left off when the device was locked, and continues scrolling.  When tries to manually scroll faster or to opposite direction, it stutters a bit, but that is expected according to kats.  (It shows similiar behavior on desktop)  

Tested on this version of 1.4
 Gaia      8dff633372022723e2ebad17fe3c826436b3b258                         │
│ Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/bc14179fc49c  │
│ BuildID   20140414000201                                                   │
│ Version   30.0a2 

And this version of Master:
│ Gaia      f3abbd2d0a60f1a1618db93f8b1957cae6de379c                         │
│ Gecko     https://hg.mozilla.org/mozilla-central/rev/215080b813a7          │
│ BuildID   20140414040203                                                   │
│ Version   31.0a1

Comment 49

[bhavana bajaj [:bajaj]]

Comment on attachment 8394883 [details] [diff] [review]
Filter duplicate scroll update messages in the APZ (v3)

Thanks for the help with verification :njpark, looks good to land.

Comment 50

[Kartikaya Gupta (email:[e-mail])]

Rebased and landed on b2g28:

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ab227cdd984c

Note that as part of rebasing this I basically had to pull in the patch at [1]. I didn't explicitly request b2g28 approval on that because the other patch on that bug was already approved for uplift into 1.3 and the second patch was fairly trivial (we had no reason to uplift it before but probably should have in hindsight).

[1] https://bug967671.bugzilla.mozilla.org/attachment.cgi?id=8375575

Comment 51

[pete siphantong [:Petes]]

Verified on the latest Tarako build

1.3T Environmental Variables:
Device: Tarako 1.3T
BuildID: 20140530014002
Gaia: e68858693b71d917c9c5ee7e215f7ceea04635f7
Gecko: 1945abae19ff
Version: 28.1
Firmware Version: SP6821a-Gonk-4.0-4-29

1.3 tarako: The screen scrolls smoothly without issue on the YouTube app

Comment 52

[Sxean Lee-David [:sxean]]

Verified fixed on following build: 
Environmental Variables:
Device: Flame 2.5
BuildID: 20150924030228
Gaia: 4bb17b24620818cbda0ba0c0d69e0ce3f914e1b7
Gecko: bf2bc1aa78c0b72d9b6b13f7a8c6ae61c60a51dc
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 44.0a1 (2.5) 
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

RESULT: 
the menu list of youtube videos within the app scrolls smoothly as expected.