Default constructor for WeakPtr results in an object that always crashes

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: joe, Assigned: jrmuizel)

Tracking

unspecified
mozilla20
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox20 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments)

Posted patch handle nullSplinter Review
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.
Attachment #692352 - Flags: review?(jwalden+bmo)
Comment on attachment 692352 [details] [diff] [review]
handle null

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

I don't like this very much.
Attachment #692352 - Flags: review?(jwalden+bmo) → review-
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.
Assignee: nobody → joe
Attachment #692352 - Flags: review- → review?(jwalden+bmo)
Hrm, is the default ctor actually useful?  It's unfortunate to have to do a heap allocation here.
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.
<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 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".  :-)
Attachment #692375 - Flags: review+
Attachment #692352 - Flags: review?(jwalden+bmo) → review-
https://hg.mozilla.org/integration/mozilla-inbound/rev/c15559168956
Assignee: joe → jmuizelaar
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/c15559168956
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I need this for patches I'm writing in bug 844323.
Blocks: 844323
blocking-b2g: --- → tef?
blocking-b2g: tef? → tef+
You need to log in before you can comment on or make changes to this bug.