1037691 - Remove public destructors of classes that inherit refcount

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Benoit Girard (:BenWa)]

+++ 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.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: part 1: Add assert

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Part 2: Fix gfx

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Part 3: Fix gfx bug in widget

Comment 4

[Benoit Girard (:BenWa)]

Attachment: part 4: Fix content

Comment 5

[Benoit Girard (:BenWa)]

Attachment: part 5: Fix DOM

Comment 6

[Benoit Girard (:BenWa)]

Attachment: part 6: Fix ipc

Comment 7

[Benoit Girard (:BenWa)]

Attachment: part 7: netwerk

Comment 8

[Benoit Girard (:BenWa)]

Attachment: part 8: Fix parser

Comment 9

[Benoit Girard (:BenWa)]

Attachment: part 9: fix rdf

Comment 10

[Benoit Girard (:BenWa)]

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

Comment 11

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=a0cd2eb49d71

Comment 12

[(no longer active)]

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 13

[Honza Bambas (:mayhemer)]

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 14

[Benjamin Smedberg]

Attachment: Remove public destructor from rdf Assertion and deal with the fact that various places in code were not refcounting it correctly, r=benwa

Comment 15

[Benjamin Smedberg]

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.

Comment 16

[Benoit Girard (:BenWa)]

(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.

Comment 17

[(no longer active)]

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 18

[Daniel Holbert [:dholbert]]

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 19

[Benoit Girard (:BenWa)]

Attachment: Part 10: Rest

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 8454757 [details] [diff] [review]
part 5: Fix DOM

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

Trivial, removing review request.

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 8454758 [details] [diff] [review]
part 6: Fix ipc

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

Trivial, removing review request.

Comment 22

[Daniel Holbert [:dholbert]]

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?

Comment 23

[Benoit Girard (:BenWa)]

(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 24

[Benoit Girard (:BenWa)]

Comment on attachment 8454761 [details] [diff] [review]
part 8: Fix parser

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

canceling trivial review.

Comment 25

[Benoit Girard (:BenWa)]

Attachment: All fixes

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 26

[(no longer active)]

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! ;)

Comment 27

[Benoit Girard (:BenWa)]

(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.

Comment 28

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=24fe8b08e6ba

Comment 29

[Benoit Girard (:BenWa)]

Attachment: All fixes r=ehsan,bent,mayhemer,jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/91a4d189f5d7

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91a4d189f5d7