1445419 - AsyncPanZoomController::CanScroll mistakenly called CanScrollWithWheel without distinguishing between wheel scrolling and non-wheel scrolling

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[張俊芝(ZHANG Junzhi)]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180123231643

Comment 1

[張俊芝(ZHANG Junzhi)]

Scrollablab doesn't mean exactly the same thing as wheel-scrollable.

However, in AsyncPanZoomController, CanScroll(const InputData& aEvent) mistakenly calls CanScrollWithWheel without distinguishing between wheel scrolling and non-wheel scrolling, which can potentially lead to wrong APZC targets during the hittesting stage.

Comment 2

[張俊芝(ZHANG Junzhi)]

Attachment: Bug 1445419 - Makes AsyncPanZoomController::CanScroll distinguish between wheel scrolling and non-wheel scrolling

Scrollable doesn't mean exactly the same thing as wheel-scrollable.

However, AsyncPanZoomController::CanScroll(const InputData& aEvent) mistakenly
calls CanScrollWithWheel without distinguishing between wheel scrolling and
non-wheel scrolling, which may potentially lead to wrong APZC targets during the
hittesting stage.

Review commit: https://reviewboard.mozilla.org/r/227504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227504/

Comment 3

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

Comment on attachment 8958597 [details]
Bug 1445419 - Makes AsyncPanZoomController::CanScroll distinguish between wheel scrolling and non-wheel scrolling

https://reviewboard.mozilla.org/r/227504/#review233432

::: commit-message-00ef5:1
(Diff revision 1)
> +Bug 1445419 - Tiny fix. Makes AsyncPanZoomController::CanScroll distinguish between wheel scrolling and non-wheel scrolling r?kats

Drop the "Tiny fix" sentence from the commit message

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1981
(Diff revision 1)
>  }
>  
>  bool
> +AsyncPanZoomController::CanScroll(const ParentLayerPoint& aDelta) const
> +{
> +  return mX.CanScroll(aDelta.x) || mY.CanScroll(aDelta.y);

This needs to hold the mRecursiveMutex here, same as CanScrollWithWheel. Otherwise looks good.

Comment 4

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8958597 [details]
Bug 1445419 - Makes AsyncPanZoomController::CanScroll distinguish between wheel scrolling and non-wheel scrolling

https://reviewboard.mozilla.org/r/227504/#review233432

> This needs to hold the mRecursiveMutex here, same as CanScrollWithWheel. Otherwise looks good.

Maybe I missed something, but I am a little confused.

The comment for |mRecursiveMutex| doesn't mention that it protects |mX| and |mY|, is the comment outdated or unintentionally omissive that needs to be updated?

  // Protects |mFrameMetrics|, |mLastContentPaintMetrics|, and |mState|.
  // Before manipulating |mFrameMetrics| or |mLastContentPaintMetrics|, the
  // monitor should be held. When setting |mState|, either the SetState()
  // function can be used, or the monitor can be held and then |mState| updated.
  // IMPORTANT: See the note about lock ordering at the top of APZCTreeManager.h.
  // This is mutable to allow entering it from 'const' methods; doing otherwise
  // would significantly limit what methods could be 'const'.
  mutable RecursiveMutex mRecursiveMutex;

Comment 5

[張俊芝(ZHANG Junzhi)]

> This needs to hold the mRecursiveMutex here, same as CanScrollWithWheel.
> Otherwise looks good.

I read every |mX| and |mY| in |AsyncPanZoomController|, and find that most of them do hold |mRecursiveMutex|, but there are exceptions:

const ParentLayerPoint AsyncPanZoomController::GetVelocityVector() const {
  return ParentLayerPoint(mX.GetVelocity(), mY.GetVelocity());
}

void AsyncPanZoomController::SetVelocityVector(const ParentLayerPoint& aVelocityVector) {
  mX.SetVelocity(aVelocityVector.x);
  mY.SetVelocity(aVelocityVector.y);
}

So I am wondering what is the specific rule on when we should hold |mRecursiveMutex|.

Comment 6

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

Axis::CanScroll(ParentLayerCoord) calls Axis::CanScroll() [1] which calls Axis::GetPageLength() [2] which calls Axis::GetFrameMetrics() [3] which calls AsyncPanZoomController::GetFrameMetrics() [4] which asserts that we have the mutex [5]. So somewhere along this call stack the mutex needs to be held. It's not that it needs to be held for *any* call into mX or mY, but some calls. (And yeah, this is gross, and if you want to do cleanup on this, you're welcome to, but it would be better to defer that until after you're done the autodir changes, or it will be a big distraction.)

[1] https://searchfox.org/mozilla-central/rev/8fa0b32c84f924c6809c690117dbd59591f79607/gfx/layers/apz/src/Axis.cpp#347
[2] https://searchfox.org/mozilla-central/rev/8fa0b32c84f924c6809c690117dbd59591f79607/gfx/layers/apz/src/Axis.cpp#342
[3] https://searchfox.org/mozilla-central/rev/8fa0b32c84f924c6809c690117dbd59591f79607/gfx/layers/apz/src/Axis.cpp#481
[4] https://searchfox.org/mozilla-central/rev/8fa0b32c84f924c6809c690117dbd59591f79607/gfx/layers/apz/src/Axis.cpp#493
[5] https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/gfx/layers/apz/src/AsyncPanZoomController.cpp#4038

Comment 7

[張俊芝(ZHANG Junzhi)]

Comment on attachment 8958597 [details]
Bug 1445419 - Makes AsyncPanZoomController::CanScroll distinguish between wheel scrolling and non-wheel scrolling

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227504/diff/1-2/

Comment 8

[張俊芝(ZHANG Junzhi)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Axis::CanScroll(ParentLayerCoord) calls Axis::CanScroll() [1] which calls
> Axis::GetPageLength() [2] which calls Axis::GetFrameMetrics() [3] which
> calls AsyncPanZoomController::GetFrameMetrics() [4] which asserts that we
> have the mutex [5]. So somewhere along this call stack the mutex needs to be
> held. It's not that it needs to be held for *any* call into mX or mY, but
> some calls. (And yeah, this is gross, and if you want to do cleanup on this,
> you're welcome to, but it would be better to defer that until after you're
> done the autodir changes, or it will be a big distraction.)
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 8fa0b32c84f924c6809c690117dbd59591f79607/gfx/layers/apz/src/Axis.cpp#347
> [2]
> https://searchfox.org/mozilla-central/rev/
> 8fa0b32c84f924c6809c690117dbd59591f79607/gfx/layers/apz/src/Axis.cpp#342
> [3]
> https://searchfox.org/mozilla-central/rev/
> 8fa0b32c84f924c6809c690117dbd59591f79607/gfx/layers/apz/src/Axis.cpp#481
> [4]
> https://searchfox.org/mozilla-central/rev/
> 8fa0b32c84f924c6809c690117dbd59591f79607/gfx/layers/apz/src/Axis.cpp#493
> [5]
> https://searchfox.org/mozilla-central/rev/
> 99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/gfx/layers/apz/src/
> AsyncPanZoomController.cpp#4038

Thank you for the information. I am not going to find all the gross relationship, I think due to the complicated relationship the list may be hard to maintain, I would prefer adding a comment alerting the indirect usage of those mRecursiveMutex-protected members when I am reforming the patch for autodir scrolling.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/391be7bb77bc
Makes AsyncPanZoomController::CanScroll distinguish between wheel scrolling and non-wheel scrolling r=kats

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/391be7bb77bc