Eliminate redundant Traverse/Unlink CC macros

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mccr8, Assigned: bjacob)

Tracking

(Blocks 1 bug)

unspecified
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

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

Comment 2

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

Comment 3

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

Comment 4

7 years ago
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'
Hm, the fun part actually is to remove the extra macro arguments.
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'
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'
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.
Not asking you to review this :-)
Posted patch part 2: manual fixups (obsolete) — Splinter Review
Attachment #677208 - Attachment is obsolete: true
Attachment #681193 - Flags: review?(continuation)
Reporter

Comment 11

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

I want to look over this, too.
Attachment #681191 - Flags: review?(continuation)
Missed one spot in B2G specific stuff. That explain the build failures we got on b2g on Try.
Attachment #681193 - Attachment is obsolete: true
Attachment #681193 - Flags: review?(continuation)
Attachment #681584 - Flags: review?(continuation)
Reporter

Comment 13

7 years ago
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!
Attachment #681584 - Flags: review?(continuation) → review+
Depends on: 811926
Still red. Issue was serious. Filed bug 811926.
Reporter

Comment 16

7 years ago
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.
Yup, exactly. But there is more than that to this: this field is a raw nsISupports*. See bug 811926.
Reporter

Comment 18

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

Comment 19

7 years ago
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. ;)
> 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?
Reporter

Comment 21

7 years ago
I guess I'm not 100% sure what NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS is actually for...
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.
Reporter

Comment 23

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

Okay, sounds good!  rs=me
Attachment #681191 - Flags: review?(continuation) → review+
Reporter

Updated

7 years ago
Depends on: 812246
No longer depends on: 812246
Reporter

Updated

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