Prepare WeakPtr to support a thread-safe variant

RESOLVED FIXED in mozilla23

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla23
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
In order for us to have a thread-safe WeakPtr variant, we need a thread-safe RefCounted, but unfortunately we can't have that without atomics support in MFBT.  But in the mean time, we can restructure the code to make it easy to support this in the future, and in Gecko we can work around it by a Gecko-specific thread-safe RefCounted.  The idea behind this patch is to make it possible to define a thread-safe WeakPtr like this:

template<class T>
class ThreadSafeWeakReferece : public ThreadSafeRefCounted<ThreadSafeWeakReference<T> >
{
  // mimic the interface of mozilla::detail::WeakReference
};

template<class T>
class SupportsThreadSafeWeakPtr : public SupportsWeakPtrBase<T, ThreadSafeWeakReferece<T> >
{
};

template<class T>
class ThreadSafeWeakPtr : public WeakPtrBase<T, ThreadSafeWeakReferece<T> >
{
  // add the ctors similar to WeakPtr
};

What this patch does is move the WeakReference class out of SupportsWeakPtr into namespace mozilla::detail, rename WeakPtr/SupportsWeakPtr to WeakPtrBase/SupportsWeakPtrBase and make them templated on the weak reference class, and provides new WeakPtr/SupportsWeakPtr derived from these new base classes with the non-thread-safe WeakReference class.
(Assignee)

Comment 1

5 years ago
Created attachment 739842 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #739842 - Flags: review?(jwalden+bmo)
Why does this block 834513?

Comment 3

5 years ago
Comment on attachment 739842 [details] [diff] [review]
Patch (v1)

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

This looks reasonable, except for one little bit.  Won't:

    WeakPtrBase<T, WeakReference> asWeakPtr()

mean that asWeakPtr() calls won't return WeakPtr, as they used to?  It seems to me SupportsWeakPtrBase needs to take another type parameter, WeakPtr<T>, so that it can return the correct thing.  And WeakPtr needs

    explicit WeakPtr(const RefPtr<WeakReference>& o) : Base(o) {}

to make it all work.

r=me if that sounds correct to you, otherwise poke me and tell me what I'm seeing wrong here.

::: mfbt/WeakPtr.h
@@ +71,5 @@
> +namespace detail {
> +
> +// This can live beyond the lifetime of the class derived from SupportsWeakPtrBase.
> +template<class T>
> +class WeakReference : public RefCounted<WeakReference<T > >

Don't need the extra space in "<T >".

@@ +88,5 @@
> +    }
> +    T* ptr;
> +};
> +
> +}

// namespace detail
Attachment #739842 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 4

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Why does this block 834513?

Because I need to use a thread safe weak pointer there.  New patch coming up shortly.
(Assignee)

Comment 5

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Comment on attachment 739842 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 739842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks reasonable, except for one little bit.  Won't:
> 
>     WeakPtrBase<T, WeakReference> asWeakPtr()
> 
> mean that asWeakPtr() calls won't return WeakPtr, as they used to?  It seems
> to me SupportsWeakPtrBase needs to take another type parameter, WeakPtr<T>,
> so that it can return the correct thing.  And WeakPtr needs
> 
>     explicit WeakPtr(const RefPtr<WeakReference>& o) : Base(o) {}
> 
> to make it all work.

I made WeakPtr constructible from WeakPtrBase in order to make this work.
(Assignee)

Updated

5 years ago
No longer blocks: 834513
(Assignee)

Updated

5 years ago
Blocks: 864035
https://hg.mozilla.org/mozilla-central/rev/c23e30df38d1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.