Skip to content

Commit

Permalink
Bug 1733465 part 1: Store nsViewManager::mRootViewManager in a RefPtr…
Browse files Browse the repository at this point in the history
… instead of manually managing its reference count. r=tnikkel

This patch is just a refactoring which shouldn't change behavior.

Before this patch, mRootViewManager is a bit of an odd hybrid in terms of its
ownership semantics.  If it's pointing to `this`, then it's not an owning
reference (i.e. we don't AddRef or Release it), vs. if it's pointing to some
other instance, then it *is* an owning reference (i.e. we *do* AddRef and
Release it).

After this patch, we change things such that it's unconditionally an owning
reference, with a null value having special meaning.

In particular:
(a) Now, we'll store it in a RefPtr and let that manage the reference counting.
(b) Now, we'll never explicitly assign it to 'this'; instead, we set it to null
and we interpret a null value as an indication that 'this' is the root.

Fortunately, this variable doesn't have many direct usages, so this slight
change in meaning/invariatnts doesn't require very much code to be updated.
This variable is mostly used via the infallible RootViewManager() getter. This
patch updates that getter in accordance with the new contract, and the callers
don't need to worry about this change.

Differential Revision: https://phabricator.services.mozilla.com/D127178
  • Loading branch information
dholbert committed Oct 1, 2021
1 parent 08c8631 commit bfb72ec
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 16 deletions.
14 changes: 3 additions & 11 deletions view/nsViewManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ nsViewManager::nsViewManager()
: mPresShell(nullptr),
mDelayedResize(NSCOORD_NONE, NSCOORD_NONE),
mRootView(nullptr),
mRootViewManager(this),
mRefreshDisableCount(0),
mPainting(false),
mRecursiveRefreshPending(false),
Expand All @@ -77,10 +76,7 @@ nsViewManager::~nsViewManager() {
mRootView = nullptr;
}

if (!IsRootVM()) {
// We have a strong ref to mRootViewManager
NS_RELEASE(mRootViewManager);
}
mRootViewManager = nullptr;

NS_ASSERTION(gViewManagers != nullptr, "About to use null gViewManagers");

Expand Down Expand Up @@ -1003,17 +999,13 @@ void nsViewManager::GetLastUserEventTime(uint32_t& aTime) {

void nsViewManager::InvalidateHierarchy() {
if (mRootView) {
if (!IsRootVM()) {
NS_RELEASE(mRootViewManager);
}
mRootViewManager = nullptr;
nsView* parent = mRootView->GetParent();
if (parent) {
mRootViewManager = parent->GetViewManager()->RootViewManager();
NS_ADDREF(mRootViewManager);
NS_ASSERTION(mRootViewManager != this,
"Root view had a parent, but it has the same view manager");
} else {
mRootViewManager = this;
}
// else, we are implicitly our own root view manager.
}
}
16 changes: 11 additions & 5 deletions view/nsViewManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,11 @@ class nsViewManager final {

void InvalidateView(nsView* aView, const nsRect& aRect);

nsViewManager* RootViewManager() const { return mRootViewManager; }
bool IsRootVM() const { return this == RootViewManager(); }
nsViewManager* RootViewManager() const {
return mRootViewManager ? mRootViewManager.get()
: const_cast<nsViewManager*>(this);
}
bool IsRootVM() const { return !mRootViewManager; }

// Whether synchronous painting is allowed at the moment. For example,
// widget geometry changes can cause synchronous painting, so they need to
Expand All @@ -400,9 +403,12 @@ class nsViewManager final {
nsSize mDelayedResize;

nsView* mRootView;
// mRootViewManager is a strong ref unless it equals |this|. It's
// never null (if we have no ancestors, it will be |this|).
nsViewManager* mRootViewManager;

// mRootViewManager is a strong reference to the root view manager, unless
// |this| is the root, in which case mRootViewManager is null. Callers
// should use RootViewManager() (which handles that case) rather than using
// mRootViewManager directly.
RefPtr<nsViewManager> mRootViewManager;

// The following members should not be accessed directly except by
// the root view manager. Some have accessor functions to enforce
Expand Down

0 comments on commit bfb72ec

Please sign in to comment.