On Firefox 61.0b8, macOS 10.13.4. - Open a MozReview page such as https://reviewboard.mozilla.org/r/246576/diff/1#index_header - While the review is loading, the list of files (just below "Diff Revision 1") has a little spinner to the left of the filename. Observe the spinner while scrolling up and down Expected: spinner stays next to the filename as you scroll Actual: spinner jumps all over the place I'm bisecting now, will post more details soon.

Here's a video demonstrating the problem (taken on the May 25 nightly with a clean profile). Note that I often had to reload the page a few times in order to catch a length of time with the spinners present. I didn't try making a reduced version of the page. I ran mozregression twice and both times I got the same regression range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b63e4bff951e97ce23e5673cb55a990a1bbb1a3c&tochange=eb34acceb9dcba6faff9eee5e8c9183755ddd37a Which doesn't really seem to make sense, but that's why I ran it twice.

Karl, any idea how your patches might have caused this?

So I'm clear, this happens with a default config? No WR enabled or anything?

Correct

For the reasons given in https://bugzilla.mozilla.org/show_bug.cgi?id=1458665#c2 I can't imagine how either of those changes would cause this. Regression ranges in bug 1458665 were not certain but they did come down to the same changesets (or nearby). I also get a similar regression range (on Linux) when I use mozregression, but mozregression never ran a build for b63e4bff951e. It seems that mozregression is doing something odd when switching from mozilla-central to autoland. I reproduce the symptoms of this bug with ~/.local/bin/mozregression --launch b63e4bff951e which uses https://queue.taskcluster.net/v1/task/JRnJuCnIRoenuepHf6qkFQ/runs/0/artifacts/public%2Fbuild%2Ftarget.tar.bz2 for b63e4bff951e https://reviewboard.mozilla.org/r/24415/diff/13/ reproduces often. ~/.local/bin/mozregression --launch dc845b3a8cbe does not reproduce. That leads to the much more likely looking regression range of https://hg.mozilla.org/integration/autoland/log?rev=ancestors(b63e4bff951e)-ancestors(dc845b3a8cbe)

Thank you, kats and karlt. So on non-WebRender, we need to apply unchanged animated values in any way? Anyway I will take this.

I was wondering why this problem doesn't happen on my local build. That's because I did set layout.scroll.root-frame-containers true there.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c79de91b7b9a02b9dcde897c2fd8a36a15c7c17c I have no idea how to write a test case for this. :/

It's possible to simulate APZ behaviour in mochitests by using setAsyncScrollOffset [1], if that helps you write a test for it. Maybe create a page with an animation, do a frame of APZ scroll, and then read back the shadow transform via DWU.getOMTAStyle? [1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/dom/interfaces/base/nsIDOMWindowUtils.idl#1405

Thanks kats for the info! getOMTAStyle() returns only animation style, so it won't be used as it is, but now I think I can write a test case with setAsyncScrollOffset and another function returning the transform value regardless of whether the value is composed by animation or not.

Finally I managed to write the test case. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae4a0130b37c7f07144294ad07157cb71dcc228b

The test failed on TV. :/ The reason is that the test sometimes proceeded before the transform value by APZC applied. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ee923cccef9926ff00f56b55620dc7984e34ee2 This try should be green. (I hope)

It turns out that we have to set the transform value in case of opacity animations too. I guess we ideally have separate variables for transform by animation and transform by APZC. But for now I am going to go with the current setup. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bb893c9cedec3d8f6b8edf5aa5f8ec42dc727a8

Comment on attachment 8982929 [details] Bug 1464568 - Set the shadow base transform for animation explicitly even if the transform value hasn't changed. https://reviewboard.mozilla.org/r/248758/#review254922 ::: gfx/layers/composite/AsyncCompositionManager.cpp:747 (Diff revision 1) > + // FIXME: Bug 1459775: It seems possible that we somehow try > + // to sample animations and skip it even if the previous value > + // has been discarded from the animation storage when we enable > + // layer tree cache. So for the safety, in the case where we > + // have no previous animation value, we set non-animating value > + // instead. I should note about bug 1459775. The tab layer cache has been disabled (bug 1465106), so the assertion (bug 1459775) would no longer happen, but still I don't still quite understand how the assertion happens (see bug 1459775 comment 11), so for the safety I did add a fail-safe instead of crashing.

Comment on attachment 8982926 [details] Bug 1464568 - Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not. https://reviewboard.mozilla.org/r/248752/#review254946 ::: gfx/layers/ipc/CompositorBridgeParent.h:118 (Diff revision 1) > + TransformsToSkip aSkip = > + TransformsToSkip::NoneOfThem) = 0; I would prefer to remove the default value here, since you're already explicitly specifying the arg at all the call sites of this function ::: gfx/layers/ipc/CompositorBridgeParent.h:246 (Diff revision 1) > + TransformsToSkip aSkip = > + TransformsToSkip::NoneOfThem) override; Ditto, drop default value ::: gfx/layers/ipc/CrossProcessCompositorBridgeParent.h:92 (Diff revision 1) > + TransformsToSkip aSkip = > + TransformsToSkip::NoneOfThem) override; Ditto, drop default value

Comment on attachment 8982927 [details] Bug 1464568 - Add an IPC call to get transform value for a given element on the compositor. https://reviewboard.mozilla.org/r/248754/#review254948 ::: gfx/layers/ipc/LayerTransactionParent.cpp:759 (Diff revision 1) > + Layer* layer = AsLayer(aLayerHandle); > + if (!layer) { > + return IPC_FAIL_NO_REASON(this); > + } > + > + mCompositorBridge->ApplyAsyncProperties(this); I guess you'll have to add the NoneOfThem arg here if you make the changes I requested in part 1 ::: gfx/layers/ipc/LayerTransactionParent.cpp:772 (Diff revision 1) > + 1.0f); > + } > + float scale = 1; > + Point3D scaledOrigin; > + Point3D transformOrigin; > + for (uint32_t i=0; i < layer->GetAnimations().Length(); i++) { nit: spaces around =

Comment on attachment 8982928 [details] Bug 1464568 - Add a new function to get transform value on the compositor. https://reviewboard.mozilla.org/r/248756/#review254952 ::: dom/base/nsDOMWindowUtils.cpp:3643 (Diff revision 1) > *aRetVal = aTarget-> > DispatchEvent(*aEvent, CallerType::System, IgnoreErrors()); > return NS_OK; > } > > -NS_IMETHODIMP > +static Result<nsIFrame*, nsresult> nice! First time I'm interacting with this C++ Result class, it's pretty handy here :)

Comment on attachment 8982929 [details] Bug 1464568 - Set the shadow base transform for animation explicitly even if the transform value hasn't changed. https://reviewboard.mozilla.org/r/248758/#review254954 Minusing for the add_task changes. Sorry to be picky about that but I spend a lot of time debugging APZ mochitest failures and really don't want to introduce more complexity in that regard. ::: commit-message-d597c:8 (Diff revision 1) > +That's because the shadow base transform value might have been changed by APZC. > + > +The test case in this patch fail without this fix on non-WebRender and the test > +case is skipped on WebRender since the bug should never happen on WebRender > +because WebRender manages animation transform value and APZ transform value > +serapately. typo: separately ::: gfx/layers/apz/test/mochitest/helper_bug1464568_transform.html:4 (Diff revision 1) > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Test that transform animation is correctly placed during asynchronos scrolling</title> typo: asynchronous ::: gfx/layers/apz/test/mochitest/helper_bug1464568_transform.html:34 (Diff revision 1) > +const SimpleTest = opener.SimpleTest; > +const ok = opener.ok.bind(opener); > +const is = opener.is.bind(opener); You shouldn't need to define `SimpleTest`, `ok`, or `is` here. They are already exported into the subtest window by the runSubtestsSeriallyInFreshWindows at https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/gfx/layers/apz/test/mochitest/apz_test_utils.js#242-244 Note in particularly that the `ok` and `is` functions will include the name of the subtest that is running which is a really nice feature, and defining these here yourself will clobber that. ::: gfx/layers/apz/test/mochitest/helper_bug1464568_transform.html:40 (Diff revision 1) > +const ok = opener.ok.bind(opener); > +const is = opener.is.bind(opener); > +const info = opener.info.bind(opener); // For AddTask.js > +const utils = SpecialPowers.getDOMWindowUtils(window); > + > +SimpleTest.finish = subtestDone; Because of this override of SimpleTest.finish I would prefer to avoid using the add_task machinery in APZ mochitests that use the runSubtestsSerially machinery. It doesn't seem like add_task provides a lot of value for cases like this where there's only one "task" and it introduces additional black-box complexity. Already we have enough trouble with APZ tests being pretty flaky and I don't want to have to go and debug subtle interactions between add_task machinery and runSubtestsSerially machinery. Can you instead structure the test like so: async function test_transform() { // your test code starting with utils.setDisplayPortForElement } if (utils.layerManagerType == 'WebRender') { ok(true, ...); subtestDone(); } else { waitUntilApzStable().then(test_transform).then(subtestDone); } You can then also drop the `info` definition and the AddTask.js import ::: gfx/layers/composite/AsyncCompositionManager.cpp:741 (Diff revision 1) > + if (layer->GetScrollMetadataCount() > 0) { > + // In the case of transform we have to set the unchanged > + // transform value again becasue APZC might have modified the > + // previous shadow base transform value. Do we need this check? APZ also modifies the shadow transform on scrollthumb layers, for which GetScrollMetadataCount() returns zero. I guess those layers would never have transform animations on them, but it still seems kind of brittle to have this check here.

Comment on attachment 8982930 [details] Bug 1464568 - Set the shadow base transform value for the case where opacity animations' calculation was skipped. https://reviewboard.mozilla.org/r/248760/#review254960 Same changes/questions here as in the previous patch: (1) please remove add_task stuff, (2) typo with asynchronous in the test title, (3) do we really need the GetScrollMetadataCount() check. Otherwise this looks fine.

Thanks for the review! (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24) > ::: gfx/layers/composite/AsyncCompositionManager.cpp:741 > (Diff revision 1) > > + if (layer->GetScrollMetadataCount() > 0) { > > + // In the case of transform we have to set the unchanged > > + // transform value again becasue APZC might have modified the > > + // previous shadow base transform value. > > Do we need this check? APZ also modifies the shadow transform on scrollthumb > layers, for which GetScrollMetadataCount() returns zero. I guess those > layers would never have transform animations on them, but it still seems > kind of brittle to have this check here. Phew, I didn't know that. Moving scrollthumbs by APZC makes quite sense (I don't quite understand why its scroll metadata count is zero though), and it theoretically can have transform animations I believe. That's fair. I will drop the GetScrollMetadataCount() check there, and revise test code. Thanks!

Comment on attachment 8982927 [details] Bug 1464568 - Add an IPC call to get transform value for a given element on the compositor. https://reviewboard.mozilla.org/r/248754/#review255034 r=me on the sync-messages.ini changes.

Comment on attachment 8982926 [details] Bug 1464568 - Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not. https://reviewboard.mozilla.org/r/248752/#review255050

Comment on attachment 8982929 [details] Bug 1464568 - Set the shadow base transform for animation explicitly even if the transform value hasn't changed. https://reviewboard.mozilla.org/r/248758/#review255052

Comment on attachment 8982930 [details] Bug 1464568 - Set the shadow base transform value for the case where opacity animations' calculation was skipped. https://reviewboard.mozilla.org/r/248760/#review255054 Thanks!

The test case failed on Android; https://treeherder.mozilla.org/#/jobs?repo=try&revision=151df58aa44347cc8d80f66470f217d5e904e254&selectedJob=181823866 It seem to me that setAsyncScrollOffset doesn't work on Android?

I am going to skip the test on Android for now.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/88bbaa441a37 Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not. r=kats https://hg.mozilla.org/integration/autoland/rev/cf6231e7ef0c Add an IPC call to get transform value for a given element on the compositor. r=froydnj,kats https://hg.mozilla.org/integration/autoland/rev/a5e8c4b793e6 Add a new function to get transform value on the compositor. r=kats https://hg.mozilla.org/integration/autoland/rev/b8c1542514b5 Set the shadow base transform for animation explicitly even if the transform value hasn't changed. r=kats https://hg.mozilla.org/integration/autoland/rev/9ea461e4570b Set the shadow base transform value for the case where opacity animations' calculation was skipped. r=kats

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36) > It seem to me that setAsyncScrollOffset doesn't work on Android? That's odd. I thought it should work everywhere. If you file a follow-up bug for that I can look into it.

https://hg.mozilla.org/mozilla-central/rev/88bbaa441a37 https://hg.mozilla.org/mozilla-central/rev/cf6231e7ef0c https://hg.mozilla.org/mozilla-central/rev/a5e8c4b793e6 https://hg.mozilla.org/mozilla-central/rev/b8c1542514b5 https://hg.mozilla.org/mozilla-central/rev/9ea461e4570b

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #36) > > It seem to me that setAsyncScrollOffset doesn't work on Android? > > That's odd. I thought it should work everywhere. If you file a follow-up bug > for that I can look into it. Filed bug 1466950 for that.

Comment on attachment 8982926 [details] Bug 1464568 - Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not. Approval Request Comment [Feature/Bug causing the regression]: bug 1454324 [User impact if declined]: Opacity and transform animations with step functions are misplaced during scrolling [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: Yes, though there are two automation test cases, but they are simplified test cases, so it would be nice that someone checks the original case. To reproduce the problem, open the reviewboard link in comment 0 and scrolling up and down during small spinners are spinning. (On my network environment, the spinners finish soon, so I can't see the original issue) [List of other uplifts needed for the feature/fix]: All patches in this bug [Is the change risky?]: I don't think so [Why is the change risky/not risky?]: Though there are five patches here, but most of them are for the automation tests, so the fix itself is pretty simple, also I added a fail-safe in the fix, see comment 20 [String changes made/needed]: None

Comment on attachment 8982926 [details] Bug 1464568 - Add an argument to ApplyAsyncProperties() to apply the transform by APZC or not. This one makes me a bit nervous given the size of the patches, but given that we've gotten multiple reports about this issue, it's a new regression in 61, and it includes automated test coverage, I'll approve for 61.0b12. Let's be vigilant for any new regression reports, though.

Part 4 needs a rebased patch due to conflicts with bug 1458457.

https://hg.mozilla.org/releases/mozilla-beta/rev/503f6513c774 https://hg.mozilla.org/releases/mozilla-beta/rev/e13452da31b5 https://hg.mozilla.org/releases/mozilla-beta/rev/8d255e70fccd https://hg.mozilla.org/releases/mozilla-beta/rev/6c95fd0a106c https://hg.mozilla.org/releases/mozilla-beta/rev/622e8a464432

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0 Build ID: 20180608100105 User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0 Build ID: 20180607135512 Verified as fixed on Nightly build 62.0a1 (2018-06-08) and Beta 61.0b12 on MacOS 10.13.5.

Thank you Cristian!