1266651 - Introduce Ptr<> and use it with nsThread

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

One trouble with raw pointers is knowing if they are null. For non-null pointers you can usually use references. But it would be nice to have a specific type that explicit means "yes, this can be null", and give a little bit of protection.

I've written a class called Ptr<> that does exactly this. I've also written NNPtr<> ("NN" means "non-null") intended for the times when a reference isn't a suitable for a non-null pointer. Of the two, Ptr<> is the more important.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Rename nsThreadShutdownContext members

A preliminary patch.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Give nsThreadShutdownContext a proper constructor

Another preliminary patch.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Introduce Ptr<> and NNPtr<>, and use them (plus references) in nsThread

I suggest reading mfbt/Ptr.h first, and then seeing how it gets used in
practice in the other files in the patch.

I feel pretty good about converting lots of non-null pointers to references.
It requires a bit more use of '&' and '*' operators when converting between
pointers and references, but I think that's reasonable.

I fell pretty good about Ptr<>. It doesn't feel too onerous to use, and gives
some nice protections.

I'm less certain about NNPtr<>. It gets much less use than references and
Ptr<>. But it doesn't help with smart pointers like RefPtr, nsCOMPtr,
UniquePtr. A more general solution would be this not_null<> thing in
forthcoming(?) versions of C++. It would be great if it (or a reimplementation
of it) could be made to work with our smart pointer classes.

There are some "njn:" comments for things I'm undecided about.

Anyway, any and all feedback welcome. In particular, whether you think this all
seems sufficient ergonomic and useful to get wide use, or if people will kick
up a fuss.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8744179 [details] [diff] [review]
(part 3) - Introduce Ptr<> and NNPtr<>, and use them (plus references) in nsThread

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

Lots of comments.  I don't know that I'd want to see this go in yet, but seems moderately useful.

::: mfbt/Ptr.h
@@ +56,5 @@
> +// NNPtr<T>
> +// --------
> +// njn: gsl::not_null can be used for smart pointers too, viz:
> +// - not_null<char*>
> +// - not_null<unique_ptr<char>>

How compatible is the proposed class with non_null?  Can we make NNPtr work with at least UniquePtr and RefPtr?

@@ +82,5 @@
> +// Ptr<T>
> +// ------
> +// Often a pointer can be legitimately null. In such cases, the use of Ptr<T>
> +// instead of a raw pointer makes this obvious. It has the following notable
> +// properties.

I am less convinced by the need of this class; the only thing it adds is the explicit Deref bit, and I'm not completely sure that's useful in general.  I'll think about it.  Perhaps if the name were more evocative, like MaybePtr<T>?

@@ +106,5 @@
> +class Ref final
> +{
> +public:
> +  // This type is used by one of NNPtr's constructors.
> +  struct DerefT {};

I would prefer this simply be at global scope; I don't think we gain anything by it being nested inside |Ref|.

@@ +111,5 @@
> +
> +  // This operation is just like |operator*| but (a) is more obvious (and
> +  // greppable) and (b) will abort safely if the pointer is null.
> +  template <typename T>
> +  static T& Deref(T* aRawPtr)

I hear people complaining about non-standard substitutes for functionality C++ already has.

@@ +131,5 @@
> +
> +  // Initialize from a raw pointer. The dummy |DerefT| argument serves purely
> +  // for documentation purposes, as a reminder that the raw pointer argument
> +  // will be dereferenced and so must be non-null.
> +  NNPtr(const Ref::DerefT&, T* aRawPtr) { DerefAndAssign(aRawPtr); }

This means that you have:

  NNPtr(Ref::Deref(), p);

?  That is pretty verbose.  I don't know that you need the extra documentation beyond |NNPtr|, since that already indicates non-nullness, and we have release asserts for it.

@@ +135,5 @@
> +  NNPtr(const Ref::DerefT&, T* aRawPtr) { DerefAndAssign(aRawPtr); }
> +
> +  // Initialize from a reference. We still do a null check to catch the
> +  // possibility of aRef being null via undefined behaviour.
> +  explicit NNPtr(T& aRef) { DerefAndAssign(&aRef); }

It would be good to |= default| or |= delete| move/copy constructor/assignment methods.

@@ +164,5 @@
> +  T* operator->() const { return get(); }
> +  T& operator*() const  { return *get(); }
> +};
> +
> +// njn: want a StaticPtr<T> variant that doesn't auto-zero in opt builds?

Ptr<T> would be usable as a global static variable if you made the default constructor MOZ_CONSTEXPR.

@@ +177,5 @@
> +  // Default to nullptr.
> +  Ptr() : mRawPtr(nullptr) {}
> +
> +  // Construct from a raw pointer.
> +  MOZ_IMPLICIT Ptr(T* aRawPtr) : mRawPtr(aRawPtr) {}

It would be good to |= default| or |= delete| move/copy constructor/assignment methods.

@@ +196,5 @@
> +
> +  // Explicit dereference.
> +  T& Deref() const
> +  {
> +    MOZ_RELEASE_ASSERT(mRawPtr);

Might be worth a comment that null-checking prior to calling |Deref| will tend to mean that the compiler can optimize away this assert.

::: xpcom/threads/nsThread.cpp
@@ +240,2 @@
>                            bool      aAwaitingShutdownAck)
> +    : mTerminatingThread(&aTerminatingThread)

Passing a reference to a ref-counted thing and then turning it back into a pointer is kind of weird.

@@ +370,5 @@
>  nsThread::ThreadFunc(void* aArg)
>  {
>    using mozilla::ipc::BackgroundChild;
>  
> +  nsThread& self = Ref::Deref(static_cast<nsThread*>(aArg));

I think we might be better served by:

  RefPtr<nsThread> self = dont_AddRef(static_cast<nsThread*>(aArg));

?  Especially if we could make that work with NNPtr or similar.  The:

  self.Release();

call later gives me the shivers.

@@ +395,5 @@
>  
>    {
>      // Scope for MessageLoop.
>      nsAutoPtr<MessageLoop> loop(
> +      new MessageLoop(MessageLoop::TYPE_MOZILLA_NONMAINTHREAD, &self));

We could even pass NNPtr<RefPtr<T>> here?

::: xpcom/threads/nsThread.h
@@ +176,3 @@
>  
>      // This is protected by mThread->mLock.
> +    Ptr<nsChainedEventQueue> mQueue;

Presumably this should be NNPtr?  At least judging by the code here; I haven't spelunked into other code to see if this gets set to nullptr somewhere.

Comment 5

[Nicholas Nethercote [inactive]]

> @@ +82,5 @@
> > +// Ptr<T>
> 
> I am less convinced by the need of this class; the only thing it adds is the
> explicit Deref bit, and I'm not completely sure that's useful in general. 
> I'll think about it. 

That's not the only thing. Possibly the most important thing is that it serves as unambiguous documentation that a pointer can be null, which is something you don't get from a raw pointer.

After that, the explicit Deref is important because it helps catch missing null checks. When you convert a raw pointer to Ptr<>, everywhere you have a '->' or a '*' you have to change it to a Deref(), which naturally leads to you look for a null check nearby.

E.g. I found the two missing null checks in bug 1265273 this way. (Well, one turned out to be a true missing null check, and the other wasn't necessary because prior code ensured -- in a non-obvious way -- that a particular pointer was non-null.)

In contrast, converting a raw pointer to a reference or NNPtr helps find unnecessary null checks, which are less important than missing null checks.

It might be helpful to think about the Rust |Option| type. In Rust you'd write:

  // x: Option<T>
  if x.is_some() {
    f(x.unwrap());
  }

With Ptr<> you'd write:

  // Ptr<T> p
  if (x) {
    f(x.Deref());
  }

Rust obviously also has pattern-matching forms that are nice and which C++ lacks.


> Perhaps if the name were more evocative, like MaybePtr<T>?

I used that name originally but discarded it because it's too long. Also, RefPtr and nsCOMPtr and UniquePtr all currently exist and can be null or non-null so it fits in with that pattern. (Maybe "PlainPtr" would fit in with that pattern better than "Ptr" but that seems silly.)

Comment 6

[Nicholas Nethercote [inactive]]

Nathan said he'd take another look.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Use Ptr<> in nsContentUtils::GetSurfaceData()

froydnj: bug 1268721 is a straightforward null-deref crash. So I made it clear
that GetSurfaceData()'s first argument can be null, by converting it to Ptr<>
and then I just let the compiler errors tell me what else needed to change as a
result.

Comment 8

[Andrew McCreight [:mccr8]]

I'm a big of a proponent for static checking as you'll find, but I don't see how Ptr<> helps. You have to assume the latter can be null. Ptr<> is opt-in, so somebody has to think about it, and I think the case we're really concerned about is things like GetSurfaceData() where nobody thought about whether something could be null or not.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8746865 [details] [diff] [review]
Use Ptr<> in nsContentUtils::GetSurfaceData()

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

A couple comments.

::: dom/base/nsContentUtils.cpp
@@ +7380,5 @@
>                image->GetFrame(imgIContainer::FRAME_CURRENT,
>                                imgIContainer::FLAG_SYNC_DECODE);
>              if (surface) {
>                RefPtr<mozilla::gfx::DataSourceSurface> dataSurface =
>                  surface->GetDataSurface();

We have an ergonomic problem here.  Ptr<T> should be useful as a return value; annotating parameters as Ptr<T> forces null checks inside those functions, but it would have been better if it was more obvious that GetDataSurface returned something that could be null.

But we couldn't have used it here, because GetDataSurface returns an already_AddRefed<DataSourceSurface>.  Adding Ptr<already_AddRefed<T>> seems a bit tedious, and you lose Ptr<T>-ness when assigning into RefPtr<T> anyway.

I guess the question here is whether we think just annotating method parameters is really sufficient.

@@ +7384,5 @@
>                  surface->GetDataSurface();
>                size_t length;
>                int32_t stride;
>                mozilla::UniquePtr<char[]> surfaceData =
> +                nsContentUtils::GetSurfaceData(dataSurface.get(), &length, &stride);

People are never going to use Ptr<T> if it requires scattering .get() calls all over the place.  We're going need to add (at a minimum):

MOZ_IMPLICIT Ptr(const UniquePtr<T>&)
MOZ_IMPLICIT Ptr(const RefPtr<T>&)
MOZ_IMPLICIT Ptr(const nsCOMPtr<T>&)

@@ +7498,3 @@
>      return nullptr;
>    }
> +  mozilla::gfx::IntSize size = aSurface.Deref().GetSize();

I assume in a landable version of the patch, we'd only have a single call to Deref somewhere?

I wonder if we should have Deref return NNPtr<T>.  Embarrassingly, I had to think about whether .GetSize() is going to call the correct function through virtual dispatch.  It's obviously going to, but I'm not sure that it's immediately obvious/familiar that it would, like it would be with ->GetSize().

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> I'm a big of a proponent for static checking as you'll find, but I don't see
> how Ptr<> helps. You have to assume the latter can be null. Ptr<> is opt-in,
> so somebody has to think about it, and I think the case we're really
> concerned about is things like GetSurfaceData() where nobody thought about
> whether something could be null or not.

Right, so njn outlined a bit of how it helps in comment 5.  I think the documentation aspect of it is a good argument--compare how much better code is when it's passing around UniquePtr<T> vs. T*.  As somebody who freely modifies code all over Gecko, documentation in the types is quite helpful.

But you're right in this sense: it is opt-in, so it's very dependent on getting buy-in from people in its use and/or reviewers who think it makes the code better.

A bit of thinking out loud: it'd be an experiment.  Maybe njn/mccr8/e10s crash folk start using it to rewrite code in the face of crashes, rather than adding |if (!aPtr) return;| all over.  As a bonus, maybe it identifies other places we could have crashed, and double bonus, it identifies places for which we already have crash-stats data.  njn posts to dev-platform about it, we have a discussion there, maybe blogs about it and gets non-Mozilla folk to weight in.  Maybe over the course of the next quarter or two, we see how it works...how expensive is it to undo the experiment and go back to raw pointers if we think it's not actually helping, or if people push back mightily against it?

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> Ptr<> is opt-in,
> so somebody has to think about it, and I think the case we're really
> concerned about is things like GetSurfaceData() where nobody thought about
> whether something could be null or not.

I'm puzzled by this objection. C++ is full of dangerous low-level things like raw pointers. If you build a safer alternative you'll always have to be disciplined and opt in. UniquePtr<> is no different in this respect.

The fact that nobody thought about nullness in the GetSurfaceData() is exactly the point. If I see a raw pointer I have no idea if it can be null or not. If I see a Ptr<> I immediately know it can be null and so I better null-check it before using it. (And the fact that Ptr<> deletes the '*' and '->' operators in favour of the explicit Deref() forces me to consider and update all the places where a null check might be necessary.)

Comment 12

[Nicholas Nethercote [inactive]]

> People are never going to use Ptr<T> if it requires scattering .get() calls
> all over the place.  We're going need to add (at a minimum):
> 
> MOZ_IMPLICIT Ptr(const UniquePtr<T>&)
> MOZ_IMPLICIT Ptr(const RefPtr<T>&)
> MOZ_IMPLICIT Ptr(const nsCOMPtr<T>&)

Which will make Ptr.h depends on UniquePtr.h, RefPtr.h and nsCOMPtr.h, right? That would be unfortunate :(


> I wonder if we should have Deref return NNPtr<T>.

It would require a different name if so.

Comment 13

[Nicholas Nethercote [inactive]]

> But you're right in this sense: it is opt-in, so it's very dependent on
> getting buy-in from people in its use and/or reviewers who think it makes
> the code better.
> 
> A bit of thinking out loud: it'd be an experiment.  Maybe njn/mccr8/e10s
> crash folk start using it to rewrite code in the face of crashes, rather
> than adding |if (!aPtr) return;| all over.  As a bonus, maybe it identifies
> other places we could have crashed, and double bonus, it identifies places
> for which we already have crash-stats data.  njn posts to dev-platform about
> it, we have a discussion there, maybe blogs about it and gets non-Mozilla
> folk to weight in.  Maybe over the course of the next quarter or two, we see
> how it works...how expensive is it to undo the experiment and go back to raw
> pointers if we think it's not actually helping, or if people push back
> mightily against it?

This seems like an extraordinarily high bar to set -- "gets non-Mozilla folk to weigh in", srsly? -- and one that's much higher than what is normally applied for new things in MFBT.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> > But you're right in this sense: it is opt-in, so it's very dependent on
> > getting buy-in from people in its use and/or reviewers who think it makes
> > the code better.
> > 
> > A bit of thinking out loud: it'd be an experiment.  Maybe njn/mccr8/e10s
> > crash folk start using it to rewrite code in the face of crashes, rather
> > than adding |if (!aPtr) return;| all over.  As a bonus, maybe it identifies
> > other places we could have crashed, and double bonus, it identifies places
> > for which we already have crash-stats data.  njn posts to dev-platform about
> > it, we have a discussion there, maybe blogs about it and gets non-Mozilla
> > folk to weight in.  Maybe over the course of the next quarter or two, we see
> > how it works...how expensive is it to undo the experiment and go back to raw
> > pointers if we think it's not actually helping, or if people push back
> > mightily against it?
> 
> This seems like an extraordinarily high bar to set -- "gets non-Mozilla folk
> to weigh in", srsly? -- and one that's much higher than what is normally
> applied for new things in MFBT.

Apologies, I could have been clearer here.  I don't mean that the non-Mozilla opinion should necessarily have any effect on what we do in our code, nor that any negative feedback would imply that we would immediately remove it from our codebase.  I meant something more along the lines of "we could post about it and perhaps people would have useful feedback".  It would not be any kind of prerequisite for landing the code.

(In reply to Nicholas Nethercote [:njn] from comment #12)
> > People are never going to use Ptr<T> if it requires scattering .get() calls
> > all over the place.  We're going need to add (at a minimum):
> > 
> > MOZ_IMPLICIT Ptr(const UniquePtr<T>&)
> > MOZ_IMPLICIT Ptr(const RefPtr<T>&)
> > MOZ_IMPLICIT Ptr(const nsCOMPtr<T>&)
> 
> Which will make Ptr.h depends on UniquePtr.h, RefPtr.h and nsCOMPtr.h,
> right? That would be unfortunate :(

Not necessarily; the types can be forward-declared and the appropriate constructors defined in the corresponding headers.

> > I wonder if we should have Deref return NNPtr<T>.
> 
> It would require a different name if so.

Indeed.  Unfortunately, lots of good names are longer than "deref". =/

Comment 15

[Nicholas Nethercote [inactive]]

> We have an ergonomic problem here.  Ptr<T> should be useful as a return
> value; annotating parameters as Ptr<T> forces null checks inside those
> functions, but it would have been better if it was more obvious that
> GetDataSurface returned something that could be null.
> 
> But we couldn't have used it here, because GetDataSurface returns an
> already_AddRefed<DataSourceSurface>.  Adding Ptr<already_AddRefed<T>> seems
> a bit tedious, and you lose Ptr<T>-ness when assigning into RefPtr<T> anyway.
> 
> I guess the question here is whether we think just annotating method
> parameters is really sufficient.

I'd like the ideas in this bug (i.e. make it clear when handles can/cannot be null) to be as broadly applicable as possible.

My thinking is that, ideally, all our current smart pointers as well as Ptr<> would come to mean "might be null". (For the existing smart pointers that is already the case.) And then we'd implement not_null<> and use that as a wrapper for all of these types when we want to restrict them to "non-null".

(For full symmetry with Ptr<>, all the existing types like RefPtr<> and UniquePtr<> and already_AddRefed<> would need to disable operators '->' and '*' and add Deref(), which I realize is a big ask.)

Comment 16

[Nicholas Nethercote [inactive]]

> My thinking is that, ideally, all our current smart pointers as well as
> Ptr<> would come to mean "might be null". (For the existing smart pointers
> that is already the case.) And then we'd implement not_null<> and use that
> as a wrapper for all of these types when we want to restrict them to
> "non-null".

Another possibility is to get rid of Ptr<> and just have MaybeNull<>, which would be symmetrical with NotNull<>.

In case it's not clear, the difference is between:

  Ptr<T>

where the '*' is implicit, and

  MaybeNull<T*>

The latter has more flexibility, e.g. you can also do:

  MaybeNull<RefPtr<T>>
  MaybeNull<UniquePtr<T>>

Comment 17

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7901f83e0b5ef0f145d48ef3c9265355c117b525
Bug 1266651 (part 1) - Rename nsThreadShutdownContext members. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/72813a2ef76fb66cb9e1dd0ed402257773352f93
Bug 1266651 (part 2) - Give nsThreadShutdownContext a proper constructor. r=froydnj.

Comment 18

[Nicholas Nethercote [inactive]]

I've landed the first two patches, which were just minor clean-ups in nsThread.cpp. I've spun off bug 1272203 for implementing NotNull. I'll hold off for now on MaybeNull, because it's probably more complex and controversial than NotNull.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7901f83e0b5e
https://hg.mozilla.org/mozilla-central/rev/72813a2ef76f