Open Bug 1251593 Opened 8 years ago Updated 2 years ago

Add the thread checking for AddRef() and Release() in mfbt RefCounted

Categories

(Core :: MFBT, defect)

defect

Tracking

()

People

(Reporter: jerry, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We have thread checking for addRef and Release in NS_INLINE_DECL_REFCOUNTING macro[1], but we don't have the similar checking in mfbt RefCounted.
There will be a potential race condition of ref-counting problem if we change the counting in different thread.
I would like to have the same checking as the NS_INLINE_DECL_REFCOUNTING does if we enable MOZILLA_INTERNAL_API.

[1]
https://hg.mozilla.org/mozilla-central/annotate/918df3a0bc1c4d07299e4f66274a7da923534577/xpcom/glue/nsISupportsImpl.h#l507
Comment on attachment 8724062 [details] [diff] [review]
the thread checking of AddRef() and Release() in RefCounted class

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

::: mfbt/RefCounted.h
@@ +29,5 @@
>  
> +#if defined(MOZILLA_INTERNAL_API) && \
> +    !defined(MOZILLA_XPCOMRT_API) && \
> +    defined(DEBUG)
> +//#define MOZ_REFCOUNTED_THREAD_CHECKING

I already catch some assert in current code base, so turn off the thread checking by default before handling the assert.
Attachment #8724062 - Flags: review?(nfroyd)
Attachment #8724062 - Flags: review?(ehsan)
Attachment #8724062 - Flags: review?(dbaron)
There's some previous work along these lines in bug 991710, though that seems to have stalled out in 2014.
Comment on attachment 8724062 [details] [diff] [review]
the thread checking of AddRef() and Release() in RefCounted class

I suspect MFBT owners aren't going to want the XPCOM dependency here.  It's also not clear to me that the #ifdefs will match people's expectations of what checking they get.

So clearing the review request since I'm not really an appropriate reviewer.

However, you should continue (at least until bug 991710 is fixed, and perhaps other issues as well) to follow the guideline that RefCounted<T> should not be used in Gecko code, as described in khuey's dev-platform post dated 2014-04-15:
https://groups.google.com/d/msg/mozilla.dev.platform/Z8muIf0Ax6o/EOZcSUz2s7gJ
which references bug 991812.
Attachment #8724062 - Flags: review?(dbaron)
Comment on attachment 8724062 [details] [diff] [review]
the thread checking of AddRef() and Release() in RefCounted class

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

I agree with dbaron.  RefCounted<T> should not be used anywhere that it isn't being used already.  Last I checked it was only used in gfx/2d and mozglue, and both of them should be considered as mistakes.
Attachment #8724062 - Flags: review?(ehsan) → review-
(In reply to :Ehsan Akhgari from comment #5)
> I agree with dbaron.  RefCounted<T> should not be used anywhere that it
> isn't being used already.  Last I checked it was only used in gfx/2d and
> mozglue, and both of them should be considered as mistakes.

I agree with the "should not be used anywhere that it isn't being used already" statement, but I don't understand why this implies "shouldn't add useful debug checks to it".  We added leak checking; surely thread safety assertions (especially considering they find problems!) would be similarly useful?
(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to :Ehsan Akhgari from comment #5)
> > I agree with dbaron.  RefCounted<T> should not be used anywhere that it
> > isn't being used already.  Last I checked it was only used in gfx/2d and
> > mozglue, and both of them should be considered as mistakes.
> 
> I agree with the "should not be used anywhere that it isn't being used
> already" statement, but I don't understand why this implies "shouldn't add
> useful debug checks to it".  We added leak checking; surely thread safety
> assertions (especially considering they find problems!) would be similarly
> useful?

Because MFBT shouldn't depend on XPCOM?  That at least used to be true when I was working on adding support for leak checking which is why we ended up with this backwards implementation.  Have we changed our position with regards to MFBT dependencies?
(In reply to :Ehsan Akhgari from comment #7)
> Because MFBT shouldn't depend on XPCOM?  That at least used to be true when
> I was working on adding support for leak checking which is why we ended up
> with this backwards implementation.  Have we changed our position with
> regards to MFBT dependencies?

No, we haven't, but if the patch conditions things on MOZILLA_INTERNAL_API:

#if defined(MOZILLA_INTERNAL_API) && \
    !defined(MOZILLA_XPCOMRT_API) && \
    defined(DEBUG)
//#define MOZ_REFCOUNTED_THREAD_CHECKING

just like our leak checking support, then I don't see what the problem is?
Flags: needinfo?(ehsan)
(In reply to Nathan Froyd [:froydnj] from comment #8)
> (In reply to :Ehsan Akhgari from comment #7)
> > Because MFBT shouldn't depend on XPCOM?  That at least used to be true when
> > I was working on adding support for leak checking which is why we ended up
> > with this backwards implementation.  Have we changed our position with
> > regards to MFBT dependencies?
> 
> No, we haven't, but if the patch conditions things on MOZILLA_INTERNAL_API:
> 
> #if defined(MOZILLA_INTERNAL_API) && \
>     !defined(MOZILLA_XPCOMRT_API) && \
>     defined(DEBUG)
> //#define MOZ_REFCOUNTED_THREAD_CHECKING
> 
> just like our leak checking support, then I don't see what the problem is?

Sure.  As an MFBT peer, it's really up to you after all!
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #9)
> Sure.  As an MFBT peer, it's really up to you after all!

Just making sure I understand the opposition point of view!
Comment on attachment 8724062 [details] [diff] [review]
the thread checking of AddRef() and Release() in RefCounted class

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

r=me with the below change.  Thanks for looking into this!

I think the patch in bug 991710 is more general, and thus to be preferred in normal cases, but given that we only use RefCounted<T> inside Gecko itself and have no desires or plans to expand its use outside of Gecko, I don't think we need a more general solution at this time.

::: mfbt/RefCounted.h
@@ +29,5 @@
>  
> +#if defined(MOZILLA_INTERNAL_API) && \
> +    !defined(MOZILLA_XPCOMRT_API) && \
> +    defined(DEBUG)
> +//#define MOZ_REFCOUNTED_THREAD_CHECKING

Please file bugs on all the places where this assert triggers, ensure those bugs are fixed, and then land this patch with this line commented out.

@@ +170,5 @@
>                                 Atomic<MozRefCountType>,
>                                 MozRefCountType>::Type mRefCnt;
> +
> +#ifdef MOZ_REFCOUNTED_THREAD_CHECKING
> +  void RefCountThreadCheck(IntegralConstant<RefCountAtomicity, AtomicRefCount>) const

I will have to remember this overload trick for future use.
Attachment #8724062 - Flags: review?(nfroyd) → review+
(In reply to :Ehsan Akhgari from comment #5)
> Comment on attachment 8724062 [details] [diff] [review]
> the thread checking of AddRef() and Release() in RefCounted class
> 
> Review of attachment 8724062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I agree with dbaron.  RefCounted<T> should not be used anywhere that it
> isn't being used already.  Last I checked it was only used in gfx/2d and
> mozglue, and both of them should be considered as mistakes.

Gfx/2d(moz2d) should use RefCounted<T> instead of the NS_INLINE_DECL_REFCOUNTING/NS_INLINE_DECL_THREADSAFE_REFCOUNTING in gecko. It could not have the dependency with gecko(except the mfbt) since it should be built standalone for another project.
Status: NEW → ASSIGNED
Depends on: 1251559
Remove the |MOZILLA_XPCOMRT_API| as bug 1251769.
Turn on |MOZ_REFCOUNTED_THREAD_CHECKING|.
Attachment #8731737 - Flags: review+
Attachment #8724062 - Attachment is obsolete: true
Please land the attachment 8731737 [details] [diff] [review] to m-c after bug 1251559 merged.
Keywords: checkin-needed
I will re-push to try for bug 1251593 and this bug 1251559 later.
Blocks: 1258263

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.

Assignee: bignose1007+bugzilla → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(simon.giesecke)

Probably still useful, and I don't see anything that looks to have done this in the interim.

Flags: needinfo?(simon.giesecke)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.