1164557 - Assertion failure in Axis::StartOverscrollAnimation()

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

The assertion in Axis::StartOverscrollAnimation(), which I added in bug 1152051, fired for me while panning and zooming around on http://people.mozilla.org/~bballo/iframe.html.

Relevant part of stack trace:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 2474.2533]
0xb452cdc2 in StartOverscrollAnimation (this=<optimized out>, aVelocity=<optimized out>) at ../../../gfx/layers/apz/src/Axis.cpp:237
warning: Source file is more recent than executable.
237       MOZ_ASSERT(mFirstOverscrollAnimationSample == 0 &&
(gdb) i s
#0  0xb452cdc2 in StartOverscrollAnimation (this=<optimized out>, aVelocity=<optimized out>) at ../../../gfx/layers/apz/src/Axis.cpp:237
#1  mozilla::layers::Axis::StartOverscrollAnimation (this=<optimized out>, aVelocity=<optimized out>) at ../../../gfx/layers/apz/src/Axis.cpp:235
#2  0xb452efb0 in OverscrollAnimation (aVelocity=..., aApzc=..., this=0xaa48ab00) at ../../../gfx/layers/apz/src/AsyncPanZoomController.cpp:661
#3  mozilla::layers::AsyncPanZoomController::StartOverscrollAnimation (this=this@entry=0xa8381000, aVelocity=...) at ../../../gfx/layers/apz/src/AsyncPanZoomController.cpp:2237
#4  0xb452efec in mozilla::layers::AsyncPanZoomController::SnapBackIfOverscrolled (this=this@entry=0xa8381000) at ../../../gfx/layers/apz/src/AsyncPanZoomController.cpp:2470
#5  0xb452f020 in mozilla::layers::OverscrollHandoffChain::SnapBackOverscrolledApzc (this=0xaaf338e0, aStart=<optimized out>) at ../../../gfx/layers/apz/src/OverscrollHandoffState.cpp:110
#6  0xb41edb62 in DispatchToMethod<IPC::Channel, bool (IPC::Channel::*)(IPC::Message*), IPC::Message*> (arg=..., method=
    (bool (IPC::Channel::*)(IPC::Channel * const, IPC::Message *)) 0xb452f001 <mozilla::layers::OverscrollHandoffChain::SnapBackOverscrolledApzc(mozilla::layers::AsyncPanZoomController const*) const>, obj=0xaaf338e0) at ../../../ipc/chromium/src/base/tuple.h:393
#7  RunnableMethod<IPC::Channel, bool (IPC::Channel::*)(IPC::Message*), Tuple1<IPC::Message*> >::Run (this=<optimized out>) at ../../../ipc/chromium/src/base/task.h:310
#8  0xb4533dc2 in mozilla::layers::AsyncPanZoomController::AdvanceAnimations (this=0xa871a000, aSampleTime=...) at ../../../gfx/layers/apz/src/AsyncPanZoomController.cpp:2692
#9  0xb4546c68 in mozilla::layers::SampleAPZAnimations (aLayer=..., aSampleTime=...) at ../../../gfx/layers/composite/AsyncCompositionManager.cpp:542

Comment 1

[Botond Ballo [:botond]]

I've also seen the 'MOZ_ASSERT(mOverscroll <= 0)' in Axis::OverscrollBy() to fire. Not sure if that's related.

Comment 2

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #1)
> I've also seen the 'MOZ_ASSERT(mOverscroll <= 0)' in Axis::OverscrollBy() to
> fire. Not sure if that's related.

This is pretty priceless: Axis::DisplacementWillOverscrollAmount() is reporting that a requested displacement of 0 causes overscroll of 0.000122, due to pre-existing rounding error that causes the values of GetCompositionEnd() and GetPageEnd() to differ by that amount. If this happens while we're already overscrolled in the negative direction, attempting to overscroll by 0.000122 asserts.

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: bz://1164557/botond

/r/8713 - Bug 1164557 - Do not start an overscroll animation if one is already running. r=kats
/r/8715 - Bug 1164557 - Use COORDINATE_EPSILON in IsZero(). r=kats
/r/8717 - Bug 1164557 - Weed out spurious calls to Axis::OverscrollBy() caused by rounding error r=kats

Pull down these commits:

hg pull -r 3e7346c25dd230c7dac46a09777bbb10577922e9 https://reviewboard-hg.mozilla.org/gecko/

Comment 4

[Botond Ballo [:botond]]

The first patch fixes the assertion in comment 0. The problem there was that if a child scrollable was flung while a parent scrollable was doing an overscroll animation, the SnapBackOverscrolledApzc() call at the end of the child's fling could trigger a new overscroll animation on the parent. The fix is to only start on overscroll animation in SnapBackIfOverscrolled() if there isn't already an overscroll animation in progress.

The second and third patches fix the assertion in comment 1. The second patch alone isn't sufficient, because the overscroll could have the spurious near-zero amount in one coordinate only, and IsZero(), which checks both coordinates of a point, doesn't weed that out. (Given the third patch, the second patch then isn't strictly necessary, but I think it's good to use COORDINATE_EPSILON consistently.)

Comment 5

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

https://reviewboard.mozilla.org/r/8713/#review7503

Comment 6

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

https://reviewboard.mozilla.org/r/8717/#review7505

Ship It!

Comment 7

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

https://reviewboard.mozilla.org/r/8715/#review7507

A little uneasy about this one since it affects more callsites than the one we care about for this bug, but after looking at them all I can't think of anything that might break as a result.

Comment 8

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

Comment on attachment 8605454 [details]
MozReview Request: bz://1164557/botond

https://reviewboard.mozilla.org/r/8711/#review7509

Comment 9

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cdfbf778323

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c65b97cfc0c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/95650b6fd83c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b26cefc284b0

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c65b97cfc0c7
https://hg.mozilla.org/mozilla-central/rev/95650b6fd83c
https://hg.mozilla.org/mozilla-central/rev/b26cefc284b0

Comment 13

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1164557 - Do not start an overscroll animation if one is already running. r=kats

Comment 14

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1164557 - Use COORDINATE_EPSILON in IsZero(). r=kats

Comment 15

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1164557 - Weed out spurious calls to Axis::OverscrollBy() caused by rounding error r=kats