Sanitize CycleCollectorParticipant implementation

RESOLVED FIXED in mozilla25

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: glandium)

Tracking

Trunk
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
The current setup is just a nightmare to hack. Need to figure out how to make
it saner without causing extra static initializers.
(Assignee)

Updated

5 years ago
Blocks: 616262
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 784623 [details] [diff] [review]
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.
Attachment #784623 - Flags: review?(continuation)
Attachment #784623 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Assignee: nobody → mh+mozilla
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
Attachment #784623 - Flags: review?(continuation)
Attachment #784623 - Flags: review?(bugs)
Attachment #784623 - Flags: review+
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
(Assignee)

Comment 4

5 years ago
(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)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 765159

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/623333f62483
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.