Closed
Bug 881323
Opened 11 years ago
Closed 11 years ago
Sanitize CycleCollectorParticipant implementation
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: smaug, Assigned: glandium)
References
Details
Attachments
(1 file)
The current setup is just a nightmare to hack. Need to figure out how to make it saner without causing extra static initializers.
Assignee | ||
Comment 1•11 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•11 years ago
|
||
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•11 years ago
|
Assignee: nobody → mh+mozilla
Comment 3•11 years ago
|
||
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+
Updated•11 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Assignee | ||
Comment 4•11 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 | ||
Comment 5•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/623333f62483
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/623333f62483
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•