1477335 - AsyncPanZoomController::mFrameMetrics being a reference provides a loophole around const-correctness

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Botond Ballo [:botond]]

I was surprised to discover today that you can mutate AsyncPanZoomController::mFrameMetrics in a |const| method of AsyncPanZoomController (without mFrameMetrics being |mutable|, or casting away constness).

I've reduced the problematic code pattern to the following:

===================================
struct FrameMetrics {
  int mScrollOffset = 0;
  void ScrollBy(int amount) {
    mScrollOffset += amount;
  }
};

struct ScrollMetadata {
  FrameMetrics mMetrics;
};

struct AsyncPanZoomController {
  ScrollMetadata mMetadata;
  FrameMetrics& mMetrics;
 
  AsyncPanZoomController() : mMetrics(mMetadata.mMetrics) {}

  void RegularOldConstMethod() const {
    mMetrics.ScrollBy(10);  // oops; compiles
  }
};
===================================

It looks like the fact that mFrameMetrics has reference type provides a loophole around const-correctness.

We should close this loophole, by replacing mFrameMetrics with a method that accesses mScrollMetadata.mMetrics.

Comment 1

[Botond Ballo [:botond]]

Fixing this would probably make a nice mentored bug.

Comment 2

[Mitch Ament]

I would like to work on this bug

Comment 3

[Botond Ballo [:botond]]

Great, thanks for your interest!

The first step is to check out the Firefox source code and build it. Instructions for that can be found here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Let me know once you've done that, and I'll write some more about how to get started with this bug itself.

Comment 4

[Mitch Ament]

Thank you for mentoring me on this bug fix. I built Firefox from source a little earlier today to play around with it a bit so I am ready to go!

Comment 5

[Botond Ballo [:botond]]

Great!

So the idea is that the class AsyncPanZoomController has a member named mFrameMetrics [1], which is of reference type, and only exists for convenience (it always refers to mScrollMetadata.mMetrics).

This reference variable unintentionally allows us to get around const-correctness, because it allows mutating mScrollMetadata.mMetrics through the reference even in member functions that are marked const. We strive for const-correctness, so we'd like to close this loophole.

To do that, we want to remove the mFrameMetrics member, and replace it with a pair of getter functions that provide access to mScrollMetadata.mMetrics. We'll need two: one marked |const| which returns |const FrameMetrics&|, and one not marked |const| which returns |FrameMetrics&|.

As part of this change, references to mFrameMetrics throughout the class need to be replaced with calls to one of these getters.

To keep things nice and short, I would suggest calling the getters "Metrics()".

(You may notice that there is an existing getter called GetFrameMetrics(), and wonder why I'm not suggesting to use that. This is because that getter makes a lock-related assertion. In theory, every access to mFrameMetrics _should_ pass that assertion, but in practice there will almost certainly be some that don't. We want to fix that too, and you could work on that too if you're interested (in a separate bug), but let's fix one thing at a time.)

There's a possibility that as part of this refactor, you may run into existing const-correctness violations that were covered up by this loophole, where we were mutating mScrollMetadata.mMetrics in a non-const method. If you run into such cases, and it's not clear how to fix them, feel free to post here and ask for suggestions.

That should be enough to get you started. If you have any questions, don't hesitate to ask!

[1] https://searchfox.org/mozilla-central/rev/10e6320abc7fb0479032f0e95e9d61e4be194b9e/gfx/layers/apz/src/AsyncPanZoomController.h#893

Comment 6

[Mitch Ament]

I am a little confused with the errors I am getting from a particular function "IsHorizontalContentRightToLeft()"

It was originally called as return mFrameMetrics.IsHorizontal... for function                                        "bool AsyncPanZoomController::IsContentOfHonouredTargetRightToLeft(bool aHonoursRoot) const"

https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#2143

In my code I first changed it to Metrics_Const() as it appeared to be an inspector function and I assumed it would require a constant caller. This gave the error "argument to member function 'IsHorizontalContentRightToLeft' has type 'const:Mozilla:layers:FrameMetrics but function is not marked constant"

Changing it to Metrics().IsHorizontal... gave the error "argument to member function 'Metrics' has type 'const mozilla::layers::AsyncPanZoomController', but function is not marked const"

And as a check I also prefixed it with mScrollMetadata.GetMetrics() instead and recieved the same error as I did with Metrics_Const().

I apologize if this is a simple question but I have been unable to figure out a solution so far. Thank you for your help!

Below are the function definitions for Metrics() and Metrics_Const() in AsyncPanZoom.h & .cpp

AsyncPanZoom.h

const FrameMetrics& Metrics_Const() const;
FrameMetrics& Metrics();

AsyncPanZoom.cpp

FrameMetrics& AsyncPanZoomController::Metrics() {
  return mScrollMetadata.GetMetrics();
}

const FrameMetrics& AsyncPanZoomController::Metrics_Const() const {
  return mScrollMetadata.GetMetrics();;
}

Comment 7

[Botond Ballo [:botond]]

(In reply to mga140130 from comment #6)
> I am a little confused with the errors I am getting from a particular
> function "IsHorizontalContentRightToLeft()"
> 
> It was originally called as return mFrameMetrics.IsHorizontal... for
> function                                        "bool
> AsyncPanZoomController::IsContentOfHonouredTargetRightToLeft(bool
> aHonoursRoot) const"
> 
> https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/
> AsyncPanZoomController.cpp#2143
> 
> In my code I first changed it to Metrics_Const() as it appeared to be an
> inspector function and I assumed it would require a constant caller. This
> gave the error "argument to member function 'IsHorizontalContentRightToLeft'
> has type 'const:Mozilla:layers:FrameMetrics but function is not marked
> constant"
> 
> Changing it to Metrics().IsHorizontal... gave the error "argument to member
> function 'Metrics' has type 'const mozilla::layers::AsyncPanZoomController',
> but function is not marked const"
> 
> And as a check I also prefixed it with mScrollMetadata.GetMetrics() instead
> and recieved the same error as I did with Metrics_Const().

Good question. This is an example of a const-correctness bug that has been covered up by this loophole.

In this case, the bug is in FrameMetrics: IsHorizontalContentRightToLeft() should be marked const but isn't. Marking it const should resolve the issue.

> const FrameMetrics& Metrics_Const() const;
> FrameMetrics& Metrics();

I probably should have mentioned this, but the two functions can use the same name (C++ allows overloading a method on whether it is marked const):

const FrameMetrics& Metrics() const;
FrameMetrics& Metrics();

Comment 8

[Mitch Ament]

I thought that c++ allowed const overloading but I wasn't sure if that was what was causing the error, thank you for clarifying. The code compiles now and it did not compile when I added a few extra const correctness tests as expected.

Are there any tests I should run other than Compiled Code for C++ and RefTest? For now I am going to run those and I will submit a patch file after that is complete.

Comment 9

[Botond Ballo [:botond]]

Great, thanks!

For this change, running |mach gtest| locally should be sufficient. We have a "Try" server that will run tests in the cloud, I'll push the patch there before committing it to make sure that other test suites are passing.

Comment 10

[Mitch Ament]

Attachment: moz_APZ.patch

Sorry about the wait on this, I planned to submit this patch before I left for dinner but gtest didn't finish in time.

This patch file should hopefully be correctly commented already.

Comment 11

[Botond Ballo [:botond]]

Did you attach the wrong patch file? It doesn't contain changes to AsyncPanZoomController but to unrelated files like EventManager.jsm.

Comment 12

[Mitch Ament]

Attachment: APZ patch fixes const correctness loophole caused by mFrameMetrics

Sorry about that I am not sure what happened there. This should be correct

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1477335 - Replace reference type mFrameMetrics with two getter functions, |Metrics()| and |Metrics() Const| which both return mScrollMetadata.mMetrics. Fixes const correctness loophole.

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

Comment 14

[Botond Ballo [:botond]]

I uploaded the patch to MozReview just to make it easier to review (MozReview's diff viewer highlights which part of a line changed, while Splinter just highlights the whole line).

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8994251 [details]
Bug 1477335 - Replace reference type mFrameMetrics with two getter functions, |Metrics()| and |Metrics() Const| which both return mScrollMetadata.mMetrics. Fixes const correctness loophole.

https://reviewboard.mozilla.org/r/258838/#review265812

Thanks! This is looking pretty good. I have a few comments below.

When uploading an updated version of the patch, please upload it in a format that includes the commit message (see [1] for how to format the commit message). Let me know if you need help getting your version control system to generate a patch in the right format.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment

::: gfx/layers/apz/src/AsyncPanZoomController.h:520
(Diff revision 1)
>  
>    void NotifyMozMouseScrollEvent(const nsString& aString) const;
>  
>    bool OverscrollBehaviorAllowsSwipe() const;
>  
> +  //|Metrics()| and |Metrics() const| are getter functions that have replaced mFrameMetrics

This comment can be omitted.

On the other hand, as AsyncPanZoomController stores several FrameMetrics variables, it would be useful to document which one this returns:

// These getters return mScrollMetadata.mMetrics.

::: gfx/layers/apz/src/AsyncPanZoomController.h:526
(Diff revision 1)
> +  //in order to maintain const-correctness. They return |FrameMetrics&| and
> +  // |const FrameMetrics&| respectively.
> +  //const FrameMetrics& Metrics() const;
> +  //FrameMetrics& Metrics();
> +
> +const FrameMetrics& Metrics() const;

Please indent these declarations one level, like other function declarations in the class.

::: gfx/layers/apz/src/AsyncPanZoomController.h:900
(Diff revision 1)
>    already_AddRefed<GestureEventListener> GetGestureEventListener() const;
>  
>    PlatformSpecificStateBase* GetPlatformSpecificState();
>  
>  protected:
> -  // Both |mFrameMetrics| and |mLastContentPaintMetrics| are protected by the
> +  // |mLastContentPaintMetrics| is protected by the

The original comment was a bit out of date, and should have mentioned |mScrollMetadata| (which we are keeping) instead of |mFrameMetrics|.

Please replace |mFrameMetrics| with |mScrollMetadata| in the original comment, rather than deleting it.

::: gfx/layers/apz/src/AsyncPanZoomController.h:904
(Diff revision 1)
> -  // Both |mFrameMetrics| and |mLastContentPaintMetrics| are protected by the
> -  // monitor. Do not read from or modify either of them without locking.
> +  // |mLastContentPaintMetrics| is protected by the
> +  // monitor. Do not read from or modify it without locking.
>    ScrollMetadata mScrollMetadata;
> -  FrameMetrics& mFrameMetrics;  // for convenience, refers to mScrollMetadata.mMetrics
>  
> -  // Protects |mFrameMetrics|, |mLastContentPaintMetrics|, and |mState|.
> +

One blank line here is sufficient.

::: gfx/layers/apz/src/AsyncPanZoomController.h:907
(Diff revision 1)
> -  FrameMetrics& mFrameMetrics;  // for convenience, refers to mScrollMetadata.mMetrics
>  
> -  // Protects |mFrameMetrics|, |mLastContentPaintMetrics|, and |mState|.
> -  // Before manipulating |mFrameMetrics| or |mLastContentPaintMetrics|, the
> +
> +
> +
> +  // Protects |mLastContentPaintMetrics| and |mState|.

Likewise here, replace |mFrameMetrics| with |mScrollMetadata| rather than removing it.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:831
(Diff revision 1)
>       mX(this),
>       mY(this),
>       mPanDirRestricted(false),
>       mPinchLocked(false),
>       mZoomConstraints(false, false,
> -        mFrameMetrics.GetDevPixelsPerCSSPixel() * kViewportMinScale / ParentLayerToScreenScale(1),
> +        mScrollMetadata.GetMetrics().GetDevPixelsPerCSSPixel() * kViewportMinScale / ParentLayerToScreenScale(1),

We can call Metrics() here too.

(The only thing to be mindful of in situations like this, is that data members are initialized in order of declaration. So in this case, since we're using mScrollMetadata in the initializer of mZoomConstraints, it's important that mScrollMetadata is declared before mZoomConstraints, which it is.)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:2142
(Diff revision 1)
>  {
>    if (aHonoursRoot) {
>      return mScrollMetadata.IsAutoDirRootContentRTL();
>    }
>    RecursiveMutexAutoLock lock(mRecursiveMutex);
> -  return mFrameMetrics.IsHorizontalContentRightToLeft();
> +  return mScrollMetadata.GetMetrics().IsHorizontalContentRightToLeft();

We can use Metrics() here

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:4160
(Diff revision 1)
>        APZC_LOG("%p detected non-empty margins which probably need updating\n", this);
>        needContentRepaint = true;
>      }
>    } else {
>      // If we're not taking the aLayerMetrics wholesale we still need to pull
> -    // in some things into our local mFrameMetrics because these things are
> +    // in some things into our local Metrics().because these things are

stray period

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:4161
(Diff revision 1)
>        needContentRepaint = true;
>      }
>    } else {
>      // If we're not taking the aLayerMetrics wholesale we still need to pull
> -    // in some things into our local mFrameMetrics because these things are
> -    // determined by Gecko and our copy in mFrameMetrics may be stale.
> +    // in some things into our local Metrics().because these things are
> +    // determined by Gecko and our copy in Metrics().may be stale.

stray period

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:4715
(Diff revision 1)
>        if (!mMetricsSharingController->StartSharingMetrics(mem, handle, mLayersId, mAPZCId)) {
>          APZC_LOG("%p failed to share FrameMetrics with content process.", this);
>        }
>      }
>    }
> -}
> +} ;

stray semicolon

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)
> (You may notice that there is an existing getter called GetFrameMetrics(),
> and wonder why I'm not suggesting to use that. This is because that getter
> makes a lock-related assertion. In theory, every access to mFrameMetrics
> _should_ pass that assertion, but in practice there will almost certainly be
> some that don't. We want to fix that too, and you could work on that too if
> you're interested (in a separate bug), but let's fix one thing at a time.)

I filed bug 1477832 for the locking issue. You're welcome to work on that as a follow-up if you're interested!

Comment 17

[Mitch Ament]

Thank you again for your mentorship on this bug! I have a project to turn in tomorrow morning but I will have the edits you requested posted by tomorrow afternoon.

I saw that someone has posted on bug 1477832 asking to take it on. Can I still work on this bug, or should I check others when I finish up here?

Comment 18

[Botond Ballo [:botond]]

(In reply to Mitch Ament from comment #17)
> I saw that someone has posted on bug 1477832 asking to take it on. Can I
> still work on this bug, or should I check others when I finish up here?

I'm happy to give you first crack at bug 1477832 :)

Comment 19

[Mitch Ament]

Attachment: APZ_Const_Correct.patch

Bug 1477335 - Replace reference type mFrameMetrics with two getter functions, |Metrics()| and |Metrics() Const| which both return mScrollMetadata.mMetrics. Fixes const correctness loophole. r=Botond

Comment 20

[Mitch Ament]

Thank you Botond! I'll start working on bug 1477832 until I hear back on this one. Sorry for the radio silence the past few days, some family came into town as a surprise.

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8994251 [details]
Bug 1477335 - Replace reference type mFrameMetrics with two getter functions, |Metrics()| and |Metrics() Const| which both return mScrollMetadata.mMetrics. Fixes const correctness loophole.

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

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8994971 [details] [diff] [review]
APZ_Const_Correct.patch

Review of attachment 8994971 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good!

Pushed to our Try server to make sure it's passing tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bf6dc10f263284f173c5ffe227def52fc7092e1

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb706953c51
Replace reference member AsyncPanZoomController::mFrameMetrics with getter functions which return mScrollMetadata.mMetrics. r=botond

Comment 24

[Botond Ballo [:botond]]

I re-worded the commit message slightly and pushed the patch to mozilla-inbound. It should merge over to mozilla-central within a day.

Comment 25

[Botond Ballo [:botond]]

(In reply to Mitch Ament from comment #20)
> I'll start working on bug 1477832 until I hear back on this one.

I posted some notes in bug 1477832 about how to get started. Feel free to ask questions if you have any!

Comment 26

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/bbb706953c51