863884 - Prepare WeakPtr to support a thread-safe variant

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why does this block 834513?

Comment 3

[Jeff Walden [:Waldo]]

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

Comment 4

[(no longer active)]

(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.

Comment 5

[(no longer active)]

(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.

Comment 6

[(no longer active)]

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

Comment 7

[Phil Ringnalda (:philor)]

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