User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

I'm using this javascript to provide smooth scrolling over a certain distance.

var var scrstp_dist = 0, scrollstep_iteration = 0;
function scrollstep(dist) {
if (Math.abs(scrstp_dist) < Math.abs(dist)) {
scrollstep_iteration++;
var n = Math.floor(dist / 20);
scrstp_dist+= n;
w.scrollBy(0, n);
console.log("Iteration: " + scrollstep_iteration + " ScrollBy value: " + n + " Amount scrolled in theory: " + scrstp_dist + " Actual position: " + window.pageYOffset);
scrstp_timeout = setTimeout("scrollstep(" + dist + ")", 20);
}
}

It can be seen here https://www.sorat-hotels.com/de/hotel/ambassador-berlin.html when you click on the green select on the right side below the image slider.

Actual results:

The console logging shows that in the some iterations, the scrollBy(0, 65) resulted in a change in scrolling position by 130 Pixel, which is weird:

Iteration: 1 ScrollBy value: 65 Amount scrolled in theory: 65 Actual position: 398
Iteration: 2 ScrollBy value: 65 Amount scrolled in theory: 130 Actual position: 463
Iteration: 3 ScrollBy value: 65 Amount scrolled in theory: 195 Actual position: 593
Iteration: 4 ScrollBy value: 65 Amount scrolled in theory: 260 Actual position: 658

Expected results:

Every scroll iteration should scroll by the specified value.

Reproducible.
Also, I was getting some error in the console

[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIContentSniffer.getMIMETypeFromContent]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: resource:///modules/FaviconLoader.jsm :: onStopRequest :: line 217" data: no]

Build ID 20190225143501
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Placing this under Core: Selection
Please feel free to change the component to the right place.

Any chance you could attach here a standalone HTML test-case that reproduces the issue? That way investigating this would be much easier.

Also, do you know if this happens to be a regression?

Thanks! Here is the reduced test case.

We sometimes call CompleteAsyncScroll twice for a single scrollBy call.

Here is a backtrace where it comes from;

#0 0x00007feaa8d1cc40 in mozilla::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, nsAtom*) (this=0x7fea93a82230, aPt=..., aRange=..., aOrigin=<optimized out>) at /home/hiro/central/layout/generic/nsGfxScrollFrame.cpp:2739
#1 0x00007feaa8d1d3f0 in mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, nsAtom*) (this=0x7fea93a82230, aRange=..., aOrigin=0x0)
at /home/hiro/central/layout/generic/nsGfxScrollFrame.cpp:2141
#2 0x00007feaa8d1dbbf in mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, nsIScrollableFrame::ScrollMode, nsAtom*, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) (this=0x7fea93a82230, aScrollPosition=..., aMode=nsIScrollableFrame::INSTANT, aOrigin=0x7feaa3288370 <mozilla::detail::gGkAtoms+82100>, aRange=0x7ffe7dcc10f8, aSnap=<optimized out>) at /home/hiro/central/layout/generic/nsGfxScrollFrame.cpp:2262
#3 0x00007feaa8d1e130 in mozilla::ScrollFrameHelper::ScrollToCSSPixelsApproximate(mozilla::gfx::PointTyped<mozilla::CSSPixel, float> const&, nsAtom*) (this=0x20, aScrollPosition=..., aOrigin=0x0) at /home/hiro/central/layout/generic/nsGfxScrollFrame.cpp:2215
#4 0x00007feaa6d87191 in mozilla::layers::ScrollFrameTo(nsIScrollableFrame*, mozilla::layers::RepaintRequest const&, bool&) (aFrame=<optimized out>, aRequest=..., aSuccessOut=<optimized out>) at /home/hiro/central/gfx/layers/apz/util/APZCCallbackHelper.cpp:148
#5 0x00007feaa6d87191 in mozilla::layers::ScrollFrame(nsIContent*, mozilla::layers::RepaintRequest const&) (aContent=<optimized out>, aRequest=...) at /home/hiro/central/gfx/layers/apz/util/APZCCallbackHelper.cpp:188
#6 0x00007feaa6d86cdc in mozilla::layers::APZCCallbackHelper::UpdateRootFrame(mozilla::layers::RepaintRequest const&) (aRequest=...)
at /home/hiro/central/gfx/layers/apz/util/APZCCallbackHelper.cpp:342
#7 0x00007feaa6d5d843 in mozilla::layers::AsyncPanZoomController::RequestContentRepaint(mozilla::layers::FrameMetrics const&, mozilla::gfx::PointTyped<mozilla::ParentLayerPixel, float> const&, mozilla::layers::RepaintRequest::ScrollOffsetUpdateType) (this=0x7fea93afb000, aFrameMetrics=..., aVelocity=..., aUpdateType=<optimized out>) at /home/hiro/central/gfx/layers/apz/src/AsyncPanZoomController.cpp:3808
#8 0x00007feaa6d5a337 in mozilla::layers::AsyncPanZoomController::RequestContentRepaint(mozilla::layers::RepaintRequest::ScrollOffsetUpdateType) (this=0x7fea93afb000, aUpdateType=<optimized out>) at /home/hiro/central/gfx/layers/apz/src/AsyncPanZoomController.cpp:3753
#9 0x00007feaa6d7efd9 in mozilla::detail::RunnableMethodArguments<mozilla::layers::RepaintRequest::ScrollOffsetUpdateType>::applyImpl<mozilla::layers::AsyncPanZoomController, void (mozilla::layers::AsyncPanZoomController::)(mozilla::layers::RepaintRequest::ScrollOffsetUpdateType), StoreCopyPassByConstLRef<mozilla::layers::RepaintRequest::ScrollOffsetUpdateType>, 0ul>(mozilla::layers::AsyncPanZoomController, void (mozilla::layers::AsyncPanZoomController::)(mozilla::layers::RepaintRequest::ScrollOffsetUpdateType), mozilla::Tuple<StoreCopyPassByConstLRef<mozilla::layers::RepaintRequest::ScrollOffsetUpdateType> >&, std::integer_sequence<unsigned long, 0ul>) (o=<optimized out>, m=<optimized out>, args=...)
at /home/hiro/central/obj-firefox/dist/include/nsThreadUtils.h:1122
#10 0x00007feaa6d7efd9 in ZN7mozilla6detail23RunnableMethodArgumentsIJNS_6layers14RepaintRequest22ScrollOffsetUpdateTypeEEE5applyINS2_22AsyncPanZoomControllerEMS7_FvS4_EEEDTcl9applyImplfp_fp0_dtdefpT10mArgumentstlSt16integer_sequenceImJLm0EEEEEEPT_T0 (this=0xffffd610, o=<optimized out>, m=<optimized out>) at /home/hiro/central/obj-firefox/dist/include/nsThreadUtils.h:1128
#11 0x00007feaa6d7efd9 in mozilla::detail::RunnableMethodImpl<mozilla::layers::AsyncPanZoomController, void (mozilla::layers::AsyncPanZoomController::)(mozilla::layers::RepaintRequest::ScrollOffsetUpdateType), true, (mozilla::RunnableKind)0, mozilla::layers::RepaintRequest::ScrollOffsetUpdateType>::Run() (this=0xffffd5d0) at /home/hiro/central/obj-firefox/dist/include/nsThreadUtils.h:1174
#12 0x00007feaa5f3b460 in nsThread::ProcessNextEvent(bool, bool) (this=<optimized out>, aMayWait=<optimized out>, aResult=0x7ffe7dcc1ac7)
at /home/hiro/central/xpcom/threads/nsThread.cpp:1179

Moving to Panning and Zooming.

I ran mozregression using the STR:

• load attachment
• click on "TO DIV 05" in the top-right box
• good is if it scrolls so that the div 05 box is just under the URL bar, bad is if it scrolls more than that (usually to box 7 or 8)

mozregression pointed to bug 1453425 as the culprit which is pretty reasonable. Ryan, can you take a look? This seems like it might affect a lot of sites that are using scrollBy.

This bug is a recent unassigned regression marked as a P1 with a testcase and a regression range, kats could we find an owner and if a patch materializes evaluate an uplift to 67? Thanks

I'll take a look at it.

nsIScrollableFrame::NotifyApzTransaction() is not getting called during the scrolling animation.

(In reply to Botond Ballo [:botond] from comment #10)

nsIScrollableFrame::NotifyApzTransaction() is not getting called during the scrolling animation.

This is due to paint skipping. The paint skipping codepath picks up the main thread scroll position to send to APZ, but does not call NotifyApzTransaction().

Calling NotifyApzTransaction() every time the main thread sends its scroll offset to APZ is a requirement for correctness of relative updates.

Depends on D29061

WebRender needs a different fix because it has a different codepath where it picks up pending updates set during paint skipping.

The test is now passing on desktop both with and without WR, but it's failing on Android (interestingly, opt and pgo only, not debug):

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=a41f112aa7f32b492ccd652ce7d0880e82670856

The Android failure seems to be an issue with the test, not the fix (we scroll less than the intended amount, not more); possibly we need to wait for something before doing the check.

Not sure if it's worth investigating, or if we should just disable the test on Android, as it does provide coverage of this issue on desktop.

I'm kind of surprised the test harness is letting you do a setTimeout(..., 20). I thought that only 0-delay setTimeout calls were permitted (or you had to do some magic thing with SpecialPowers)?

I'd be ok with disabling the test on Android to land the fix, and then investigating. As far as I can tell the test is simple enough that it should pass; there might be another underlying bug somewhere.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)

I'm kind of surprised the test harness is letting you do a setTimeout(..., 20). I thought that only 0-delay setTimeout calls were permitted (or you had to do some magic thing with SpecialPowers)?

I guess we bypass that by not propagating SimpleTests's setTimeout into the child window in runSubtestsSeriallyInFreshWindows :)

Should I revise the test to use requestAnimationFrame instead?

https://hg.mozilla.org/mozilla-central/rev/8ebd99752f72
https://hg.mozilla.org/mozilla-central/rev/58093e575a85

(In reply to Botond Ballo [:botond] from comment #15)

The Android failure seems to be an issue with the test, not the fix (we scroll less than the intended amount, not more); possibly we need to wait for something before doing the check.

Actually, scrolling less than the intended amount is a behaviour I see on Android on the test page from comment 4 as well, so I guess it was a problem with the fix, after all.

(In reply to Botond Ballo [:botond] from comment #20)

Actually, scrolling less than the intended amount is a behaviour I see on Android on the test page from comment 4 as well, so I guess it was a problem with the fix, after all.

I'll track that in bug 1547435 since that's the test change we'll want to make for it anyways.

Given that we are 8 days away from the 67 release date, and that the fix for this bug has caused an Android regression (bug 1547435) whose fix would also have to be uplifted, I am now leaning towards not uplifting this to 67, letting it ride the 68 train instead. I'm open to being convinced otherwise if someone has strong opinions.

We build RC tomrrow, let's let it ride the 68 train.