Closed Bug 1037691 Opened 5 years ago Closed 5 years ago

Remove public destructors of classes that inherit refcount

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(2 files, 11 obsolete files)

5.16 KB, patch
ehsan
: review-
Details | Diff | Splinter Review
72.03 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
+++ 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.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8454752 - Flags: review?(ehsan)
Attached patch Part 2: Fix gfx (obsolete) — Splinter Review
Attachment #8454753 - Flags: review?(jmuizelaar)
Attached patch Part 3: Fix gfx bug in widget (obsolete) — Splinter Review
Attachment #8454754 - Flags: review?(jmuizelaar)
Attached patch part 4: Fix content (obsolete) — Splinter Review
Attachment #8454756 - Flags: review?(bent.mozilla)
Attached patch part 5: Fix DOM (obsolete) — Splinter Review
Attachment #8454757 - Flags: review?(bent.mozilla)
Attached patch part 6: Fix ipc (obsolete) — Splinter Review
Attachment #8454758 - Flags: review?(bent.mozilla)
Attached patch part 7: netwerk (obsolete) — Splinter Review
Attachment #8454759 - Flags: review?(honzab.moz)
Attached patch part 8: Fix parser (obsolete) — Splinter Review
Attachment #8454761 - Flags: review?(hsivonen)
Attached patch part 9: fix rdf (obsolete) — Splinter Review
Attachment #8454762 - Flags: review?(benjamin)
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...
Attachment #8454752 - Flags: review?(ehsan) → review-
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.
Attachment #8454759 - Flags: review?(honzab.moz) → review+
Attachment #8455361 - Flags: review?(bgirard)
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.
Attachment #8454762 - Flags: review?(benjamin) → review-
Attachment #8455361 - Flags: review?(bgirard) → review+
(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.
Attachment #8454754 - Flags: review?(jmuizelaar) → 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.)
Attached patch Part 10: Rest (obsolete) — Splinter Review
Attachment #8455728 - Flags: review?(ehsan)
Comment on attachment 8454757 [details] [diff] [review]
part 5: Fix DOM

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

Trivial, removing review request.
Attachment #8454757 - Flags: review?(bent.mozilla)
Comment on attachment 8454758 [details] [diff] [review]
part 6: Fix ipc

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

Trivial, removing review request.
Attachment #8454758 - Flags: review?(bent.mozilla)
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.
Attachment #8454761 - Flags: review?(hsivonen)
Attachment #8454753 - Flags: review?(jmuizelaar)
Attached patch All fixes (obsolete) — Splinter 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>.
Attachment #8454753 - Attachment is obsolete: true
Attachment #8454754 - Attachment is obsolete: true
Attachment #8454757 - Attachment is obsolete: true
Attachment #8454758 - Attachment is obsolete: true
Attachment #8454759 - Attachment is obsolete: true
Attachment #8454761 - Attachment is obsolete: true
Attachment #8454762 - Attachment is obsolete: true
Attachment #8455361 - Attachment is obsolete: true
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! ;)
Attachment #8455728 - Flags: review?(ehsan) → review+
(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.
Attachment #8454756 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a4d189f5d7
Attachment #8454756 - Attachment is obsolete: true
Attachment #8455728 - Attachment is obsolete: true
Attachment #8455796 - Attachment is obsolete: true
Attachment #8456340 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/91a4d189f5d7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.