Closed Bug 1255068 Opened 8 years ago Closed 8 years ago

Header image on twitter APZ scrolls at twice the regular speed or jitters

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- verified
firefox48 --- verified

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

STR:
 1. Go to https://twitter.com/jlongster and wait for the page to load.
 2. Scroll down slowly.

Observe how the header image runs away.

I suspect that this is a regression from bug 1238564 and that we're improperly nesting frame metrics.
Nothing to do with FrameMetrics nesting. It's an image layer with a scale, and this scale seems to throw off the APZ adjustment calculation.
It's not that either. It seems to have to do with how we treat empty transactions that only update layer transforms.
Blocks: 1192910
No longer blocks: 1238564
Attached file testcase (obsolete) —
I said I'd look into this, assigning so I don't forget.
Assignee: mstange → bugmail.mozilla
The problem here is that when APZ receives a layer transaction, it assumes that the scroll offset found on the layer's metrics, and the layer's content-side transform, are in sync. That is, when content scrolls a layer by modifying its content-side transform, APZ assumes the amount scrolled is reflected in the content-side scroll offset. It then takes any remaining difference between its async scroll offset and the content-side scroll offset, and sets a corresponding shadow transform on the layer.

The problem with empty transactions is that they update the content-side transform (that happens here [1], which then takes effect here [2]), but do not make a corresponding update to the content-side scroll offset as stored by the FrameMetrics on the layer (that's computed in ContainerState::SetupScrollingMetadata(), which happens during frame-layer-building, which doesn't happen for empty transactions). As a result, APZ thinks content hasn't updated its scroll offset yet (even though it has internally, in the copy stored on the nsIScrollableFrame, and reflected it in the content-side transform), and sets a large shadow transform, which is added to the content-side transform, resulting in scrolling twice as fast. Doing a full paint restores the scroll offset to its correct position.

I guess to fix this we need to update the scroll offset in the FrameMetrics in the empty transaction case too. It's not immediately clear where or how to do this. (The code that sets the content-side transform happens during restyle processing, and is not specific to scroll frames.)

[1] https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/layout/generic/nsFrame.cpp#5316
[2] https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/gfx/layers/client/ClientLayerManager.cpp#279
Thanks for investigating! It's not obvious to me how to fix this problem either, will give it some more thought. We should fix it before attempting to uplift bug 1192910, probably.
I think there's the easy way and the hard way of fixing it.

The easy way is: In nsIFrame::TryUpdateTransformOnly, check whether the layer has any frame metrics. For each frame metrics, look up the scroll frame, get a new frame metrics from it, compare the two frame metrics, and if they changed, return false and have the caller schedule a regular paint.

The hard way is: Do the same thing as the easy way but don't bail out if the frame metrics changed. Instead, set the new frame metrics on the layer. Then in APZ land, do everything you can to support different "main thread frame metrics" for the same scroll frame on different layers. For each layer, the async adjustment would adjust by the difference of the current APZ scroll position and the main thread scroll position *for that layer*.
However, letting different layers' frame metrics for the same scroll frame drift away from each other probably only works until there is a scroll position update forced by the main thread, which needs to clobber pending scroll position changes from APZ. In that case we would probably still have to force a full paint.
Oh, no, this won't work for transforms inside scrolled opacities. Layout sets the frame metrics on the opacity ContainerLayer, but for main thread scrolling, it only changes the transform of the transform ContainerLayer. Main thread scrolling does not move the opacity ContainerLayer.

So the "easy way" needs to also walk the ancestor layers of the transform ContainerLayer and check their FrameMetrics as well, and bail out if anything changed.

And the "hard way" gets much more complicated. We probably need to make APZ scrolling and main thread scrolling touch the same layers' transforms. Still thinking about how to do that.
Taking back. I'll implement the "easy way" approach.
Assignee: bugmail.mozilla → mstange
Attachment #8728526 - Attachment is obsolete: true
With this patch we do a full paint for transform changes after "paint skipped scrolls", and we still do empty transactions if the scroll position hasn't changed.
Comment on attachment 8729686 [details]
MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow

https://reviewboard.mozilla.org/r/39551/#review36227

This looks ok to me based on the previous analysis of the problem and your proposed solution. However I'm totally unfamiliar with empty transactions and this part of the code so it might be worth getting somebody else to look over it as well.

::: layout/generic/nsFrame.cpp:5282
(Diff revision 1)
> +    if (!scrollableFrame) {
> +      continue;

Can you think of a scenario in which this might happen? I'm wondering if it's safer to return false in this case.
Attachment #8729686 - Flags: review?(bugmail.mozilla) → review+
https://reviewboard.mozilla.org/r/39551/#review36227

I'll ask Matt for another review.

> Can you think of a scenario in which this might happen? I'm wondering if it's safer to return false in this case.

I can't, and you're right, false will be safer. I'll return false.
Comment on attachment 8729686 [details]
MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39551/diff/1-2/
Attachment #8729686 - Attachment description: MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats → MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow
Attachment #8729686 - Flags: review?(matt.woodrow)
Comment on attachment 8729686 [details]
MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow

https://reviewboard.mozilla.org/r/39551/#review36303

It seems like actually hitting this optimization is going to be super rare once APZ is enabled, since most layers will have the root scrollframes FM in their ancestor tree.

On top of that, I really hate the empty transaction transform code, so maybe we should consider just killing it entirely?

It was added for the original b2g homescreen (where scrolling was done via transform changes, to compensate for the lack of scroll snapping support in the compositor), which clearly isn't relevant any more.
Attachment #8729686 - Flags: review?(matt.woodrow) → review+
With this patch we're still allowing empty transaction for transform changes on layers with frame metrics if the scroll position in the frame metrics is up to date.

That said, I'm not too attached to this code either.
https://hg.mozilla.org/mozilla-central/rev/6ad26654ae6a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Although this was exposed by bug 1192910, it is a pre-existing issue, so it affects 46 and 47 as well (any codeline with any form of paint skipping). Wontfix'ing for 46 because APZ will only be enabled there for a couple of weeks and it doesn't seem worth it. However we should uplift to 47.
Hi Markus, please nominate bug for uplift to Aurora47. Thanks!
Flags: needinfo?(mstange)
Comment on attachment 8729686 [details]
MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow

Approval Request Comment
[Feature/regressing bug #]: bug 1192910 (APZ paint skipping)
[User impact if declined]: parts of the page moving unexpectedly during scrolling
[Describe test coverage new/current, TreeHerder]: there is no test for this specific issue, but in general the test coverage in this area is okay. I should write a test for this bug.
[Risks and why]: low - this patch causes us to skip an optimization in a case where it doesn't work and falls back to the proven working behavior
[String/UUID change made/needed]: none
Flags: needinfo?(mstange)
Attachment #8729686 - Flags: approval-mozilla-aurora?
Comment on attachment 8729686 [details]
MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow

Kats would like to see this uplifted, Aurora47+
Attachment #8729686 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
backed out for causing android crashes/assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2213611&repo=mozilla-aurora
Flags: needinfo?(mstange)
Flags: needinfo?(mstange)
Attachment #8734508 - Flags: review?(bugmail.mozilla)
Attachment #8734508 - Flags: review?(bugmail.mozilla) → review+
Depends on: 1261266
Flags: qe-verify+
Reproduced with 2016-03-09 Nightly.
Verified as fixed using Firefox 47 beta 6 and latest Aurora 48.0a2 2016-05-19 under Win 10 64-bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.