Closed Bug 665564 Opened 9 years ago Closed 9 years ago

New mozilla::RefCounted class conflicts with CycleCollector enum

Categories

(Core :: XPCOM, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: bas.schouten, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

There's an enumeration in the CycleCollector code called CCNodeType that puts some constants into the global namespace, one of those constants is 'RefCounted' which will conflict with the new mozilla::RefCounted class introduced in bug 661973.

For Azure we will start using the RefCounted class and it conflicts in numerous places. The proposed fix is to prefix all enumeration members with NodeType_

Attached is a patch.
Attachment #540483 - Flags: review?(peterv)
Attachment #540483 - Flags: feedback?(jones.chris.g)
There are also a few places in comments that use the enum names that should probably be updated with the new name.

nsCycleCollectionParticipant.h:
    // If type is RefCounted you must call DescribeNode() with an accurate

nsXPConnect.cpp:
    // Note that the conditions under which we specify GCMarked vs.
    // GCUnmarked are different between ExplainLiveExpectedGarbage and

Many of the DescribeNodes that you updated could be replaced with calls to NS_IMPL_CYCLE_COLLECTION_DESCRIBE, but perhaps Peter has an opinion about which way is better.
Comment on attachment 540483 [details] [diff] [review]
Prefix CCNodeType enumeration members

I'm not sure what style peterv prefers or whether this enum can live in another lexical context, but we definitely need to fix this enum.  I'm surprised it didn't bite our Yarr code, but maybe that uses "wtf::RefCounted".
Attachment #540483 - Flags: feedback?(jones.chris.g) → feedback+
Comment on attachment 540483 [details] [diff] [review]
Prefix CCNodeType enumeration members

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

Maybe Andrew can review this quicker.
Attachment #540483 - Flags: review?(continuation)
(In reply to comment #3)
> Maybe Andrew can review this quicker.

Sorry I didn't review this within 24 hours! :-/

Could we move the enum into nsCycleCollectionTraversalCallback?

(In reply to comment #1)
> Many of the DescribeNodes that you updated could be replaced with calls to
> NS_IMPL_CYCLE_COLLECTION_DESCRIBE, but perhaps Peter has an opinion about
> which way is better.

Yeah, replacing them with NS_IMPL_CYCLE_COLLECTION_DESCRIBE seems like a good idea!
Comment on attachment 540483 [details] [diff] [review]
Prefix CCNodeType enumeration members

Sorry, I don't think I'm the right sort of person to review a change like this.
Attachment #540483 - Flags: review?(continuation)
(In reply to comment #4)
> (In reply to comment #3)
> > Maybe Andrew can review this quicker.
> 
> Sorry I didn't review this within 24 hours! :-/

We need to get this fixed quickly because Azure needs to land soon, and this is blocking us from using the new mfbt refptr classes :).

> 
> Could we move the enum into nsCycleCollectionTraversalCallback?

We could move it, although I suppose it would cause long identifiers :)
If you added a DescribeRefcountedNode callback, and then just use that everywhere, then that would eliminate all calls to DescribeNode except in nsXPConnect.  At that point, you could almost eliminate the enum altogether from public, except that you'd need some new private header file that only the cycle collector and XPConnect use, and class DebugWrapperTraversalCallback in nsContentUtils also uses the enum class name, for some odd reason.  I don't know if having two entries into DescribeNode would be bad for performance or whatever.
Well, and you could add DescribeGCMarkedNode and DescribeGCUnmarkedNode.  Then you could eliminate DescribeNode from the CC interface, and then it would be okay to move the enum from nsCycleCollectionParticipant.h into nsCycleCollector.cpp.  This would require minor futzing in nsXPConnect, but maybe it wouldn't be that bad.
(In reply to comment #8)
> Well, and you could add DescribeGCMarkedNode and DescribeGCUnmarkedNode. 
> Then you could eliminate DescribeNode from the CC interface, and then it
> would be okay to move the enum from nsCycleCollectionParticipant.h into
> nsCycleCollector.cpp.  This would require minor futzing in nsXPConnect, but
> maybe it wouldn't be that bad.

I don't really feel comfortable making that big a change to a component I know nothing about in 1 day :).
Sure.  I can whip up a patch if Peter thinks that might be worth looking into.
A lighter weight alternative is to make a new macro like

#define NS_IMPL_CYCLE_COLLECTION_DESCRIBE_STRING(_class, _refcnt, _name) \
    cb.DescribeNode(RefCounted, _refcnt, sizeof(_class), _name);

This and NS_IMPL_CYCLE_COLLECTION_DESCRIBE will cover every use of the enums outside of nsXPConnect and nsCycleCollector, so if the RefCount name becomes something like BlahBlahHereIsMyClass::RefCounted it will only be an annoyance in 2 files.
(In reply to comment #10)
> Sure.  I can whip up a patch if Peter thinks that might be worth looking
> into.

Go for it, hiding the enum in the .cpp seems better.
This patch splits DescribeNode into DescribeRefCountedNode and DescribeGCedNode.  This allows us to entirely remove CCNodeType, RefCounted, GCMarked and GCUnmarked from the code.

It compiles, and I was able to browse for a few minutes.  I'll tryserver it.
Attachment #540819 - Flags: review?(peterv)
Attachment #540819 - Flags: feedback?(jones.chris.g)
For some reason I called it "CCNum" instead of "CCNodeType" in the patch comment, so I just fixed that.
Attachment #540819 - Attachment is obsolete: true
Attachment #540822 - Flags: review?(peterv)
Attachment #540822 - Flags: feedback?(jones.chris.g)
Attachment #540819 - Flags: review?(peterv)
Attachment #540819 - Flags: feedback?(jones.chris.g)
Looks like this had a clean try run on Linux. I'll do a full try run.
http://tbpl.mozilla.org/?tree=Try&rev=37a7f0745b03
Attachment #540822 - Flags: feedback?(jones.chris.g) → feedback+
Try run was good (I rebuilt the 3 oranges and they were okay).
http://tbpl.mozilla.org/?tree=Try&rev=32babdd01893
Can we push this ASAP?
Waiting for review from Peter.
Component: General → XPCOM
QA Contact: general → xpcom
Attachment #540483 - Attachment is obsolete: true
Attachment #540483 - Flags: review?(peterv)
Maybe Ben can review my patch today if Peter isn't going to be able to get to this on whatever time scale the Azure people are looking for?
Azure needs to land today if it is to ship with Firefox 7.
Attachment #540822 - Flags: review?(bent.mozilla)
Comment on attachment 540822 [details] [diff] [review]
remove CCNodeType by changing DescribeNode to Describe{RefCounted,GCed}Node

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

This looks good, but I'd still like peterv to review it later. Any changes he wants to make can happen after Azure lands. r=me!

::: xpcom/base/nsCycleCollector.cpp
@@ +1461,5 @@
>      NS_IMETHOD_(void) NoteXPCOMRoot(nsISupports *root);
>  
>  private:
> +    NS_IMETHOD_(void) DescribeNode(PRUint32 refCount, size_t objSz,
> +                                   const char *objName);

This should no longer be virtual, and can you inline it?
Attachment #540822 - Flags: review?(bent.mozilla) → review+
Oh, good point!  Yeah, that should be inline-able.
I addressed bent's review comments, by inlining the method and making it non-virtual.  Carrying forward his r+.

I'll push this to mozilla-incoming as soon as I make sure it builds, as it touches a bunch of files.
Assignee: bas.schouten → continuation
Attachment #540822 - Attachment is obsolete: true
Attachment #541501 - Flags: review+
Attachment #540822 - Flags: review?(peterv)
Looks okay to me, but now I have to wait until the tree reopens from Bug 666760...
Keywords: checkin-needed
Version: unspecified → Trunk
Landed on mozilla-inbound.
Keywords: checkin-needed
Whiteboard: [inbound]
Backed out from mozilla-inbound as the most likely culprit for causing mac64 opt test orange.
Whiteboard: [inbound]
Could this be the de-virtualization. If so, andrew, can you reland this with the virtualization returned, since we have a green try run on that.
I can do it in like half an hour, or you can do it now if you want.  Just land the patch with bent's r+.
Half an hour is fine, we won't be landing Azure tonight.
I think the tree will still be closed by the time I go to bed, so I won't be able to get to it tonight.  I'll try firing off some try runs over night to try to figure out what the deal is.
Sounds good, we'll deal with it tomorrow.
OSX64 opt try runs for the two versions against moz-central tip:
http://tbpl.mozilla.org/?tree=Try&rev=1c27a6a1cfbb
http://tbpl.mozilla.org/?tree=Try&rev=33fa747aa4c1
The new version of my patch came out clean on try.  I'm now running the new version against try after an Inbound merge, in case one of the patches in m-i caused my patch to freak out:
http://tbpl.mozilla.org/?tree=Try&rev=54c3e213206d
I'm also trying the other patch that landed at the same time as me in case that is the problem (though it seems unlikely given how small it is):
http://tbpl.mozilla.org/?tree=Try&rev=8c56bee019b6
If both of those are clean, then maybe the oranges were magically due to some kind of build system freakout after the mass cancellations of tests that happened right before I landed.
Comment on attachment 541501 [details] [diff] [review]
remove CCNodeType by changing DescribeNode to Describe{RefCounted,GCed}Node

I'll address any additional review comments from Peter in a followup bug, as needed.
Attachment #541501 - Flags: review?(peterv)
http://hg.mozilla.org/mozilla-central/rev/233d73076b2e

I'm not sure regarding the status of this bug, why is there a pushed and reviewed patch and then a new version of the patch with a review request? It's so confusing.

Please move additional work to a follow-up as it should have been.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Attachment #541501 - Flags: review?(peterv)
Blocks: CleanupCC
You need to log in before you can comment on or make changes to this bug.