923554 - AtomicSupportsWeakPtr is not currently usable in an atomic way

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

If I have a WeakPtr to some AtomicSupportsWeakPtr object, how do I use it atomically?

WeakPtr allows dereferencing as a plain pointer, but that is dangerous in multithreaded settings. Indeed, the object pointed to by the weak pointer could disappear between the time the T* is obtained from the WeakPtr, and the time the T* is subsequently dereferenced.

The way Android pointers (sp and wp) solve this problem is by not providing any dereferencing operator or method (no operator->, operator* etc) aside from a scary unsafe_get() method, and instead having people construct strong pointers from weak pointers.

So the usage pattern would be:

  WeakPtr<T> myWeakPointer;

  // some scope where we need to access the T pointed to by myWeakPointer 
  {
    RefPtr<T> myStrongPointer(myWeakPoint);
    // now we know that that object won't disappear before we leave that scope

    if (myStrongPointer) {
      myStrongPointer->doSomething();
    }
  }

Now, I can understand that existing users of the non-Atomic SupportsWeakPtr may prefer to keep the convenience of being able to use a WeakPtr naturally like any other pointer.

So do you think that Atomic and non-Atomic flavors of WeakPtr / SupportsWeakPtr need to be decoupled to have each the API that makes sense for them (convenience for the non-atomic variant, actual atomicness for the atomic variant)?

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Waldo, your opinion please!

Comment 2

[(no longer active)]

I discussed this with Benoit.  There are more problems here.

1. <http://mxr.mozilla.org/mozilla-central/source/mfbt/WeakPtr.h?rev=552bca1bc885#104> is not thread safe, so if you race on a asWeakPtr() call on two threads, you may end up with two WeakReferences.

2. WeakPtrBase::operator*/-> are not safe even in the non-atomic case, since they unconditionally dereference the pointers.  This has been broken since Jeff first implemented this class.  I filed bug 924658 for that.

As for this bug, we should back out the "thread-safe" implementation of AtomicSupportsWeakPtr that bug 864035 added.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Comment 4

[Sotaro Ikeda [:sotaro]]

Current AtomicSupportsWeakPtr is useless and should not be used. But if we want to use thread safe strong poitner, thread safe weak pointer becomes also necessary. We can live with without the thread safe weak pointer. But implementation becomes trickier and error prone when to implement circular references.

Comment 5

[(no longer active)]

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> Current AtomicSupportsWeakPtr is useless and should not be used. But if we
> want to use thread safe strong poitner, thread safe weak pointer becomes
> also necessary. We can live with without the thread safe weak pointer. But
> implementation becomes trickier and error prone when to implement circular
> references.

Yes, the current implementation is not actually thread-safe, and that is worse than not having a thread-safe implementation.  Adding a real thread-safe implementation should be discussed in another bug.

Comment 6

[(no longer active)]

Comment on attachment 814607 [details] [diff] [review]
Patch (v1)

Asking Nathan for MFBT review help.  We shouldn't let this code sit in our tree for too long, someone may start using it. :(

Comment 7

[(no longer active)]

Comment on attachment 814607 [details] [diff] [review]
Patch (v1)

https://hg.mozilla.org/integration/mozilla-inbound/rev/287878a32dd2

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/287878a32dd2

Comment 9

[Jeff Walden [:Waldo]]

Hum.  I remember seeing the "operator*/-> are not safe even in the non-atomic case" thing originally, then somehow rationalizing to myself that it was sensible.  Something about the reference-counted ownership here being sufficient to guarantee liveness...somehow?  Bleh.  If the atomic stuff doesn't work, clearly it's gotta go, tho.

Comment 10

[(no longer active)]

(In reply to comment #9)
> Hum.  I remember seeing the "operator*/-> are not safe even in the non-atomic
> case" thing originally, then somehow rationalizing to myself that it was
> sensible.  Something about the reference-counted ownership here being
> sufficient to guarantee liveness...somehow?  Bleh.  If the atomic stuff doesn't
> work, clearly it's gotta go, tho.

I'd be curious if you can convince me of that too.  :-)