821804 - Default constructor for WeakPtr results in an object that always crashes

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Joe Drew (not getting mail)]

Attachment: handle null

The default constructor for WeakPtr leads to an object that crashes whenever it's used. This patch tests for this case and just returns null and/or asserts.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 692352 [details] [diff] [review]
handle null

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

I don't like this very much.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Another approach

This avoids the extra tests but requires a heap allocation for implicit construction.

This might be simpler but I'm not sure it's better.

Comment 3

[(no longer active)]

Hrm, is the default ctor actually useful?  It's unfortunate to have to do a heap allocation here.

Comment 4

[Jeff Walden [:Waldo]]

WeakPtr by nature requires a heap allocation of a member, because you need something to manage the lifetime of the object to be weakly held alive -- which can go past the lifetime of any individual WeakPtr.

A slight alternative is to do it like nsSupportsWeakReference (the SupportsWeakPtr analog in the XPCOM world) does.  There, an nsWeakReference is just a raw pointer that nsSupportsWeakReference will clear on destruction if events proceed in that order.  (And of course if nsWeakReference gets destroyed first, it'll tell nsSupportsWeakReference this so that it doesn't inform the existing nsWeakReference.)  The issue with this is that it uses a raw pointer -- which means it needs to rely on something to do lifetime management, namely XPCOM refcounting.  (WeakPtr foists the management on the SupportsWeakPtr<T>::WeakReference class, which *is* refcounted, unlike SupportsWeakPtr.)

Anyway.  Perhaps this should all go in a comment in WeakPtr.h, cleaned up and made coherent, or at least as coherent as this all can be given the number of referents in the air.

Comment 5

[Jeff Walden [:Waldo]]

<Waldo> joe: was the use case for the default WeakPtr constructor so that you could have member variables, that would be initialized not in the constructor, or something?
<joe> Waldo: yeah, i believe so
<Waldo> modulo type-unsafety and no operator support, this is something nsSupportsWeakReference/nsWeakReference did almost better, if I've paged them into memory far enough

WeakPtr's current setup still leaves me feeling a bit sickly, but Jeff's patch does somewhat less violence to the coherency of the existing setup, so let's go with that.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 692375 [details] [diff] [review]
Another approach

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

::: mfbt/WeakPtr.h
@@ +112,5 @@
>  class WeakPtr
>  {
>    public:
>      WeakPtr(const WeakPtr<T>& o) : ref(o.ref) {}
> +    // ensure that ref is always dereferencable

Capitalize this, tho, and "dereferenceable".  :-)

Comment 7

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c15559168956

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c15559168956

Comment 9

[Justin Lebar (not reading bugmail)]

I need this for patches I'm writing in bug 844323.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/aa9fa3143f90
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a9f7cef6d805