665564 - New mozilla::RefCounted class conflicts with CycleCollector enum

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Prefix CCNodeType enumeration members

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.

Comment 1

[Andrew McCreight [:mccr8]]

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 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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".

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 540483 [details] [diff] [review]
Prefix CCNodeType enumeration members

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

Maybe Andrew can review this quicker.

Comment 4

[Peter Van der Beken [:peterv]]

(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 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Bas Schouten (:bas.schouten)]

(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 :)

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Bas Schouten (:bas.schouten)]

(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 :).

Comment 10

[Andrew McCreight [:mccr8]]

Sure.  I can whip up a patch if Peter thinks that might be worth looking into.

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Peter Van der Beken [:peterv]]

(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.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: remove CCNum by changing DescribeNode to Describe{RefCounted,GCed}Node

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.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: remove CCNodeType by changing DescribeNode to Describe{RefCounted,GCed}Node

For some reason I called it "CCNum" instead of "CCNodeType" in the patch comment, so I just fixed that.

Comment 15

[Andrew McCreight [:mccr8]]

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

Comment 16

[Andrew McCreight [:mccr8]]

Try run was good (I rebuilt the 3 oranges and they were okay).
http://tbpl.mozilla.org/?tree=Try&rev=32babdd01893

Comment 17

[Bas Schouten (:bas.schouten)]

Can we push this ASAP?

Comment 18

[Andrew McCreight [:mccr8]]

Waiting for review from Peter.

Comment 19

[Andrew McCreight [:mccr8]]

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?

Comment 20

[Bas Schouten (:bas.schouten)]

Azure needs to land today if it is to ship with Firefox 7.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 22

[Andrew McCreight [:mccr8]]

Oh, good point!  Yeah, that should be inline-able.

Comment 23

[Andrew McCreight [:mccr8]]

Attachment: remove CCNodeType by changing DescribeNode to Describe{RefCounted,GCed}Node

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.

Comment 24

[Andrew McCreight [:mccr8]]

Looks okay to me, but now I have to wait until the tree reopens from Bug 666760...

Comment 25

[Andrew McCreight [:mccr8]]

Landed on mozilla-inbound.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Backed out from mozilla-inbound as the most likely culprit for causing mac64 opt test orange.

Comment 27

[Bas Schouten (:bas.schouten)]

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.

Comment 28

[Andrew McCreight [:mccr8]]

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+.

Comment 29

[Bas Schouten (:bas.schouten)]

Half an hour is fine, we won't be landing Azure tonight.

Comment 30

[Andrew McCreight [:mccr8]]

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.

Comment 31

[Bas Schouten (:bas.schouten)]

Sounds good, we'll deal with it tomorrow.

Comment 32

[Andrew McCreight [:mccr8]]

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

Comment 33

[Andrew McCreight [:mccr8]]

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 34

[Bas Schouten (:bas.schouten)]

Relanded on mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/233d73076b2e

Comment 35

[Andrew McCreight [:mccr8]]

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.

Comment 36

[Marco Bonardo [:mak]]

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.