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)
Core
MFBT
Tracking
()
NEW
People
(Reporter: jerry, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
3.23 KB,
patch
|
jerry
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
Reporter | ||
Comment 2•8 years ago
|
||
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.
Reporter | ||
Updated•8 years ago
|
Attachment #8724062 -
Flags: review?(nfroyd)
Attachment #8724062 -
Flags: review?(ehsan)
Attachment #8724062 -
Flags: review?(dbaron)
Comment 3•8 years ago
|
||
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 5•8 years ago
|
||
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-
Comment 6•8 years ago
|
||
(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?
Comment 7•8 years ago
|
||
(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?
Comment 8•8 years ago
|
||
(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)
Comment 9•8 years ago
|
||
(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)
Comment 10•8 years ago
|
||
(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 11•8 years ago
|
||
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+
Reporter | ||
Comment 12•8 years ago
|
||
(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.
Reporter | ||
Comment 13•8 years ago
|
||
Remove the |MOZILLA_XPCOMRT_API| as bug 1251769. Turn on |MOZ_REFCOUNTED_THREAD_CHECKING|.
Attachment #8731737 -
Flags: review+
Reporter | ||
Updated•8 years ago
|
Attachment #8724062 -
Attachment is obsolete: true
Reporter | ||
Comment 14•8 years ago
|
||
Please land the attachment 8731737 [details] [diff] [review] to m-c after bug 1251559 merged.
Keywords: checkin-needed
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/516d8328dcb6
Keywords: checkin-needed
Comment 16•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/520292912175
Reporter | ||
Comment 17•8 years ago
|
||
I will re-push to try for bug 1251593 and this bug 1251559 later.
Comment 18•2 years ago
|
||
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)
Comment 19•2 years ago
|
||
Probably still useful, and I don't see anything that looks to have done this in the interim.
Flags: needinfo?(simon.giesecke)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•