807437 - Eliminate redundant Traverse/Unlink CC macros

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Bug 806279 makes a bunch of the CC macros redundant, so we should eliminate them to make things less confusing. I split this into a separate bug because the patches are large and tedious.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Note that more is coming in bug 806279. The plan is to make all the _AMBIGUOUS macros redundant. Better wait until that is done before making the big automatic changes.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: convert traverse functions in CPP files, WIP

Just for fun, here's what the conversion of Traverse functions in .cpp files looks like.

Comment 3

[Andrew McCreight [:mccr8]]

It would also be nice to do the folding of the Traverse/Unlink declarations into a single line here.

Another thing that would be nice to do somewhere is to get rid of the weird cases that currently manually call the NoteEdge/NoteChild functions separately.

Comment 4

[Andrew McCreight [:mccr8]]

This is what I did for the conversions. You need to also run this for .cpp, because I was too lazy to figure out how to do that in one pass. Although the few .h files are kind of nice to have separate, as they are less boilerplatey. I haven't checked if these are up-to-date with the latest changes.

find . -name '*.h' | xargs perl -pi -e 's/NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR|NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY|NS_IMPL_CYCLE_COLLECTION_UNLINK_NSTARRAY/NS_IMPL_CYCLE_COLLECTION_UNLINK/g'

find . -name '*.h' | xargs perl -pi -e 's/NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR\(|NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY\(|NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR\(/NS_IMPL_CYCLE_COLLECTION_TRAVERSE\(/g'

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Hm, the fun part actually is to remove the extra macro arguments.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Ta-dah! The single command that converts it all is:

find . -type f | grep -v \\.hg | grep -v ^\\./obj | xargs sed -i 's/CYCLE_COLLECTION_\(UNLINK\|TRAVERSE\)_\(NSCOMPTR\|NSCOMPTR_AMBIGUOUS\|NSCOMARRAY\|NATIVE_PTR\|NATIVE_MEMBER\|NSTARRAY\|NSTARRAY_OF_NSCOMPTR\|NSTARRAY_MEMBER\)(\([a-zA-Z0-9_ ]\+\).*)\([^)]*\)/CYCLE_COLLECTION_\1(\3)\4/g'

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

The previous command couldn't handle multi-line patterns. Turns out that we have many.

Switched back to perl for that:

find . -type f | grep -v \\.hg | grep -v ^\\./obj | grep -v event_impl_gen\\.py | xargs perl -i -pe 'BEGIN{undef $/;} s/CYCLE_COLLECTION_(UNLINK|TRAVERSE)_(NSCOMPTR|NSCOMPTR_AMBIGUOUS|NSCOMARRAY|NATIVE_PTR|NATIVE_MEMBER|NSTARRAY|NSTARRAY_OF_NSCOMPTR|NSTARRAY_MEMBER)\(([a-zA-Z0-9_]+).*?\)/CYCLE_COLLECTION_\1\(\3\)/gs'

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Generalized the regex a little for some unusual cases:

find . -type f | grep -v \\.hg | grep -v ^\\./obj | grep -v event_impl_gen\\.py | xargs perl -i -pe 'BEGIN{undef $/;} s/CYCLE_COLLECTION_(UNLINK|TRAVERSE)_(NSCOMPTR|NSCOMPTR_AMBIGUOUS|NSCOMARRAY|NATIVE_PTR|NATIVE_MEMBER|NSTARRAY|NSTARRAY_OF_NSCOMPTR|NSTARRAY_MEMBER)\(([^,)]+).*?\)/CYCLE_COLLECTION_\1\(\3\)/gs'

This still doesn't handle fields with () parentheses such as mArray.ElementAt(i)->Foo but there are very few such cases.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 1: the automatic changes made by the perl script

Not asking you to review this :-)

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 2: manual fixups

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 681191 [details] [diff] [review]
part 1: the automatic changes made by the perl script

I want to look over this, too.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 2: manual fixups

Missed one spot in B2G specific stuff. That explain the build failures we got on b2g on Try.

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 681584 [details] [diff] [review]
part 2: manual fixups

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

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ -470,5 @@
>  
>  #define NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR(_field)                       \
>    CycleCollectionNoteChild(cb, tmp->_field, #_field);
>  
> -#define NS_IMPL_CYCLE_COLLECTION_TRAVERSE(_field)                     \

yay!

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Try B2G: https://tbpl.mozilla.org/?tree=Try&rev=a77ef5025948

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Still red. Issue was serious. Filed bug 811926.

Comment 16

[Andrew McCreight [:mccr8]]

Well, I noticed your script doesn't properly convert TelephonyCall or CallEvent, I guess because the script matches the close paren of ToISupports. I don't know if that is related.

 NS_IMPL_CYCLE_COLLECTION_TRAVERSE(tmp->mCall->ToISupports(),
                                   TelephonyCall, "mCall")

The ", TelephonyCall, "mCall"" shouldn't be there.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Yup, exactly. But there is more than that to this: this field is a raw nsISupports*. See bug 811926.

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 681191 [details] [diff] [review]
part 1: the automatic changes made by the perl script

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

Aside from the two places with ToISupports() where the regexps missed things, my main concern here is the conversion of the _AMBIGUOUS macros. I've been auditing these by hand. Of the ones I've looked at, a number of them would seem to have their behavior changed by your patch. It seems like this is probably an error in the existing code, but maybe not. Here's what I have so far, I don't claim it is entirely right or sensible.

::: accessible/src/base/nsAccessiblePivot.cpp
@@ -57,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CLASS(nsAccessiblePivot)
>  
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsAccessiblePivot)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mRoot, nsIAccessible)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mPosition, nsIAccessible)

These fields have type nsRefPtr<Accessible>, but Accessible inherits from  nsAccessNode (via nsAccessNodeWrap), so it looks like it will end up being a different pointer? But maybe the AMBIGUOUS field is actually wrong?

::: accessible/src/generic/DocAccessible.cpp
@@ -133,5 @@
>    }
>  
>    uint32_t i, length = tmp->mChildDocuments.Length();
>    for (i = 0; i < length; ++i) {
> -    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mChildDocuments[i],

This has type DocAccessible, which inherits from Accessible. As above, this means with your patch we're getting a different pointer (I think?).

::: content/base/src/nsContentIterator.cpp
@@ -1147,5 @@
>  NS_INTERFACE_MAP_END_INHERITING(nsContentIterator)
>  
>  NS_IMPL_CYCLE_COLLECTION_CLASS(nsContentSubtreeIterator)
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsContentSubtreeIterator, nsContentIterator)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mRange, nsIDOMRange)

This matches the declaration.

::: content/base/src/nsDocument.cpp
@@ -1658,5 @@
>      tmp->mBoxObjectTable->EnumerateRead(BoxObjectTraverser, &cb);
>    }
>  
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mChannel)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mStyleAttrStyleSheet, nsIStyleSheet)

This matches the declaration.

@@ -1665,5 @@
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mOnloadBlocker)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mFirstBaseNodeWithHref)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDOMImplementation)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mImageMaps,
> -                                                       nsIDOMNodeList)

mImageMaps has type nsRefPtr<nsContentList>, but nsContentList and all of its parents seem to be unambiguous, so I guess this wasn't really needed anyways.

::: content/base/src/nsDocumentEncoder.cpp
@@ -184,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsDocumentEncoder)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDocument)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mSelection)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mRange, nsIDOMRange)

This looks fine.

::: content/base/src/nsEventSource.cpp
@@ -644,5 @@
>  
>  NS_IMPL_CYCLE_COLLECTION_CLASS(AsyncVerifyRedirectCallbackFwr)
>  
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(AsyncVerifyRedirectCallbackFwr)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mEventSource, nsIEventSource)

mEventSource has type nsRefPtr<nsEventSource>, and nsEventSource CC inherits from nsDOMEventTargetHelper, so I think this will end up turning it into a nsDOMEventTargetHelper pointer, instead of an nsIEventSource pointer, which seems odd.

::: content/base/src/nsXMLHttpRequest.cpp
@@ -606,5 @@
> -
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mChannelEventSink)
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mProgressEventSink)
> -
> -  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mUpload,

This is kind of weird. mUpload has type nsRefPtr<nsXMLHttpRequestUpload>, but nsXMLHttpRequestUpload isn't a cycle collected class, so I'm not sure your conversion stuff will work? It inherits from nsXHREventTarget and nsIXMLHttpRequestUpload, but only the former seems to be cycle collected, so it seems a little odd it would be trying to forward to the second one.

Comment 19

[Andrew McCreight [:mccr8]]

Though I suppose any AMBIGUOUS problems would really be a problem with bug 806279, as those args are ignored by the time we get here. ;)

Comment 20

[Boris Zbarsky [:bzbarsky]]

> These fields have type nsRefPtr<Accessible>, but Accessible inherits from  nsAccessNode
> (via nsAccessNodeWrap), so it looks like it will end up being a different pointer?

Why does that matter, though?  It doesn't matter which nsISupports gets passed to NoteXPCOMChild, since all that will do is QI to the cycle collection interface, which will return the same thing no matter what, right?

Comment 21

[Andrew McCreight [:mccr8]]

I guess I'm not 100% sure what NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS is actually for...

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Before bug 806279,  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR had no way to infer any way to cast to nsISupports* if it's an ambiguous base. It couldn't use the cycle collection innerclass's Upcase() because it had to support non-CC fields. Whence the need for the separate _AMBIGUOUS variant.

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 681191 [details] [diff] [review]
part 1: the automatic changes made by the perl script

Okay, sounds good!  rs=me

Comment 24

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6c9caeb3422b
http://hg.mozilla.org/integration/mozilla-inbound/rev/dce37eb7b01a

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6c9caeb3422b
https://hg.mozilla.org/mozilla-central/rev/dce37eb7b01a