New mozilla::RefCounted class conflicts with CycleCollector enum

RESOLVED FIXED in mozilla7

Status

()

Core
XPCOM
--
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bas, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Attachment #540483 - Flags: review?(peterv)
Attachment #540483 - Flags: feedback?(jones.chris.g)
(Assignee)

Comment 1

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

Comment 5

6 years ago
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)
(Reporter)

Comment 6

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

Comment 7

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

Comment 8

6 years ago
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.
(Reporter)

Comment 9

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

Comment 10

6 years ago
Sure.  I can whip up a patch if Peter thinks that might be worth looking into.
(Assignee)

Comment 11

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

Comment 13

6 years ago
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.
Attachment #540819 - Flags: review?(peterv)
Attachment #540819 - Flags: feedback?(jones.chris.g)
(Assignee)

Comment 14

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

Comment 15

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

Comment 16

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

Comment 17

6 years ago
Can we push this ASAP?
(Assignee)

Comment 18

6 years ago
Waiting for review from Peter.
Component: General → XPCOM
QA Contact: general → xpcom
(Assignee)

Updated

6 years ago
Attachment #540483 - Attachment is obsolete: true
Attachment #540483 - Flags: review?(peterv)
(Assignee)

Comment 19

6 years ago
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?
(Reporter)

Comment 20

6 years ago
Azure needs to land today if it is to ship with Firefox 7.
(Assignee)

Updated

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

Comment 22

6 years ago
Oh, good point!  Yeah, that should be inline-able.
(Assignee)

Comment 23

6 years ago
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.
Assignee: bas.schouten → continuation
Attachment #540822 - Attachment is obsolete: true
Attachment #541501 - Flags: review+
Attachment #540822 - Flags: review?(peterv)
(Assignee)

Comment 24

6 years ago
Looks okay to me, but now I have to wait until the tree reopens from Bug 666760...
Keywords: checkin-needed
Version: unspecified → Trunk
(Assignee)

Comment 25

6 years ago
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]
(Reporter)

Comment 27

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

Comment 28

6 years ago
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+.
(Reporter)

Comment 29

6 years ago
Half an hour is fine, we won't be landing Azure tonight.
(Assignee)

Comment 30

6 years ago
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.
(Reporter)

Comment 31

6 years ago
Sounds good, we'll deal with it tomorrow.
(Assignee)

Comment 32

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

Comment 33

6 years ago
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.
(Reporter)

Comment 34

6 years ago
Relanded on mozilla-inbound:

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

Comment 35

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Attachment #541501 - Flags: review?(peterv)
(Assignee)

Updated

5 years ago
Blocks: 749370
You need to log in before you can comment on or make changes to this bug.