1258263 - Add thread safe ref-counting for mfbt::WeakPtr

Status: NEW

(Core :: MFBT, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

+++ This bug was initially created as a clone of Bug #1251593 +++

With bug 1251593, we have thread checking assert for mfbt::RefCounted. The mfbt::RefCounted's ctor, AddRef() and Release() should be at the same thread.

Here is the usage that will hit the assert:

  class RasterImage
  {
    ....
    NS_DECL_THREADSAFE_ISUPPORTS    // use thread safe ref-counting for RasterImage

    ....
    WeakPtr<layers::ImageContainer> mImageContainer; // WeakPtr has a non-thread safe ref-counting member
  }

When we create RasterImage at main thread and call RasterImage::~dtor at another thread, we hit the assert checking inside |mImageContainer|.

My first thought is just extend the WeakPtr to use thread safe counting inside, but there is still a race condition between WeakPtr and SupportsWeakPtr.

  class C : public SupportsWeakPtr<C>
  {
    ....
  }

  C* ptr = new C();

  WeakPtr<C> weak = ptr;

  if (weak) {
    // Do something with weak.
    // We still can't make sure the |ptr| is valid even even we use thread safe counting inside WeakPtr.
    ....
  }

Comment 1

[Nathan Froyd [:froydnj]]

I'm pretty sure we decided to not support thread-safe refcounting in WeakPtr because of the exact race condition you mention.  The RasterImage code should be fixed, instead.

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Currently, I have no idea how to fix the |mImageContainer| usage inside RasterImage.

How about have the same usage as:
https://dxr.mozilla.org/mozilla-central/source/security/sandbox/chromium/base/memory/weak_ptr.h#50

The weakPtr could be passed at any thread(use thread safe ref-counting inside), but we should check the weakPtr dereferenced and the weakPtr referent thread(and we need some xpcom thread checking code).

Another idea to solve the race condition between WeakPtr and SupportsWeakPtr is that:
Use the same interface as c++ weak_ptr does. When we call lock, we can make sure the object is available or not for future usage.
http://www.cplusplus.com/reference/memory/weak_ptr/lock/

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #2)
> Currently, I have no idea how to fix the |mImageContainer| usage inside
> RasterImage.

Ugh, this is terrible.

> How about have the same usage as:
> https://dxr.mozilla.org/mozilla-central/source/security/sandbox/chromium/
> base/memory/weak_ptr.h#50
> 
> The weakPtr could be passed at any thread(use thread safe ref-counting
> inside), but we should check the weakPtr dereferenced and the weakPtr
> referent thread(and we need some xpcom thread checking code).

Assuming I understand the Chromium restrictions correctly, I believe this would be more restrictive than what we typically allow in Gecko (though arguably safer, as the current bug shows).  It wouldn't surprise me if making that change required a significant rewrite of several SupportsWeakPtr clients.

I think a simpler way to do this is to require SupportsWeakPtr to take an atomicity template argument that can be passed through into WeakReference, and from there into RefCounted.  Or have a SupportsWeakPtrFromMultipleThreads class or similar.

> Another idea to solve the race condition between WeakPtr and SupportsWeakPtr
> is that:
> Use the same interface as c++ weak_ptr does. When we call lock, we can make
> sure the object is available or not for future usage.
> http://www.cplusplus.com/reference/memory/weak_ptr/lock/

We could do that too; I think that starts to get a bit complicated because there's no type immediately at hand to return from the lock() method, and writing one strikes me as complicated at first glance.

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Jerry Shih[:jerry] (UTC+8) from comment #2)
> > How about have the same usage as:
> > https://dxr.mozilla.org/mozilla-central/source/security/sandbox/chromium/
> > base/memory/weak_ptr.h#50
> > 
> > The weakPtr could be passed at any thread(use thread safe ref-counting
> > inside), but we should check the weakPtr dereferenced and the weakPtr
> > referent thread(and we need some xpcom thread checking code).
> 
> Assuming I understand the Chromium restrictions correctly, I believe this
> would be more restrictive than what we typically allow in Gecko (though
> arguably safer, as the current bug shows).  It wouldn't surprise me if
> making that change required a significant rewrite of several SupportsWeakPtr
> clients.
> 
> I think a simpler way to do this is to require SupportsWeakPtr to take an
> atomicity template argument that can be passed through into WeakReference,
> and from there into RefCounted.  Or have a
> SupportsWeakPtrFromMultipleThreads class or similar.
> 

We already have that before.
https://bugzilla.mozilla.org/show_bug.cgi?id=923554
The reason we remove that is that it still has race condition problem as comment 0.

I will try to think about the c++11 lock() method.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > I think a simpler way to do this is to require SupportsWeakPtr to take an
> > atomicity template argument that can be passed through into WeakReference,
> > and from there into RefCounted.  Or have a
> > SupportsWeakPtrFromMultipleThreads class or similar.
> 
> We already have that before.
> https://bugzilla.mozilla.org/show_bug.cgi?id=923554
> The reason we remove that is that it still has race condition problem as
> comment 0.

So we did, thank you for looking!  I guess we need lock() or similar, then.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sg, could you have a look please?
For more information, please visit auto_nag documentation.