881323 - Sanitize CycleCollectorParticipant implementation

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

The current setup is just a nightmare to hack. Need to figure out how to make
it saner without causing extra static initializers.

Comment 1

[Mike Hommey [:glandium]]

So, it turns out that now we can use constexpr (MOZ_CONSTEXPR), and since constexpr on trivial constructors makes gcc behave with vtables (i.e. not emit a static initializer), it means we can essentially revert bug 616262 and be back to the old vtables.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Re-implement CycleCollectorParticipant with actual vtables, with constexpr to avoid static initializers

For the first one of you that takes this review. That more or less brings us back to what the code was before bug 616262.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 784623 [details] [diff] [review]
Re-implement CycleCollectorParticipant with actual vtables, with constexpr to avoid static initializers

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

Thanks for fixing this!

::: content/base/src/nsXMLHttpRequest.h
@@ -688,5 @@
>                                               public nsIChannelEventSink,
>                                               public nsIProgressEventSink,
>                                               public nsIInterfaceRequestor,
> -                                             public nsITimerCallback,
> -                                             public nsCycleCollectionParticipant

Woah, that is... odd.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +359,3 @@
>  {
>    CycleCollectedJSRuntime* runtime = reinterpret_cast<CycleCollectedJSRuntime*>
> +    (reinterpret_cast<char*>(this) -

This weirdness could probably be undone now that participants are normal classes, but I guess it is okay.

@@ -850,5 @@
>  
>  void
>  CycleCollectedJSRuntime::AddJSHolder(void* aHolder, nsScriptObjectTracer* aTracer)
>  {
> -  MOZ_ASSERT(aTracer->Trace, "AddJSHolder needs a non-null Trace function");

So, because this is just a regular method, Trace is always going to be non-null?

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +142,5 @@
> +        NS_ASSERTION(false, "Forgot to implement CanSkipThisReal?");
> +        return false;
> +    }
> +
> +    bool mMightSkip;

It looks like this field could be const and private.

@@ +433,5 @@
>  
>  #define NS_DECL_CYCLE_COLLECTION_CLASS_BODY_NO_UNLINK(_class, _base)           \
>  public:                                                                        \
> +  NS_IMETHOD Traverse(void *p, nsCycleCollectionTraversalCallback &cb);        \
> +  NS_IMETHOD_(void) DeleteCycleCollectable(void *n)                            \

p instead of n maybe?

@@ +627,5 @@
>    {                                                                            \
>       NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS_BODY(_class)                        \
>       static nsCycleCollectionParticipant* GetParticipant()                     \
>       {                                                                         \
> +    return &_class::NS_CYCLE_COLLECTION_INNERNAME;                             \

nit: indentation

@@ +646,3 @@
>      static nsScriptObjectTracer* GetParticipant()                              \
>      {                                                                          \
> +    return &_class::NS_CYCLE_COLLECTION_INNERNAME;                             \

nit: indentation

Comment 4

[Mike Hommey [:glandium]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Comment on attachment 784623 [details] [diff] [review]
> Re-implement CycleCollectorParticipant with actual vtables, with constexpr
> to avoid static initializers
> 
> Review of attachment 784623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for fixing this!
> 
> ::: content/base/src/nsXMLHttpRequest.h
> @@ -688,5 @@
> >                                               public nsIChannelEventSink,
> >                                               public nsIProgressEventSink,
> >                                               public nsIInterfaceRequestor,
> > -                                             public nsITimerCallback,
> > -                                             public nsCycleCollectionParticipant
> 
> Woah, that is... odd.

That one was just plain wrong. It could go without the patch. But the patch breaks without this removal.

> @@ -850,5 @@
> >  
> >  void
> >  CycleCollectedJSRuntime::AddJSHolder(void* aHolder, nsScriptObjectTracer* aTracer)
> >  {
> > -  MOZ_ASSERT(aTracer->Trace, "AddJSHolder needs a non-null Trace function");
> 
> So, because this is just a regular method, Trace is always going to be
> non-null?

Trace is either going to be non-null, or the compile will break (because of the Trace = 0 definition in CycleCollectionParticipant)

Comment 5

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/623333f62483

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/623333f62483