+++ This bug was initially created as a clone of Bug #1027251 +++ Just following the path of bug 1027251 to also cover the case where the class inherit the refcount indirectly.

This is only partially completed. I'm going to try and get this landed to avoid excessive rot.

Comment on attachment 8454752 [details] [diff] [review] part 1: Add assert Review of attachment 8454752 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/nsISupportsImpl.h @@ +85,5 @@ > } > # define MOZ_IS_DESTRUCTIBLE(X) (mozilla::IsDestructibleFallback<X>::value) > #endif > > +#ifdef MOZ_CAN_USE_IS_DESTRUCTIBLE_FALLBACK Why hide this code behind this macro? @@ +108,5 @@ > + template<typename T> > + struct HasAddRef > + : HasAddRefImpl::Selector<T>::type > + { > + }; I wish we could implement std::is_member_function_pointer in mfbt/TypeTraits.h for this. That would require variadic template support though, and it may work in different ways on different compilers, so I don't want to make you do that here, but at least please file a follow-up. @@ +126,5 @@ > MOZ_IS_DESTRUCTIBLE(X), \ > "Class " #X " has no public destructor. That's good! So please " \ > "remove the HasDangerousPublicDestructor specialization for it."); > #else > +#define MOZ_ASSERT_NO_PUBLIC_DTOR_IF_REFCOUNTED(X) Why is this line needed? @@ +131,4 @@ > #define MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING(X) > #endif > > +#if defined(MOZ_HAS_ADDREF) && defined(MOZ_IS_DESTRUCTIBLE) I guess I don't understand the macro logic here at all. Which compilers would barf on HasAddRef above? I think we need to handle those specifically, although looking over the implementation of HasAddRef I can't immediately spot a feature being used which wouldn't work on all of our compilers...

Comment on attachment 8454759 [details] [diff] [review] part 7: netwerk Review of attachment 8454759 [details] [diff] [review]: ----------------------------------------------------------------- r+ with few nits, if this builds, go land it. ::: netwerk/cache2/CacheEntry.h @@ +363,5 @@ > class CacheOutputCloseListener : public nsRunnable > { > public: > void OnOutputClosed(); > +protected: make it private please, this class should probably be marked as MOZ_FINAL. (put to the private: section just bellow) ::: netwerk/cache2/CacheFileIOManager.cpp @@ +1100,5 @@ > { > MOZ_COUNT_DTOR(UpdateIndexEntryEvent); > } > > +public: not sure Run() methods in this file need to be public.

Comment on attachment 8454762 [details] [diff] [review] part 9: fix rdf I believe that this is an indication of a real bug, so I attached an alternate patch.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #15) > Comment on attachment 8454762 [details] [diff] [review] > part 9: fix rdf > > I believe that this is an indication of a real bug, so I attached an > alternate patch. I was planning on dealing with it in follow up. Since this is a large refactor I wanted to have a patch queue that had no side effects. But I'm ok with this tagging along.

blanket-r=me on all patches which make destructors private for this purpose unless they contain anything interesting that you want the owners/peers to review.

Comment on attachment 8454754 [details] [diff] [review] Part 3: Fix gfx bug in widget One nit: >+++ b/widget/xpwidgets/nsBaseWidget.cpp >@@ -900,17 +900,17 @@ void nsBaseWidget::CreateCompositor(int > return; > } > > // Initialize LayerScope on the main thread. > LayerScope::Init(); > > mCompositorParent = NewCompositorParent(aWidth, aHeight); > MessageChannel *parentChannel = mCompositorParent->GetIPCChannel(); >- ClientLayerManager* lm = new ClientLayerManager(this); >+ nsRefPtr<ClientLayerManager> lm = new ClientLayerManager(this); A bit lower down, we have, for the normal case: mLayerManager = lm; return; http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseWidget.cpp#940 ...where mLayerManager is a nsRefPtr. Now that 'lm' is also a nsRefPtr, we'll be doing an unnecessary AddRef/Release dance here. (We increment count to 2, for the assignment, and then we decrement it back to 1 when lm goes out of scope) You can do away with the dance & make this a bit faster by using: mLayerManager = lm.forget(); (Since 'forget()' returns already_AddRefed, which nsRefPtr's reassignment operator knows how to handle.)

Comment on attachment 8454757 [details] [diff] [review] part 5: Fix DOM Review of attachment 8454757 [details] [diff] [review]: ----------------------------------------------------------------- Trivial, removing review request.

Comment on attachment 8454758 [details] [diff] [review] part 6: Fix ipc Review of attachment 8454758 [details] [diff] [review]: ----------------------------------------------------------------- Trivial, removing review request.

Comment on attachment 8455728 [details] [diff] [review] Part 10: Rest >+++ b/layout/base/MaskLayerImageCache.cpp >+// This case is particially 'particularly' because it uses AddRef/Release to track the use >+// not to release the object. >+template<> >+struct mozilla::HasDangerousPublicDestructor<MaskLayerImageCache::MaskLayerImageKey> Nit: typo in the comment there -- I think maybe you meant to say particularly 'particular' or something like that?

(In reply to Daniel Holbert [:dholbert] from comment #18) Done but I don't really think this type of trivial optimization matters. (In reply to Daniel Holbert [:dholbert] from comment #22) > Comment on attachment 8455728 [details] [diff] [review] > Part 10: Rest > > >+++ b/layout/base/MaskLayerImageCache.cpp > >+// This case is particially 'particularly' because it uses AddRef/Release to track the use > >+// not to release the object. > >+template<> > >+struct mozilla::HasDangerousPublicDestructor<MaskLayerImageCache::MaskLayerImageKey> > > Nit: typo in the comment there -- I think maybe you meant to say > particularly 'particular' > or something like that? Thanks1 Fixed.

Comment on attachment 8454761 [details] [diff] [review] part 8: Fix parser Review of attachment 8454761 [details] [diff] [review]: ----------------------------------------------------------------- canceling trivial review.

I was going to follow bjacob' footsteps but I decided that asking for these reviews from different peers is a waste of everyone time. If someone has a valid objection to anything landed here please let me know. I will however wait for :bent to look into HasDangerousPublicDestructor<MiscContainer>.

Comment on attachment 8455728 [details] [diff] [review] Part 10: Rest Review of attachment 8455728 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/MaskLayerImageCache.cpp @@ +57,5 @@ > entry->mContainer = aContainer; > } > > +// This case is particially 'particularly' because it uses AddRef/Release to track the use > +// not to release the object. This comment makes no sense! ;)

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #26) > Comment on attachment 8455728 [details] [diff] [review] > Part 10: Rest > > Review of attachment 8455728 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/MaskLayerImageCache.cpp > @@ +57,5 @@ > > entry->mContainer = aContainer; > > } > > > > +// This case is particially 'particularly' because it uses AddRef/Release to track the use > > +// not to release the object. > > This comment makes no sense! ;) It was a copy paste error: // This case is particularly 'clever' because it uses AddRef/Release to track the use // not to release the object.