Add thread safe ref-counting for mfbt::WeakPtr

ASSIGNED
Assigned to

Status

()

Core
MFBT
ASSIGNED
2 years ago
3 months ago

People

(Reporter: jerry, Assigned: jerry)

Tracking

(Depends on: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

2 years ago
+++ 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.
    ....
  }
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Flags: needinfo?(nfroyd)
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(nfroyd)
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 2

2 years ago
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/
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 4

2 years ago
(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.
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
You need to log in before you can comment on or make changes to this bug.