Last Comment Bug 665564 - New mozilla::RefCounted class conflicts with CycleCollector enum
: New mozilla::RefCounted class conflicts with CycleCollector enum
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla7
Assigned To: Andrew McCreight (PTO-ish through 6-29) [:mccr8]
:
Mentors:
Depends on:
Blocks: CleanupCC 651858
  Show dependency treegraph
 
Reported: 2011-06-20 08:55 PDT by Bas Schouten (:bas.schouten)
Modified: 2012-06-13 21:29 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Prefix CCNodeType enumeration members (12.55 KB, patch)
2011-06-20 08:55 PDT, Bas Schouten (:bas.schouten)
cjones.bugs: feedback+
Details | Diff | Review
remove CCNum by changing DescribeNode to Describe{RefCounted,GCed}Node (21.12 KB, patch)
2011-06-21 11:51 PDT, Andrew McCreight (PTO-ish through 6-29) [:mccr8]
no flags Details | Diff | Review
remove CCNodeType by changing DescribeNode to Describe{RefCounted,GCed}Node (21.12 KB, patch)
2011-06-21 11:53 PDT, Andrew McCreight (PTO-ish through 6-29) [:mccr8]
bent.mozilla: review+
cjones.bugs: feedback+
Details | Diff | Review
remove CCNodeType by changing DescribeNode to Describe{RefCounted,GCed}Node (21.25 KB, patch)
2011-06-23 14:30 PDT, Andrew McCreight (PTO-ish through 6-29) [:mccr8]
continuation: review+
Details | Diff | Review

Description Bas Schouten (:bas.schouten) 2011-06-20 08:55:57 PDT
Created attachment 540483 [details] [diff] [review]
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 (PTO-ish through 6-29) [:mccr8] 2011-06-20 09:25:02 PDT
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 2011-06-20 12:15:31 PDT
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) (Exited; email my personal email if necessary) 2011-06-20 15:33:23 PDT
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] 2011-06-21 06:19:32 PDT
(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 (PTO-ish through 6-29) [:mccr8] 2011-06-21 06:47:47 PDT
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) 2011-06-21 07:10:26 PDT
(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 (PTO-ish through 6-29) [:mccr8] 2011-06-21 07:19:25 PDT
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 (PTO-ish through 6-29) [:mccr8] 2011-06-21 07:24:30 PDT
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) 2011-06-21 07:44:41 PDT
(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 (PTO-ish through 6-29) [:mccr8] 2011-06-21 07:47:43 PDT
Sure.  I can whip up a patch if Peter thinks that might be worth looking into.
Comment 11 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-06-21 08:01:01 PDT
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] 2011-06-21 08:45:28 PDT
(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 (PTO-ish through 6-29) [:mccr8] 2011-06-21 11:51:16 PDT
Created attachment 540819 [details] [diff] [review]
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 (PTO-ish through 6-29) [:mccr8] 2011-06-21 11:53:57 PDT
Created attachment 540822 [details] [diff] [review]
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 (PTO-ish through 6-29) [:mccr8] 2011-06-21 17:28:18 PDT
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 (PTO-ish through 6-29) [:mccr8] 2011-06-22 12:31:06 PDT
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) 2011-06-22 15:55:13 PDT
Can we push this ASAP?
Comment 18 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-06-22 16:09:05 PDT
Waiting for review from Peter.
Comment 19 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-06-23 10:51:31 PDT
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) 2011-06-23 10:52:12 PDT
Azure needs to land today if it is to ship with Firefox 7.
Comment 21 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-23 13:53:10 PDT
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 (PTO-ish through 6-29) [:mccr8] 2011-06-23 14:05:05 PDT
Oh, good point!  Yeah, that should be inline-able.
Comment 23 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-06-23 14:30:15 PDT
Created attachment 541501 [details] [diff] [review]
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 (PTO-ish through 6-29) [:mccr8] 2011-06-23 15:35:02 PDT
Looks okay to me, but now I have to wait until the tree reopens from Bug 666760...
Comment 25 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-06-23 16:38:02 PDT
Landed on mozilla-inbound.
Comment 26 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-23 21:44:18 PDT
Backed out from mozilla-inbound as the most likely culprit for causing mac64 opt test orange.
Comment 27 Bas Schouten (:bas.schouten) 2011-06-23 21:47:30 PDT
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 (PTO-ish through 6-29) [:mccr8] 2011-06-23 21:52:43 PDT
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) 2011-06-23 21:54:02 PDT
Half an hour is fine, we won't be landing Azure tonight.
Comment 30 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-06-23 22:18:56 PDT
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) 2011-06-23 22:31:02 PDT
Sounds good, we'll deal with it tomorrow.
Comment 32 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-06-23 23:02:39 PDT
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 (PTO-ish through 6-29) [:mccr8] 2011-06-24 06:36:58 PDT
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) 2011-06-24 10:35:15 PDT
Relanded on mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/233d73076b2e
Comment 35 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2011-06-24 10:58:08 PDT
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] 2011-06-25 03:18:48 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.