Closed Bug 821804 Opened 8 years ago Closed 8 years ago

Default constructor for WeakPtr results in an object that always crashes

Categories

(Core :: MFBT, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
blocking-b2g tef+
Tracking Status
firefox20 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: joe, Assigned: jrmuizel)

References

Details

Attachments

(2 files)

Attached 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-
Attached patch Another approachSplinter 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: 8 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.