Default constructor for WeakPtr results in an object that always crashes

RESOLVED FIXED in Firefox 20

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: jrmuizel)

Tracking

unspecified
mozilla20
x86
Mac OS X
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 692352 [details] [diff] [review]
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.
Attachment #692352 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 1

5 years ago
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-
(Assignee)

Comment 2

5 years ago
Created attachment 692375 [details] [diff] [review]
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.
(Reporter)

Updated

5 years ago
Assignee: nobody → joe
(Reporter)

Updated

5 years ago
Attachment #692352 - Flags: review- → review?(jwalden+bmo)

Comment 3

5 years ago
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-
(Reporter)

Comment 7

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
I need this for patches I'm writing in bug 844323.
Blocks: 844323
blocking-b2g: --- → tef?

Updated

5 years ago
blocking-b2g: tef? → tef+

Updated

5 years ago
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
https://hg.mozilla.org/releases/mozilla-b2g18/rev/aa9fa3143f90
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a9f7cef6d805
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
status-firefox20: --- → fixed
You need to log in before you can comment on or make changes to this bug.