1270638 - Initialize mRefCnt to 1, instead of 0, when creating new reference counted objects

Status: NEW

(Core :: MFBT, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently, when creating new reference counted objects, the reference count starts at 0. This means that if the object passes its `this` pointer somewhere which ends up taking a strong reference, it can be destroyed before its constructor completes and the caller AddRefs the object for the first time.

It would be nice to have the reference count of these objects start at 1 instead of 0, which would mean that the object would not be able to die before its constructor completes, as it has at least one reference keeping it alive. For this to work, however, the way that reference counted objects are constructed would have to be changed.

Currently most reference counted objects are constructed as follows:

RefPtr<Object> o = new Object();

We also provide the MakeAndAddRef method, which creates the object, AddRefs it, and returns an already_AddRefed<T>. If we change all instances of the above to instead use MakeAndAddRef, and prohibit the use of `new` on objects with mRefCnt members outside of mfbt, we can change MakeAndAddRef to not addref the pointer, and change the default value for mRefCnt to be 1. This would prevent objects from being destructed during their constructors.

The first step in this would be to change new -> MakeAndAddRef, which ideally could be done with some form of rewriting tool, to avoid the extra effort. Then an analysis can be written to prevent new uses of `new` from entering the code base, and finally the change to the default value of mRefCnt can be made.

Comment 1

[Mike Hommey [:glandium]]

(In reply to Michael Layzell [:mystor] from comment #0)
> it can be destroyed before its constructor completes

How?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Hommey [:glandium] from comment #1)
> (In reply to Michael Layzell [:mystor] from comment #0)
> > it can be destroyed before its constructor completes
> 
> How?

class C {
  // refcnt stuff
  C() {
    RefPtr p = this;
  }
};

This would cause the new instance to be AddRef-ed to have a mRefCnt of 1, then that reference would be destroyed, and the object would be destructed, before the constructor returns to the caller.

This could also happen where the RefPtr is taken by a method which C calls in its constructor, rather than C creating the RefPtr directly.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One issue to be careful of is making sure that the increment-to-1 that would (with this change) be implicit in the constructor is properly logged to the reference-count logging/debugging tools.  Be sure to consider base/derived classes and how they are logged (i.e., potential for logging as a different class name or at an offset address).

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I was going to say things like 'why not just initialize the ref-count to 1? Or shift its meaning down 1?', as it seemed like a more localized change; but I'm not sure it's actually possible with how RefPtr works -- but if there was such a solution, I would prefer it, to keep it small.

However...
Instead of focusing on ref-counts starting at 1 and removing some new's as just a necessary step to do that, how about we flip things and make it a publicized primary goal to actually remove *all* naked new's (i.e., even for non-refcounted classes) in favor of std-like Make...<T>() functions, to better expose ownership from the get-go?
And as a side-benefit of that, we could easily change the initial ref-count value to avoid these RefPtr-in-constructor issues.

(I admit it may just be a rephrasing of the problem, that would have the same solution as you propose -- but it seems to me a more noble goal that more developers would welcome.)

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Gerald Squelart [:gerald] from comment #4)
> I was going to say things like 'why not just initialize the ref-count to 1?
> Or shift its meaning down 1?', as it seemed like a more localized change;
> but I'm not sure it's actually possible with how RefPtr works -- but if
> there was such a solution, I would prefer it, to keep it small.
> 
> However...
> Instead of focusing on ref-counts starting at 1 and removing some new's as
> just a necessary step to do that, how about we flip things and make it a
> publicized primary goal to actually remove *all* naked new's (i.e., even for
> non-refcounted classes) in favor of std-like Make...<T>() functions, to
> better expose ownership from the get-go?
> And as a side-benefit of that, we could easily change the initial ref-count
> value to avoid these RefPtr-in-constructor issues.
> 
> (I admit it may just be a rephrasing of the problem, that would have the
> same solution as you propose -- but it seems to me a more noble goal that
> more developers would welcome.)

Yeah, I think that would be the best solution to the problem. I wanted to restrict the scope of the problem to one which fixes a potential soundness issue (the possibility for an object to be destroyed before its constructor completes), rather than it being focused on something which could be bikeshedded (like whether or not naked "new"s are OK).

But yes, the first goal would be to replace everything with std-like Make...<T>() functions, and then the potential soundness issue would be an easy fix which would fall out of all of that hard work.

I'm hoping that a large amount of the work required to perform the new->Make<T> changes can be done automatically with a rewriting tool, but we'll have to see how it goes.

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #3)
> One issue to be careful of is making sure that the increment-to-1 that would
> (with this change) be implicit in the constructor is properly logged to the
> reference-count logging/debugging tools.  Be sure to consider base/derived
> classes and how they are logged (i.e., potential for logging as a different
> class name or at an offset address).

I'm hoping that I'll get help from someone like you who knows what they're doing when it comes to actually making the change to starting the reference count at one. For now, the biggest hurdle is eliminating the direct calls to new, and I think worrying about the specifics of reference count logging before we overcome that is a bit pre-emptive.