1257296 - WeakPtr<> should not silently behave badly when you null check it

Status: RESOLVED INVALID

(Core :: MFBT, defect)

Description

[Andrew McCreight [:mccr8]]

Bug 1256061 seems to have happened because I was able to do "if (foo)" where foo has type WeakReference<> without the compiler complaining, when what I really meant was foo.get(). I don't know what the right fix is, but I feel like this should either not compile or there should be an overload that behaves as you would expect. I'm not even sure how my code compiled.

RefCounted<> is the parent class, and I think it has the same issue. RefPtr<> seems to have various operator * and operator bool things to deal with this.

Comment 1

[Andrew McCreight [:mccr8]]

Nathan, what do you think is the right way to fix this?

Comment 2

[Andrew McCreight [:mccr8]]

Daniel pointed out that the bug in question actually involves WeakPtr, not WeakReference. So that makes a little more sense that it compiles. Is it using operator*?

Comment 3

[Andrew McCreight [:mccr8]]

And dholbert points out that if it is just using operator*, then my patch in bug 1256061 did not change what code executes.

Comment 4

[Nathan Froyd [:froydnj]]

I don't think this is INVALID; it seems like it's reasonable to try removing operator() from WeakPtr to force people to think a little harder about this...though then they might write:

if (x.get()) {
  ...do something with x.get()...
}

and if the problem is that x.get() goes away between the test and the use...

Comment 5

[Daniel Holbert [:dholbert]]

It's INVALID from the perspective that WeakPtr is consistent with our other smart-pointer types here (in that you can check it as part of a boolean condition, and that's effectively a null-check -- with the extra bonus meaning that this *also* checks whether the thing is still alive, for WeakPtr).

(When mccr8 filed this, he had WeakPtr & WeakReference mixed up & thought something different & bizarre was going on.)

Perhaps you're saying that this automatic checking is undesirable, though? I'm not immediately sure why... (seems nice from a consistency perspective)

(I agree that "x.get() goes away between the test and the use" is something to be worried about, but I don't see how we can help with that here other than by encouraging people to think carefully about operations that might destroy the WeakPtr-controlled object, and to re-check its validity when necessary.)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Perhaps you're saying that this automatic checking is undesirable, though?
> I'm not immediately sure why... (seems nice from a consistency perspective)

Sorry, s/checking/bool conversion/

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Perhaps you're saying that this automatic checking is undesirable, though?
> I'm not immediately sure why... (seems nice from a consistency perspective)
> 
> (I agree that "x.get() goes away between the test and the use" is something
> to be worried about, but I don't see how we can help with that here other
> than by encouraging people to think carefully about operations that might
> destroy the WeakPtr-controlled object, and to re-check its validity when
> necessary.)

It is consistent with our other smart pointer types, but I'm not sure that's a plus in this context.  It seems valuable to provide a very minimal interface to WeakPtr so that people will think a little harder about how to use it; compare std::weak_ptr to WeakPtr, for instance.  See bug 1258263, which is in the back of my mind when thinking about this as well.