Closed Bug 1257296 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED INVALID
Tracking Status
firefox48 --- affected

People

(Reporter: mccr8, Unassigned)

References

Details

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.
Nathan, what do you think is the right way to fix this?
Flags: needinfo?(nfroyd)
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*?
Summary: RefCounted<> should not silently behave badly when you null check it → WeakPtr<> should not silently behave badly when you null check it
And dholbert points out that if it is just using operator*, then my patch in bug 1256061 did not change what code executes.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(nfroyd)
Resolution: --- → INVALID
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...
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.)
(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/
(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.
You need to log in before you can comment on or make changes to this bug.