1857288 - APZCTreeManager::PrepareNodeForLayer creates a AsyncPanZoomController but doesn't store it in an owning RefPtr

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Daniel Holbert [:dholbert]]

While poking around AsyncPanZoomController lifetime to try to understand bug 1853246, I noticed a case where we create an AsyncPanZoomController instance without putting it in an owning RefPtr (or otherwise ensuring that it has a nonzero refcount). This could create dangerous situations; if its refcount is left at zero, then it's "unstable" in the sense that any temporary RefPtr stack-variable that it happens to get stored in (e.g. buried in some helper function somewhere) will destroy the object when that temporary goes out of scope.

Here's what I'm looking at in particular:
https://searchfox.org/mozilla-central/rev/d436104fc6c31677b08a851796bead25153be699/gfx/layers/apz/src/APZCTreeManager.cpp#1158,1229

AsyncPanZoomController* apzc = nullptr;
...
    apzc = NewAPZCInstance(aLayersId, geckoContentController);

That call to NewAPZCInstance() is just a wrapper for new AsyncPanZoomController, which probably returns an object with zero refcount (as all refcounted objects start out), which expects to be stored in an owning reference in order to keep it reliably alive.

botond, could you take a look here? I'm not sure what object eventually gets an owning reference[1] to the freshly-allocated apzc, but maybe we should make that ownership-transfer a bit more immediate/explicit right after construction? (We might want to make NewAPZCInstance to return an already_AddRefed type to nudge the caller to take ownership immediately, too).

Alternately/also, maybe the local variable apzc should be a RefPtr; but that's potentially unnecessary for cases where we're not freshly allocating. And it'd probably be unnecessary for this case where we are freshly allocating, too, if we get the object to its owner ASAP.

[1] side note RE persistent references to this object: in some cases this fresh apzc might be kept alive by its own mGestureEventListener member (which has a RefPtr back to it and hence gives it a refcount of 1 instead of 0, for as long as mGestureEventListener is alive); but it also might not, since in some cases we leave its mGestureEventListener member-var unset. From a quick skim of this code, I'm not sure what else, if anything, ends up with an owning reference to it.

Comment 1

[Botond Ballo [:botond]]

Thanks for the analysis. I don't think there's an actual lifetime issue here, as the HitTestingTreeNode takes ownership of the AsyncPanZoomController pretty promptly on this line, but we can make that more explicit in the code.

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1857288 - Make ownership transfer after creation of AsyncPanZoomController more explicit. r=dholbert

Comment 3

[Daniel Veditz [:dveditz]]

sec-other based on comment 1: keeping hidden while we continue to audit similar code to bug 1853246

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a0a61165aa2
Make ownership transfer after creation of AsyncPanZoomController more explicit. r=dholbert

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3a0a61165aa2

Comment 6

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter