806279 - Make CC macros more generic, to the extent possible

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Attachment: part 1: implement type-generic CC UNLINK/TRAVERSE macros

Goal: make it easy and fool-proof, for Mozilla developers, to declare their classes to the CC.

Remark: the various helper macros such as NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2 already serve that purpose.

Problem: they are very limited in what they can be used to declare, so that in many cases they can't be used. Indeed, they use NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR, which means that:
 1. They can only be used to declare refpointers and not e.g. arrays.
 2. They cannot handle the cases where nsISupports is an ambiguous base (then need _AMBIGUOUS macros).

So the goal of this bug is to fix the above 2 issues with NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR:

1. (The main problem) Replace NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR by a generic macro NS_IMPL_CYCLE_COLLECTION_UNLINK that can be used to declare other types e.g. arrays. This is feasible using a template helper.

2. (Much smaller problem) Generalize the use of the ToSupports helpers to automatically handle the case where nsISupports is an ambiguous base. Currently, ToSupports lives there:
http://hg.mozilla.org/mozilla-central/file/e069342dc665/js/xpconnect/src/qsObjectHelper.h#l16
I guess it could be moved to nsISupports itself. Users who need to declare to the CC classes that have nsISupports as an ambiguous base, would then simply need to overload ToSuppports for it, just like when writing DOM bindings.

The attached patch is a proposal implementation of part 1. It adds type-generic NS_IMPL_CYCLE_COLLECTION_UNLINK and NS_IMPL_CYCLE_COLLECTION_TRAVERSE macros. I would like feedback before going further.

Maybe the most controversial part about it is going to be that it adds a requirement that any refpointer class, and any array class, that will be used in fields declared to the CC, has to first be declared itself with new macros:

NS_TEACH_CYCLE_COLLECTION_HOW_TO_UNLINK_REFPTR_CLASS
NS_TEACH_CYCLE_COLLECTION_HOW_TO_UNLINK_ARRAY_CLASS
NS_TEACH_CYCLE_COLLECTION_HOW_TO_TRAVERSE_REFPTR_CLASS
NS_TEACH_CYCLE_COLLECTION_HOW_TO_TRAVERSE_ARRAY_CLASS

If you wonder why these take varargs, that's because you may typically want to pass them e.g. "MyArrayClass<T, Allocator>" and the comma there would be interpreted by the preprocessor as macro argument delimiter. It's also a bit ugly that the user has to list template parameters manually, as in

template<class E, class Alloc>
NS_TEACH_CYCLE_COLLECTION_HOW_TO_UNLINK_ARRAY_CLASS(Clear, nsTArray<E, Alloc>)

But I don't see a way around that in C++98 without variadic templates. If we ever get variadic templates, we'll be able to make this nicer.

If feedback is positive, the plan is to do part 2 (should be much more down-to-earth but potentially tedious if many ToSupports functions need to be added) and adapt the helper macros such as NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2 to use them. It should then be possible in the vast majority of cases to declare one's classes to the CC simply by using one of those nice helper macros.

Comment 1

[(no longer active)]

I talked with Benoit about this over lunch today.  I think that we should land something like this together with another patch which removes the usages and definitions of the _NSCOMPTR/_NSTARRAY/... macros.  I really don't want us to add to the number of ways that we can hook something up to the cycle collector.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I like the approach and I agree with Ehsan.

Comment 3

[Andrew McCreight [:mccr8]]

Very nice!  I like the concept.  I think it is reasonable to require a few declarations of the helpers for these classes, as it seems like there are only a few of them.  I have some comments on a few of the specifics.

Why the extra level of indirection in declaring these nsImplCycleCollectionUnlinkHelper classes?  Why can't avoid that and just declare things like:

template<typename T>
inline void nsImplCycleCollectionUnlink(nsRefPtr<T>& field)
{
  field = nullptr;
}

I tried it locally and it seemed to work okay.

Also, I'm not sure how much value macroizing these individual declarations really adds, especially given the VARARGS weirdness, but I could be convinced otherwise.

You could avoid the double-string-argument weirdness by declaring something like:
#define NS_CYCLE_COLLECTION_NOTE_ARRAY_EDGE_NAME(_cb, _name)  \
  if (NS_UNLIKELY((_cb).WantDebugInfo())) {                   \
    nsAutoCString arrayEdgeName(_name);                       \
    arrayEdgeName.AppendLiteral("[i]");                       \
    (_cb).NoteNextEdgeName(arrayEdgeName.get());              \
  }
This would cause a minor amount of code bloat, and additional runtime cost when we're creating dumps, but it might be a reasonable tradeoff to avoid having these phantom arguments you don't use sometimes.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 676041 [details] [diff] [review]
part 1: implement type-generic CC UNLINK/TRAVERSE macros

But note, new code should use Gecko coding style.
2 space indentation, correct parameter naming ... all that usual.

Comment 5

[Andrew McCreight [:mccr8]]

Also, I'd be happy to write the mindless patch that eliminate the now-redundant NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR etc. macros if you don't want to, Benoit.

Comment 6

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

Attachment: part 1: implement type-generic CC UNLINK/TRAVERSE macros

OK, thanks for the comments, this version applies your review comments.

Indeed, in this particular case there was no value in the indirection to a template struct helper.

Also removed the TEACH_ macros.

NS_CYCLE_COLLECTION_NOTE_EDGE_NAME didn't need to be a macro, either. It's now a function. Applied your idea to avoid passing the two literal strings. For the array case, since it's not a litteral string anymore, it didn't cost much to put the real array index instead of an abstract index, "[i]". I couldn't use nsStrings as you suggested, because that created a circular dependency: nsString headers included nsTArray.h which included my stuff. The simplest seemed to be to use PR_snprintf directly. Also, notice that while the documentation says that nsCAutoString has a static storage of 64 bytes, MXR says that nsCAutoString is a typedef for nsCString. There is nsPrintfCString, but I couldn't use it either due to the above-mentioned circular dependency problem.

Comment 7

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

Attachment: part 1: implement type-generic CC UNLINK/TRAVERSE macros

Missed a #include "nscore.h"

Comment 8

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsTString.h#436 and http://mxr.mozilla.org/mozilla-central/ident?i=nsTAutoString_CharT show that the nsAutoCString definition is actually a bit more than the typedef that MXR shows.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

nit about the new code.
* & etc go with the type, not with parameter name.

Comment 10

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

Attachment: part 1: implement type-generic CC UNLINK/TRAVERSE macros

Applied style nit.

Comment 11

[Andrew McCreight [:mccr8]]

Why do you parametrize over CallbackType?  Is that just to avoid include hell?

Comment 12

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

Yes, that's the only reason. If you're OK to move nsCycleCollectionTraversalCallback to this file, then we don't need to templatize in CallbackType.

Comment 13

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

The next step is to remove all the _AMBIGUOUS macros.

As noted above NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS is easy: just need ToSupports overloads.

Other _AMBIGUOUS macros seem to rely on things like NS_DECL_CYCLE_COLLECTION_CLASS_BODY that want to implement a Downcast method casting a nsISupports to a subclass that has nISupports as an ambiguous base. To do this generically we'll need functions akin to ToSupports but doing the converse job. This could take the form a function template

template<typename T>
inline T* CastSupportsTo(nsISupports*);

That we would then specialize. If we need this for templated types, then we will need to partially specialize CaseSupportsTo e.g.

template<typename T>
inline Foo<T>* CastSupportsTo<Foo<T> >(nsISupports*);

which isn't allowed for function templates. So _there_ we will need a level of indirection with a template struct helper.

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 676984 [details] [diff] [review]
part 1: implement type-generic CC UNLINK/TRAVERSE macros

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

This looks good! r- because of my questions about ImplCycleCollectionTraverse.

::: xpcom/glue/nsCOMArray.h
@@ +116,5 @@
> +  aField.Clear();
> +}
> +
> +template<typename CallbackType>
> +inline void ImplCycleCollectionTraverse(CallbackType& aCallback,

It is kind of a shame how boiler-platey these functions are, but I'm not sure what is the lesser of evils is between boilerplate and weird VARARGS macro templates. :) Oh well, people probably don't add new array types very often!

Maybe you could just make the body a macro? Depends on how you feel about if that is better/worse...

::: xpcom/glue/nsCycleCollectionTraverse.h
@@ +18,5 @@
> +    aCallback.NoteNextEdgeName(aName);
> +  }
> +}
> +
> +#define NS_CYCLE_COLLECTION_NOTE_EDGE_NAME CycleCollectionNoteEdgeName

I think it makes more sense to put this in nsCycleCollectionParticipant.h, as it is mostly a backwards compatibility shim.

@@ +35,5 @@
> +}
> +
> +template<typename CallbackType, typename T>
> +inline void ImplCycleCollectionTraverse(CallbackType& aCallback,
> +                                        T* aField,

Is there some reason this has to be here and not in nsCycleCollectionParticipant.h? This file is used in places that want to declare how they behave as cycle collected fields, whereas this method is used by places that actually are declaring Traverse/Unlink functions, and will be including nsCCP.h. Then you could drop the CallbackType parameter, and use the actual type instead.

Given that NoteXPCOMChild takes an nsISupports* as a child, can we avoid parameterizing over T here and just make aField have type nsISupports*?  I don't know how subtyping, overloading, and templates are going to interact here.

Ideally, we'd have one case for "raw pointers that are subtypes of nsISupports" and one case for "raw pointers that aren't subtypes of nsISupports", so we could cover the NATIVE case. This isn't a huge deal because NATIVE is pretty rare right now, but do you think it might be possible to eventually support that?

Comment 15

[Andrew McCreight [:mccr8]]

nit: Also, please make the \'s line up in NS_IMPL_CYCLE_COLLECTION_TRAVERSE, NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS and  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR. Thanks.

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 676984 [details] [diff] [review]
part 1: implement type-generic CC UNLINK/TRAVERSE macros

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

::: xpcom/glue/nsTObserverArray.h
@@ +378,5 @@
> +{
> +  size_t length = aField.Length();
> +  for (size_t i = 0; i < length; ++i) {
> +    CycleCollectionNoteArrayEdgeName(aCallback, aFieldName, i);
> +    aCallback.NoteXPCOMChild(aField[i]);

I started poking around at the various places in the code that could possibly use this (yay!), and from trying to replace the current glob of code in nsAccessiblePivot, it seems like maybe this should be aField.ElementAt(i), not aField[i]? Does that make sense?

Comment 17

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

I used operator[] because that seemed to be the accessor that every array class would offer. Do you mean we have an array class that doesn't offer operator[]?

If so, we could always revert to the macro approach and add a macro parameter allowing to choose an accessor method (like 'operator[]' or 'ElementAt'). Or we could have an overridable template helper for that, but that would be a bit "over the board". I would then really ask, why would these classes not want to offer operator[]?

Comment 18

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

Oh wait, I got confused in comment 17. That code is in the specific custom code added for nsTObserverArray.h. There, we know that the class at hand, nsTObserverArray, offers operator[]. I don't understand the problem?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 676984 [details] [diff] [review]
part 1: implement type-generic CC UNLINK/TRAVERSE macros

I'll review the patch after Andrew's comments are addressed. 
(IMO ImplCycleCollectionTraverse is ok. There aren't that many cases using it.)

But I don't understand why we need anything for nsCOMArray_base
Why isn't nsCOMArray<T> enough?


Why is 
template<typename CallbackType, typename E>
inline void ImplCycleCollectionTraverse(CallbackType& aCallback,
                                        FallibleTArray<E>& aField,
                                        const char* aFieldName)
ok? I mean, wouldn't that "work" with
FallibleTArray<nsISupports*>, which isn't actually keeping strong refs
to its elements.

Comment 20

[Andrew McCreight [:mccr8]]

(In reply to Benoit Jacob [:bjacob] from comment #18)
> There, we know that the class at hand,
> nsTObserverArray, offers operator[]. I don't understand the problem?

nsTObserverArray doesn't define an operator[], as far as I can see.  I could file a bug for one if you'd like.

Comment 21

[Andrew McCreight [:mccr8]]

Now that I am thinking about it, I wonder if it might be a good idea to drop the T* case of ImplCycleCollectionTraverse altogether, thus requiring explicit use of the RAWPTR macro. There are only 5 uses of this macro, and I think it is kind of weird and dangerous. I'm not even sure offhand why those 5 cases are okay.

(In reply to Olli Pettay [:smaug] from comment #19)
> I mean, wouldn't that "work" with
> FallibleTArray<nsISupports*>, which isn't actually keeping strong refs
> to its elements.

This is certainly unfortunate, but the same problem exists with using NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR on an array like that. I guess at least in that case it is somewhat more obvious that you are doing something wrong. The simplest way to sort of deal with this would be to change
  aCallback.NoteXPCOMChild(aField[i]);
to
  aCallback.NoteXPCOMChild(aField[i].get());
...so that you'd get an error if you tried to use this on an nsTArray that doesn't have a get method.

A fancier way to deal with this would be to do something like
  ImplCycleCollectionTraverse(aCallback, aField[i], aFieldName (munged up with []);
...then it would attempt (I think?) to recursively instantiate it with the inner type, and thus work for nsRefPtr and nsCOMPtr, but fail for raw pointers, assuming we drop the T* constructor...

Comment 22

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

(In reply to Andrew McCreight [:mccr8] from comment #14)
> Ideally, we'd have one case for "raw pointers that are subtypes of
> nsISupports" and one case for "raw pointers that aren't subtypes of
> nsISupports", so we could cover the NATIVE case. This isn't a huge deal
> because NATIVE is pretty rare right now, but do you think it might be
> possible to eventually support that?

So, yes we can do this with SFINAE. The least hairy way to do that --- only a few lines as shown below --- assumes that it's OK that we add one dummy typedef to nsISupports. I assume that's OK as it has no runtime incidence. We could even make it private and friend the helper below. Here's the code (selfcontained runnable example):

=============

#include <iostream>

struct nsISupports {
  typedef int ISupportsMarkerTypedef;
};
struct SomeClassInheritingISupports : nsISupports {};
struct SomeClassInheritingISupports2 : nsISupports {};

struct SomeClassHavingISupportsAsAmbiguousBase
  : SomeClassInheritingISupports
  , SomeClassInheritingISupports2
{};

struct SomeUnrelatedClass {};

template <typename T>
struct IsISupports {
    typedef char yes[1];
    typedef char no[2];

    template <typename C>
    static yes& test(typename C::ISupportsMarkerTypedef*);

    template <typename>
    static no& test(...);

    static const bool value = sizeof(test<T>(0)) == sizeof(yes);
};

#define TEST(type) \
  std::cout << #type " does "; \
  if (!IsISupports<type>::value) \
    std::cout << "*NOT* "; \
  std::cout << "inherit ISupports" << std::endl;

int main() {
  TEST(nsISupports)
  TEST(SomeClassInheritingISupports)
  TEST(SomeClassHavingISupportsAsAmbiguousBase)
  TEST(SomeUnrelatedClass)
}

======================

Output:

bjacob:~$ g++ a.cpp -o a && ./a
nsISupports does inherit ISupports
SomeClassInheritingISupports does inherit ISupports
SomeClassHavingISupportsAsAmbiguousBase does inherit ISupports
SomeUnrelatedClass does *NOT* inherit ISupports

======================

Comment 23

[Mike Hommey [:glandium]]

(In reply to Benoit Jacob [:bjacob] from comment #22)
> (In reply to Andrew McCreight [:mccr8] from comment #14)
> > Ideally, we'd have one case for "raw pointers that are subtypes of
> > nsISupports" and one case for "raw pointers that aren't subtypes of
> > nsISupports", so we could cover the NATIVE case. This isn't a huge deal
> > because NATIVE is pretty rare right now, but do you think it might be
> > possible to eventually support that?
> 
> So, yes we can do this with SFINAE. The least hairy way to do that --- only
> a few lines as shown below --- assumes that it's OK that we add one dummy
> typedef to nsISupports. I assume that's OK as it has no runtime incidence.
> We could even make it private and friend the helper below. Here's the code
> (selfcontained runnable example):

mozilla::IsBaseOf from mozilla/TypeTraits.h does the same thing, but doesn't need a typedef in nsISupports.
You can use

Comment 24

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

(In reply to Mike Hommey [:glandium] from comment #23)
> mozilla::IsBaseOf from mozilla/TypeTraits.h does the same thing, but doesn't
> need a typedef in nsISupports.
> You can use

Oh, I didn't know that we had that. I also didn't know about that trick. Tested it, it really works --- even in the ambiguous-base case. Thanks!

Comment 25

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

Attachment: part 1: generic TRAVERSE/UNLINK for different pointer/array types

Hopefully this addresses above review comments and I didn't forget something.

The raw-pointer case is moved back to nsCCP.h. A forthcoming patch is going to use IsBaseOf to split it into the above-discussed two cases.

Sorry about the confusion about ElementAt. I was still thinking in terms of generic TEACH_ macros that had to work with all array types. That is of course no longer part of the picture.

Comment 26

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

Attachment: part 2: generic BEGIN_ macros folding in the NATIVE case

Next up is folding in the NATIVE_ TRAVERSE and UNLINK macros. For that, we will need to unify NoteXPCOMChild and NoteNativeChild into a single templated function. Is that OK? My plan would be to keep those functions unchanged and add a new NoteXPCOMOrNativeChild function on top of them.

Comment 27

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

Attachment: part 2: generic BEGIN_ macros folding in the NATIVE case

update: fix compile. Some more code was relying on macros to declare a local variable named 's'.

Comment 28

[Andrew McCreight [:mccr8]]

Over all, part 1 looks good to me. My remaining concern is what to do about "raw" pointers. Maybe if I give a little context my concerns will be more clear.

When we call NoteXPCOMChild, we are telling the cycle collector that this is an edge we own a reference on, and it will increment the number of references to the child it thinks it has. Generally, we only want to do this for ref counted pointers, because that is the only time we really own a reference. If we do this for other pointers, then the cycle collector may think there are more references than there really are, and throw things away that should be kept alive.

The exceptions here, where the RAWPTR macros are used, are bizarre: instead of holding a direct ref counted pointer, there is actually some other object somewhere else that holds the ref counted pointer, and we are careful not to traverse that other edge.

Your patch I think goes too far in making it easy to report raw pointers to the cycle collector. The basic idea here is that we don't want raw pointers to nsISupports things to be easy to use. Do you agree, Olli?

1. The generic T* case of ImplCycleCollectionTraverse will make individual raw pointers get slurped up automatically. To fix this, I think you can remove the generic T* case defined in nsCycleCollectionParticipant.h and leave the existing definition of NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR alone. As I said above, this only is used 5 places, so making these annoying isn't a huge deal.

2. Some of the arrays you add support for (nsTArray, nsTObserverArray) do not strongly hold their elements. If somebody had an nsTArray<nsINode*>, then it would work with this patch without any alarm, which would be bad.

The existing cases used with NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR are nsTArray<nsCOMPtr<*>>, nsTArray<nsRefPtr<*>>, nsTArray<WebGLRefPtr<*>>,   FallibleTArray<nsCOMPtr<*>> and nsTArray<WebGLVertexAttribData>. Is there any way we can require that the inner type be a declared CC class? The WebGLVertex case is weird, but maybe we could special case that.

For nsTObserverArray, the only use seems to be nsTObserverArray<nsCOMPtr<nsIAccessiblePivotObserver> > in nsAccessiblePivot.

NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY and NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_MEMBER (which are native, and thus not covered in part 1, but I include them for completeness) only use nsTArray<nsRefPtr<*>>.

Comment 29

[Andrew McCreight [:mccr8]]

> Is there any way we can require that the inner type be a declared CC class

By this I mean, "a class we've declared using ImplCycleCollectionTraverse", as in nsRefPtr, etc.

Comment 30

[Andrew McCreight [:mccr8]]

Something like this is what I was thinking for nsTArray:
  for (size_t i = 0; i < length; ++i) {
    ImplCycleCollectionTraverse(aCallback, aField[i], aFieldName);
  }

I'm not sure if that works or not. This particular implementation loses the niceness of the names, but meh that's not a big deal. On the other hand, it would potentially handle things like nsTArray<nsTArray<nsCOMPtr<nsIContent>>>. ;)

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #28) 
> Your patch I think goes too far in making it easy to report raw pointers to
> the cycle collector. The basic idea here is that we don't want raw pointers
> to nsISupports things to be easy to use. Do you agree, Olli?
I agree. Raw pointers are exception and it is IMO ok if using them is a bit difficult
(since people should use smart pointers whenever possible).
Making it easy to pass non-owning raw pointers to CC is error prone.

Comment 32

[Andrew McCreight [:mccr8]]

The thing I posted in comment 30 is wrong, but thanks to the C++ FAQ I think I have something that does work:

  for (size_t i = 0; i < length; ++i) {
    ImplCycleCollectionTraverse<CallbackType, typename E::element_type>(aCallback, aField[i], aFieldName);
  }

This requires adding |typedef T element_type| to WebGLRefPtr, but otherwise seems to work. Well, it does not work for the odd WebGLVertexAttribData case, but maybe I can figure out some way to handle that.

Comment 33

[Andrew McCreight [:mccr8]]

Attachment: make raw pointers more annoying to use, WIP

Comment 34

[Andrew McCreight [:mccr8]]

Attachment: make raw pointers more annoying to use, WIP

This covers the rest of the cases.  For mAttribBuffers, I just inlined the original macro. This loses the nice [i] on array field names, but I don't know if that matters or not. I'm not sure of any nice way to work around that, as you'll need some kind of autopointers to pass into the array edge name generator, then pass that as chars to the inner function or something...

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Benoit, are you going to upload some new patches or should the current ones be reviewed?

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 677391 [details] [diff] [review]
part 2: generic BEGIN_ macros folding in the NATIVE case


>+template <typename T,
>+         bool IsISupports = mozilla::IsBaseOf<nsISupports, T>::value>
Missing one space before bool

>+struct DowncastGenericPointerImpl
>+{
>+  static T* Run(void *p) {
{ should be in the next line

>+    nsISupports *s = static_cast<nsISupports*>(p);
>+    NS_ASSERTION(NS_CYCLE_COLLECTION_CLASSNAME(T)::CheckForRightISupports(s),
>+                 "not the nsISupports pointer we expect");
>+    return NS_CYCLE_COLLECTION_CLASSNAME(T)::Downcast(s);
>+  }
>+};
>+
>+template <typename T>
>+struct DowncastGenericPointerImpl<T, false>
>+{
>+  static T* Run(void *p) {
ditto

>+    return static_cast<T*>(p);
>+  }
>+};
>+
>+template <typename T>
>+T* DowncastGenericPointer(void *p) {
ditto

Comment 37

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

(In reply to Olli Pettay [:smaug] from comment #35)
> Benoit, are you going to upload some new patches or should the current ones
> be reviewed?

Andrew's comments and patch mean undoing/redoing the Part 1 patch. It seems better to fold all of that into a single 'Part 1' patch than have patches undoing each other. So I am working on that now.

Comment 38

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

Attachment: part 1: implement type-generic CC UNLINK/TRAVERSE macros

The amount of time that this took is probably unreasonable, but that was fun.

This patch replaces both the earlier "part 1" patch and Andrew's "make raw pointers more annoying" patch.

Since ImplCycleCollectionTraverse functions now call each other "recursively" and the specialization for raw pointers is removed (per Andrew's patch), this correctly fails to compile with raw pointers, so I found that accessing the element_type typedef was no longer needed to prevent from accidentally using this on raw pointers. Am I missing something? This is removed in the present patch.

nsCycleCollectionTraversalCallback is now split into its own header so we can #include it and no longer template on the callback type.

The other big part of this patch is the new header, which is now called nsCycleCollectionEdgeName.h, and corresponding .cpp files.

This is about the strings that we pass around in the debug-info case. It is possible that I spent too much time on something unimportant, but looking at the existing code, it seems that quite a bit of attention was being paid to that, e.g. the NoteNextEdgeName stuff, so I thought we might as well get it right.

These files declare/define two new classes:

  CycleCollectionEdgeNameNoDebugInfo
  CycleCollectionEdgeNameWantDebugInfo

whose sole purpose is to handle the strings for passing the field name strings if WANT_DEBUG_INFO.

The key design goal here was to have zero overhead when we don't want debug info, aside from the single if(WantDebugInfo) in the NS_IMPL_CYCLE_COLLECTION_TRAVERSE macro. So actually, with this patch, when not using debug info, traversing should now have less overhead, as for example when we traverse an array of pointers, we now do if(WantDebugInfo) only once of the whole array instead of doing it for each array element.

See the code in NS_IMPL_CYCLE_COLLECTION_TRAVERSE:

#define NS_IMPL_CYCLE_COLLECTION_TRAVERSE(_field)                              \
  if (MOZ_UNLIKELY(cb.WantDebugInfo())) { \
    ImplCycleCollectionTraverse(cb, tmp->_field, \
                                CycleCollectionEdgeNameWantDebugInfo(cb, #_field)); \
  } else {\
    ImplCycleCollectionTraverse(cb, tmp->_field, CycleCollectionEdgeNameNoDebugInfo()); \
  }

So in the non-debug-info case, we construct a CycleCollectionEdgeNameNoDebugInfo which is an _empty_ struct where the methods are _no operations_ and are entirely inlined. So this should have zero cost at all.

In the debug info case, we construct a CycleCollectionEdgeNameWantDebugInfo which, in case of recursive fields, will be appended and copied by value at each recursion step: see WithSuffix. This has overhead, but it only affects the debug-info case.

The ImplCycleCollectionTraverse functions are then templated in the CycleCollectionEdgeNameType so that in the non-debug-info case it is then known at compile time that we are only passing around empty structs and doing no-ops.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Getting quite complicated :/

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

And at least we shouldn't add too many new .h and .cpp files.

Comment 41

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

Is the concern only about the number of files --- in which case it is easy enough to address by folding some files into each other --- or is it about introducing these new CycleCollectionEdgeName* classes?

To help decide on this, let's summarize the benefits of the two CycleCollectionEdgeName* classes:
 - remove the runtime overhead (even in non-debug-info mode) of testing if(WantDebugInfo) for each array element.
 - provide fully accurate debug-info names in debug-info mode.

Comment 42

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

(In reply to Olli Pettay [:smaug] from comment #19)
> But I don't understand why we need anything for nsCOMArray_base
> Why isn't nsCOMArray<T> enough?

I think I forgot to answer this. The reason is that in nsArray (note, no T here) the mArray field is a nsCOMArray_base.

Comment 43

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

By the way, do you have a performance test to check if the removal of the per-array-element conditional branching had any significant impact?

Comment 44

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

Also, I would like instructions to test the WantDebugInfo case please.

Comment 45

[Andrew McCreight [:mccr8]]

Just do a CC dump: https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump

> By the way, do you have a performance test to check if the removal of the per-array-element conditional branching had any significant impact?

No, sorry.

Comment 46

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

Attachment: part 1: implement type-generic CC UNLINK/TRAVERSE macros

Minor updates post checking an actual CC log:
  1. The overwhelming majority of entries were FragmentOrElement, so I ported it so it prints nice array indices.
  2. Fixed format strings (%ul was wrong, I meant %lu, and 'z' is the right modifier for size_t).

Also: replaced CycleCollectionNoteEdgeName and CycleCollectionNoteArrayEdgeName by a single varargs CycleCollectionNoteEdgeName function.

By the way: note that the custom code in FragmentOrElement is still doing per-array-element if(WantDebugInfo), so, as they are the majority, there is no hope that my patch by itself would give a significant performance improvement. One would have to fix this in FragmentOrElement / nsAttrAndChildArray before one would have a chance of seeing the difference.

Comment 47

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

Meh, PR_vsnprintf doesn't know about %zu. Will use %lu. Not uploading a new patch for that.

Comment 48

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

Attachment: part 2: generic BEGIN_ macros folding in the NATIVE case

Comment 49

[Andrew McCreight [:mccr8]]

Comment on attachment 678200 [details] [diff] [review]
part 1: implement type-generic CC UNLINK/TRAVERSE macros

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

Thanks for addressing the raw pointers problem. Sorry for being a pain here.

The varargs in CycleCollectionNoteEdgeName and WithSuffix kind of seem like overkill. They are both used almost entirely for arrays. The single exception for the former is FragmentOrElement, where we can probably survive with a slightly uglier array-ish syntax, and for the latter it is VertexAttributeData, where I don't think it matters too much. It just seems a little dangerous to pass around printf strings that are only going to be checked at runtime.

The way you avoid doing checks in loops is clever, but there is a bit of a tradeoff in that you are going to have a lot of code duplication of those loops. Maybe that doesn't matter.

r- mostly because I think you, Olli and I need to some kind of consensus on this vararg stuff. :)

::: xpcom/glue/nsCycleCollectionEdgeName.cpp
@@ +34,5 @@
> +}
> +
> +void
> +CycleCollectionEdgeNameWantDebugInfo::Init(nsCycleCollectionTraversalCallback& aCallback,
> +                                   const char *aName)

nit: indent so it aligns, move * to the left

@@ +61,5 @@
> +  char buf[bufSize];
> +
> +  va_list a;
> +  va_start(a, aFormat);
> +  PR_vsnprintf(buf, bufSize, aFormat, a);

Instead of defining bufSize, you could do sizeof(buf) here and below. Though I guess that's only right with single byte characters...

@@ +70,5 @@
> +  return copy;
> +}
> +
> +void CycleCollectionNoteEdgeNameImpl(nsCycleCollectionTraversalCallback& aCallback,
> +                                     const char *aFormat,

nit: * on the left

::: xpcom/glue/nsCycleCollectionEdgeName.h
@@ +10,5 @@
> +#include "mozilla/Likely.h"
> +
> +class CycleCollectionEdgeNameWantDebugInfo
> +{
> +  struct Private;

nit: I think the right style here is to move these three declarations to a "private:" section at the end of the class, with the field mPrivate listed at the very end.

@@ +12,5 @@
> +class CycleCollectionEdgeNameWantDebugInfo
> +{
> +  struct Private;
> +
> +  Private *mPrivate;

nit: * should be with the type on the left, here and in a bunch of other places in the file, though the other places are all arguments.

@@ +42,5 @@
> +void CycleCollectionNoteEdgeNameImpl(nsCycleCollectionTraversalCallback& aCallback,
> +                                     const char *aFormat,
> +                                     va_list aArgList);
> +
> +// Must be inlined so that in the no-debug-info case this is just a simple if().

If it must be inlined, perhaps MOZ_ALWAYS_INLINE?  I've seen people say that compilers basically just ignore "inline" and do whatever they want. Depends on how convinced you are this this needs to happen. :)

::: xpcom/glue/nsTObserverArray.h
@@ +388,5 @@
> +{
> +  size_t length = aField.Length();
> +  for (size_t i = 0; i < length; ++i) {
> +    ImplCycleCollectionTraverse(aCallback,
> +                                aField.ElementAt(i),

This can be aField[i] now that I fixed that other bug, if you prefer.

Comment 50

[Andrew McCreight [:mccr8]]

Is the idea in part 2 that you define this  DowncastGenericPointerImpl, where the first case is exactly the downcast stuff we're doing already for nsISupports stuff, and the second case is the downcast stuff we're doing for native stuff? This part of the original code is something I'm a little weaker on, so I want to make sure I'm following it before I dig in more.

Comment 51

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

(In reply to Andrew McCreight [:mccr8] from comment #49)
> Thanks for addressing the raw pointers problem. Sorry for being a pain here.

No problem. While I want to move on to other patches, it is very important to keep such central code as good as possible.

The root problem is I conflated several things together and some were hard to decide on because we lack data (e.g. how much speed do we gain, and what is the code size increase, with the optimization to avoid checks in loops).

I'll try to make a patch that refocuses on the original purpose of this bug and then we can always carry out the other issues in other bugs.

> If it must be inlined, perhaps MOZ_ALWAYS_INLINE?  I've seen people say that
> compilers basically just ignore "inline" and do whatever they want. Depends
> on how convinced you are this this needs to happen. :)

It's not generally true that compilers ignore 'inline', for example in GCC 'inline' has the effect of multiplying by 5 the pseudo-instruction-count threshold at which it will decide to inline. But it's true that sometimes compilers make the wrong decision... so MOZ_ALWAYS_INLINE would be a good idea here.

Comment 52

[Andrew McCreight [:mccr8]]

(In reply to Benoit Jacob [:bjacob] from comment #51)
> No problem. While I want to move on to other patches, it is very important
> to keep such central code as good as possible.
Great, I agree.

> I'll try to make a patch that refocuses on the original purpose of this bug
> and then we can always carry out the other issues in other bugs.
That sounds like a good idea, thanks. It can be very easy with this code to want to try to fix it all at once.

Comment 53

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

Attachment: part 1: implement type-generic CC UNLINK/TRAVERSE macros

This version comes back to a plain CycleCollectionNoteEdgeName function.

The only catch is it takes an optional |aFlags| parameter, with currently only one flag to indicate that the edge name is an array element (and so should be suffixed with [i]).

The reason for this design is that we already know that the right design is to have the ImplCycleCollectionTraverse functions calling each other 'recursively' for nested data structures like nsTArray<nsRefPtr> so we have to provide a means by which the first ImplCycleCollectionTraverse can inform the second one that the edge name is an array element.

Comment 54

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

(In reply to Andrew McCreight [:mccr8] from comment #50)
> Is the idea in part 2 that you define this  DowncastGenericPointerImpl,
> where the first case is exactly the downcast stuff we're doing already for
> nsISupports stuff, and the second case is the downcast stuff we're doing for
> native stuff? This part of the original code is something I'm a little
> weaker on, so I want to make sure I'm following it before I dig in more.

Yes, exactly. We have two cases: nsISupports and native. In both cases we want to "downcast" a pointer. In order to be able to with unified macros i.e. without the separate NATIVE_ macros, we need to be able to do that automatically in C++: decide in which case we are (that is what IsBaseOf<nsISupports, T> does for us) and call the appropriate downcasting function in each case (that is what the C++ idiom here is doing).

I am not sure if this idiom has a name already in C++ lore, but in my previous project before Mozilla (Eigen) we called it a "meta selector". The idiom goes as follows: you want to implement C<T> that will be like A<T> if Condition<T> is true and will be like B<T> otherwise. How do you do that? The idea is of course partial template specialization, but with respect to what parameter? As long as the only template parameter to C is T, it is not easy to see how to do the specialization. The idea is to add a second template parameter to C, and do the partial specialization on it. To make this transparent, this second template parameter takes a default value which is where you pass in Condition<T>. From the caller's perspective, as long as they let it have its default value, this second template parameter is transparent, they don't need to know about it. So the general pattern is:

template<typename T, bool cond = Condition<T>::value>
struct C
  : A<T>
{
  ...
};

template<typename T>
struct C<T, false>
  : B<T>
{
  ...
};

Comment 55

[Andrew McCreight [:mccr8]]

Comment on attachment 678627 [details] [diff] [review]
part 1: implement type-generic CC UNLINK/TRAVERSE macros

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

Looks good, thanks.

::: xpcom/glue/nsCycleCollectionTraversalCallback.h
@@ +10,5 @@
> +
> +class nsCycleCollectionParticipant;
> +
> +/**
> + * Callback definitions

This "Callback definitions" comment can just be deleted.

@@ +13,5 @@
> +/**
> + * Callback definitions
> + */
> +typedef void
> +(* TraceCallback)(void *p, const char* name, void *closure);

|TraceCallback| isn't used anywhere in this file, so I think it can stay in nsCycleCollectionParticipant.h

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 678627 [details] [diff] [review]
part 1: implement type-generic CC UNLINK/TRAVERSE macros

>+ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& aCallback,
>+                            mozilla::WebGLVertexAttribData& aField,
>+                            const char* aName,
>+                            unsigned aFlags)
Make this uint32_t everywhere.

>+++ b/xpcom/glue/nsCycleCollectionTraversalCallback.h
>@@ -0,0 +1,68 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef nsCycleCollectionTraversalCallback_h__
>+#define nsCycleCollectionTraversalCallback_h__
>+
>+#include "nsISupports.h"
>+
>+class nsCycleCollectionParticipant;
>+
>+/**
>+ * Callback definitions
>+ */
>+typedef void
>+(* TraceCallback)(void *p, const char* name, void *closure);
>+
>+class NS_NO_VTABLE nsCycleCollectionTraversalCallback
>+{
>+public:
>+    // You must call DescribeRefCountedNode() with an accurate
>+    // refcount, otherwise cycle collection will fail, and probably crash.
>+    // If the callback cares about objname, it should put
>+    // WANT_DEBUG_INFO in mFlags.
>+    NS_IMETHOD_(void) DescribeRefCountedNode(nsrefcnt refcount,
>+                                             const char* objname) = 0;
New code (even if just moved to a new file) should use 2 space indentation.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 678203 [details] [diff] [review]
part 2: generic BEGIN_ macros folding in the NATIVE case

Since nsCycleCollectionParticipant.h uses odd coding style,
I'm not going to complain about using the same style :)

Comment 58

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

About the part 2 patch, I wanted to ask if you could think of a better name than DowncastGenericPointer which really isn't great. Maybe DowncastXPCOMOrNative() ? To make it clear that what it does is dispatch between these two cases.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Ah yes, DowncastXPCOMOrNative sounds better.

Comment 60

[Andrew McCreight [:mccr8]]

Comment on attachment 678203 [details] [diff] [review]
part 2: generic BEGIN_ macros folding in the NATIVE case

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

Looks good.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +329,5 @@
> +  }
> +};
> +
> +template <typename T>
> +struct DowncastGenericPointerImpl<T, false>

Okay, so this works because the compiler picks the most specific instantiation, and if the second arg is false, the second definition is the most specific, but if the second arg is true, then the first one is? Wacky.

These are methods and not functions because function templates are restricted so that it wouldn't work?

A comment for this case saying it is for native CC participants would be nice. Probably not really needed for the other one, as it is fairly obvious that it is ISupports.

@@ +338,5 @@
> +  }
> +};
> +
> +template <typename T>
> +T* DowncastGenericPointer(void *p)

Changing the name to DowncastXPCOMOrNative or something like DowncastCCParticipant sounds good to me.

@@ +347,5 @@
>  ///////////////////////////////////////////////////////////////////////////////
>  // Helpers for implementing CanSkip methods
>  ///////////////////////////////////////////////////////////////////////////////
>  
>  #define NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(_class)                        \

The nice thing here about these changes is that it will make it easier to support skippability for native classes.

Comment 61

[Andrew McCreight [:mccr8]]

Amazing how much gunk that clears out.

Comment 62

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

(In reply to Andrew McCreight [:mccr8] from comment #60)
> Comment on attachment 678203 [details] [diff] [review]
> part 2: generic BEGIN_ macros folding in the NATIVE case
> 
> Review of attachment 678203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> ::: xpcom/glue/nsCycleCollectionParticipant.h
> @@ +329,5 @@
> > +  }
> > +};
> > +
> > +template <typename T>
> > +struct DowncastGenericPointerImpl<T, false>
> 
> Okay, so this works because the compiler picks the most specific
> instantiation, and if the second arg is false, the second definition is the
> most specific, but if the second arg is true, then the first one is? Wacky.

Well, in this case, it's a bit simpler than that: we have a template with a partial specialization, so the compiler will first see if the partial specialization matches, and if not it will fall back to the default case.

Now, we can rewrite this in such a way that it never falls backs, and would fail to compile if it fell back. Like this, taking notation from comment 54:

template<typename T, bool cond = Condition<T>::value>
struct C
{
 // empty struct so that calling methods on it will fail to compile
};

template<typename T>
struct C<T, true>
  : A<T>
{
  ...
};

template<typename T>
struct C<T, false>
  : B<T>
{
  ...
};


Do you like this better like this?

> 
> These are methods and not functions because function templates are
> restricted so that it wouldn't work?

Yes: function templates cannot be partially specialized, not even in C++11.

They also can't have default template argument values in C++98, but that part is fixed in C++11.

> 
> A comment for this case saying it is for native CC participants would be
> nice. Probably not really needed for the other one, as it is fairly obvious
> that it is ISupports.

OK

> Changing the name to DowncastXPCOMOrNative or something like
> DowncastCCParticipant sounds good to me.

OK

Comment 63

[Andrew McCreight [:mccr8]]

> Do you like this better like this?
Whatever you prefer is fine with me.

Comment 64

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

What do we want to do with NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR and other related macros?

At this point it's not hard to unify them with the rest, but IIUC they take raw pointers so we don't want to make it easy to do by mistake. Do we want to fold them into the RAWPTR macro?

Comment 65

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

Hm in fact I don't understand what NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR is about?

Comment 66

[Andrew McCreight [:mccr8]]

"Native" in this context means non-nsISupports. For nsISupports classes, we can get their participant via a hacky QI, but for non-nsISupports we must explicitly pass along the participant, which is mainly what distinguishes them.

Generally speaking, "native" pointers should be ref counted.

Although, looking over the uses, I see that this isn't always the case:
  http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsBindingManager.cpp#1721
  http://mxr.mozilla.org/mozilla-central/source/dom/telephony/CallEvent.cpp#33
  probably some more

...but those really should be turned into some kind of RAW_PTR macro!

Some of them might be fixable in other ways.  For instance, the CallEvent thing could get the same treatment as that weird WebGL texture thing you had to add a custom thing for. On the other hand, they look to only be used once, so maybe it isn't worth the effort.

Comment 67

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

So for example, this:

  http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/NotificationController.cpp#71

should use the non-NATIVE macros because the field being declared here,

 http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/NotificationController.h#264

is actually inheriting nsISupports?

I think I'll leave the NATIVE_ macros untouched as they seem to be used in few cases and legitimately used in even fewer cases, and in the legitimate use cases, if I understand comment 66 correctly, we can't unify them with the non-NATIVE macros anyway as the second argument giving the CC participant can't be automatically guessed.

Comment 68

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

Ah wait, the CC participant seems to be automatically determined from the field type by NS_CYCLE_COLLECTION_PARTICIPANT -- so we actually could unify the NATIVE_ macros, if I understand correctly.

Comment 69

[Andrew McCreight [:mccr8]]

> is actually inheriting nsISupports?

Yeah, that sounds right to me. It is just a normal ISupports CC class.

(In reply to Benoit Jacob [:bjacob] from comment #68)
> Ah wait, the CC participant seems to be automatically determined from the
> field type by NS_CYCLE_COLLECTION_PARTICIPANT -- so we actually could unify
> the NATIVE_ macros, if I understand correctly.

That is correct. (Which means it doesn't work with inheritance, unfortunately...)

Comment 70

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

Attachment: part 1 for landing

Comment 71

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

Attachment: part 2 for landing

Comment 72

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

(In reply to Andrew McCreight [:mccr8] from comment #69)
> > is actually inheriting nsISupports?
> 
> Yeah, that sounds right to me. It is just a normal ISupports CC class.

Hah, so here is the reason why that code was declaring that array of isupports as an array of native objects:

We don't currently have a macro to declare an array of _AMBIGUOUS isupports.

That code was just working around that by declaring it as an array of natives.

So I'll conflate that patch with the one removing _AMBIGUOUS macros by using ToSupports functions.

Comment 73

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

Attachment: part 3: fold the TRAVERSE_NATIVE_ case

This removes the need to have separate macros to declare native pointers/arrays to traverse.

This adds more stuff to nsCycleCollectionEdgeName.h which becomes ill-named. OK to rename it to nsCycleCollectionHelpers.h in a subsequent patch? Can you propose a better name? It contains whatever needs to be included by code that has to declare new generic refpointer/array classes to the CC.

The main addition is a function, CycleCollectionNoteParticipant, that takes a pointer and passes it to either NoteXPCOMChild or NoteNativeChild depending on whether it is a nsISupports.

Another change is that ToSupports moves from qsObjectHelper.h to nsISupportsImpl.h so that it can be used anywhere in XPCOM. I planned to do that only for the next patch (removing _AMBIGUOUS macros) but was forced to do it now for the reason below (*).

The rest of this patch is just local fixes to port code that was using the NATIVE_ macros to their new, slightly changed behavior:
 - The old NATIVE_ macros were not prepending tmp->## to the _field name, contrary to the other TRAVERSE macros.
 - (*) The old NATIVE_ macros were sometimes used (like in DocAccessible.cpp) to declare arrays of pointers that had nsISupports* as an ambiguous base. Indeed, there was no TRAVERSE macro for that case --- there was only TRAVERSE_NSCOMPTR_AMBIGUOUS but it didn't handle arrays. So the new implementation of these macros has to allow for ambiguous bases, and we do that by calling ToSupports in CycleCollectionNoteParticipant. We then have to add ToSupports overloads for each CC class having nsISupports as an ambiguous base.

Comment 74

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

Attachment: part 4 WIP: fold the _AMBIGUOUS case

WIP, doesn't fully compile yet: more ToSupports overloads are needed.

Do you agree with this approach?

ToSupports is already the way to deal with ambiguous nsISupports in the new DOM bindings. The part 3 patch promotes it to The One Way to deal with that everywhere. The good thing is that for each class that has nsISupports as an ambiguous base, we'll just have to write this ToSupports overload once and then we can use it for all sorts of things.

Comment 75

[Andrew McCreight [:mccr8]]

Comment on attachment 679514 [details] [diff] [review]
part 3: fold the TRAVERSE_NATIVE_ case

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

My feedback comments suggest some fairly tedious changes, so I took the liberty of throwing together patch that addresses all of them. (You'll have to update your tree, as I fixed up a new audio class that has landed since you wrote your patch.)

This was also a useful exercise to figure out why you did some things a certain way, as I tried to do it differently, and then I saw why...

::: content/media/webaudio/AudioContext.cpp
@@ +31,5 @@
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mWindow)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDestination)
>    // Cannot use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR since AudioListener
>    // does not inherit from nsISupports.
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(mListener, AudioListener, "listener")

It seems like that while you are touching this line anyways, you might as well change this to NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mListener), because the last two args are being dropped. Similarly in DelayNode, GainNode, PannerNode. Also in DynamicsCompressorNode which has landed since you wrote this patch.

::: content/xbl/src/nsXBLBinding.cpp
@@ +426,5 @@
>  
>    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mInsertionPointTable key");
>    cb.NoteXPCOMChild(aKey);
>    if (aData) {
> +    ImplCycleCollectionTraverse(cb, *aData, "mInsertionPointTable value", 0);

The 0 is a little unfortunate here. Can you add 0 as a default argument for nsTArray? Probably not worth bothering for the other cases...

::: content/xul/content/src/nsXULElement.cpp
@@ +1851,5 @@
>              }
>          }
>          for (i = 0; i < elem->mChildren.Length(); ++i) {
> +            CycleCollectionNoteEdgeName(cb, "mChildren[i]");
> +            CycleCollectionNoteParticipant(cb, elem->mChildren[i].get());

The type of mChildren is nsTArray<nsRefPtr<nsXULPrototypeNode> >, so can you just turn this whole loop into a |ImplCycleCollectionTraverse(cb, elem->mChildren, "mChildren", 0);|  (or without the 0 if you add that as a default argument!) kind of thing?

::: xpcom/glue/nsCycleCollectionEdgeName.h
@@ +52,5 @@
> +struct CycleCollectionNoteParticipantImpl<T, true>
> +{
> +  static void Run(nsCycleCollectionTraversalCallback& aCallback, T* p)
> +  {
> +    aCallback.NoteXPCOMChild(ToSupports(p));

I don't really understand how this ToSupports thing works, but it sounds like a standard mechanism you are just generalizing, so I'll trust that you are doing the right thing. :)

@@ +67,5 @@
> +};
> +
> +template <typename T>
> +inline void CycleCollectionNoteParticipant
> +  (nsCycleCollectionTraversalCallback& aCallback, T* p)

I think it makes sense to add const char* and uint32_t arguments to this function, then add a call to CycleCollectionEdgeName, as every use of this function is prefaced with a call to CycleCollectionEdgeName. Then you could change this file name to nsCycleCollectionNoteParticipant.h. Eventually, we'll probably be able to eliminate all of the legacy uses of NOTE_EDGE_NAME, and inline CycleCollectionEdgeName into this function.

Have a default value for the flags argument, like with NoteEdgeName.

Then you can update nsContentUtils, nsBindingManager, nsXBLPrototypeBinding, nsMaybeWeakPtr, nsAutoPtr, nsCOMArray, nsCOMPtr.

@@ +71,5 @@
> +  (nsCycleCollectionTraversalCallback& aCallback, T* p)
> +{
> +  CycleCollectionNoteParticipantImpl<T>::Run(aCallback, p);
> +}
> +

Also, I noticed this file says tab-width:4 at the top, but is only using a tab-width of 2. I don't know what is right (or care a huge amount...), but they should at least be made consistent or it will be really annoying to edit this file.

I also noticed that you don't use the right indentation in a few files where you add some code.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +479,5 @@
>  
>  #define NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR(_field)                       \
>    PR_BEGIN_MACRO                                                               \
>      NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, #_field);                           \
> +    CycleCollectionNoteParticipant(cb, tmp->_field);                           \

If you roll NoteEdge into NoteParticipant, this can be simplified.

Comment 76

[Andrew McCreight [:mccr8]]

Attachment: part 3 feedback in patch form

Comment 77

[Andrew McCreight [:mccr8]]

Comment on attachment 679526 [details] [diff] [review]
part 4 WIP: fold the _AMBIGUOUS case

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

I need to think some more about this, as multiple inheritance is not my strong suit, but if it is okay with Olli it is fine with me...

::: dom/base/nsGlobalWindow.h
@@ +1170,5 @@
>  
> +inline nsISupports*
> +ToSupports(nsGlobalWindow* p)
> +{
> +    return static_cast<nsPIDOMWindow*>(p);

This is generating the canonical nsISupports pointer, not the canonical cycle collector participanting pointer. Is that okay?  (Above it has  NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS_AMBIGUOUS(nsGlobalWindow, nsIScriptGlobalObject)).

Comment 78

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #77)
> This is generating the canonical nsISupports pointer, not the canonical
> cycle collector participanting pointer. Is that okay?  (Above it has 
> NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS_AMBIGUOUS(nsGlobalWind
> ow, nsIScriptGlobalObject)).

For instance from nsGlobalWindow.cpp:
566   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mWindow,
567                                                        nsIScriptGlobalObject)

...but if I understand correctly, your ToSupports function is going to act more like 

NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mWindow, nsPIDOMWindow)?

Comment 79

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

Oops. I didn't pay any attention to that. I just picked a random base class inheriting nsISupports, randomly. Will fix.

Comment 80

[Andrew McCreight [:mccr8]]

Is there any way we can roll that into the NS_DECL_CYCLE_COLLECTION_*_AMBIGUOUS macros? I'm guessing no, because it is within a class, but it would be convenient. Though I'm not sure those declarations are at precisely the same points we need for ToSupports.

Comment 81

[Andrew McCreight [:mccr8]]

Maybe the DECL_AMBIGUOUS(type1, type2) macro could do something like
  typedef type2 MyMagicParent;

and then you could define a templated ToSupports like:

template<T>
ToSupports(T *x)
{
  return static_cast<T::MyMagicParent*>(x);
}

...or something like that.

Comment 82

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

Comment 81 is clever and would work as long as only one field in a given class, is declared _AMBIGUOUS.

But:
 - By keeping the need to use a _AMBIGUOUS macro we would give up on the original goal of this bug which was to have generic enough CC macros that most users could use the helper macros such as NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2.
 - In DOM bindings, people have already started adding custom ToSupports overloads by hand rather than abstracting that behind a macro, and I think that's a good idea, for the usual reason: it allows the developer to understand what's going on. It's not awful enough, not difficult enough to understand, not bug-prone enough to warrant hiding behind a macro.

Comment 83

[Andrew McCreight [:mccr8]]

I meant for the NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS macros, not the NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS macros. The MagicParent would indicate the parent class. If we don't hide the ToSupports stuff behind a macro (which I agree is not really great), then it would be good to somehow ensure that the NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS class agrees with ToSupports.

Comment 84

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 679514 [details] [diff] [review]
part 3: fold the TRAVERSE_NATIVE_ case

2 space indentation, please.

>+inline nsISupports*
>+ToSupports(DocAccessible *p)
* goes with type

Comment 85

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 679526 [details] [diff] [review]
part 4 WIP: fold the _AMBIGUOUS case


>+inline nsISupports*
>+ToSupports(DOMRequest* p)
>+{
>+  return static_cast<nsIDOMDOMRequest*>(p);
  return static_cast<nsIDOMEventTarget*>(p);




>+inline nsISupports*
>+ToSupports(nsDOMDeviceStorage* p)
>+{
>+  return static_cast<nsIDOMDeviceStorage*>(p);
I think ditto here too



>+inline nsISupports*
>+ToSupports(nsDOMDeviceStorageCursor* p)
>+{
>+  return static_cast<nsIDOMDeviceStorageCursor*>(p);
>+}
I think here too.
We want to return the right nsISupports


>+inline nsISupports*
>+ToSupports(ArchiveReader* p)
>+{
>+    return static_cast<nsIDOMArchiveReader*>(p);
>+}
And here...

Should probably add some generic check for all the ToSupports that the right
object is returned.

>+inline nsISupports*
>+ToSupports(FileHandle* p)
>+{
>+    return static_cast<nsIDOMFileHandle*>(p);
>+}
This is wrong too, I think

Comment 86

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

(In reply to Andrew McCreight [:mccr8] from comment #83)
> I meant for the NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS macros, not the
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS macros. The MagicParent
> would indicate the parent class. If we don't hide the ToSupports stuff
> behind a macro (which I agree is not really great), then it would be good to
> somehow ensure that the NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS class
> agrees with ToSupports.

Ah, OK. I agree with doing that check.

Regarding the proposal in comment 81, another issue is that it would require switching ToSupports to being a single function template that people specialize for their classes, instead of being a collection of unrelated overloaded functions, as it currently is.

Comment 87

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

(In reply to Andrew McCreight [:mccr8] from comment #75)
> Comment on attachment 679514 [details] [diff] [review]
> part 3: fold the TRAVERSE_NATIVE_ case
> 
> Review of attachment 679514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My feedback comments suggest some fairly tedious changes, so I took the
> liberty of throwing together patch that addresses all of them.

Thanks!

> > +    ImplCycleCollectionTraverse(cb, *aData, "mInsertionPointTable value", 0);
> 
> The 0 is a little unfortunate here. Can you add 0 as a default argument for
> nsTArray? Probably not worth bothering for the other cases...

I would like it to be done for all cases, so there is no discrepancy. Otherwise people copying code, or writing more ImplCCTraverse functions, are going to be confused.

> 
> ::: xpcom/glue/nsCycleCollectionEdgeName.h
> @@ +52,5 @@
> > +struct CycleCollectionNoteParticipantImpl<T, true>
> > +{
> > +  static void Run(nsCycleCollectionTraversalCallback& aCallback, T* p)
> > +  {
> > +    aCallback.NoteXPCOMChild(ToSupports(p));
> 
> I don't really understand how this ToSupports thing works, but it sounds
> like a standard mechanism you are just generalizing, so I'll trust that you
> are doing the right thing. :)

See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

> 
> @@ +67,5 @@
> > +};
> > +
> > +template <typename T>
> > +inline void CycleCollectionNoteParticipant
> > +  (nsCycleCollectionTraversalCallback& aCallback, T* p)
> 
> I think it makes sense to add const char* and uint32_t arguments to this
> function, then add a call to CycleCollectionEdgeName, as every use of this
> function is prefaced with a call to CycleCollectionEdgeName. Then you could
> change this file name to nsCycleCollectionNoteParticipant.h. Eventually,
> we'll probably be able to eliminate all of the legacy uses of
> NOTE_EDGE_NAME, and inline CycleCollectionEdgeName into this function.

Good idea!

> @@ +71,5 @@
> > +  (nsCycleCollectionTraversalCallback& aCallback, T* p)
> > +{
> > +  CycleCollectionNoteParticipantImpl<T>::Run(aCallback, p);
> > +}
> > +
> 
> Also, I noticed this file says tab-width:4 at the top, but is only using a
> tab-width of 2. I don't know what is right (or care a huge amount...), but
> they should at least be made consistent or it will be really annoying to
> edit this file.

I think 2 is the new standard, so we should fix the comment.

> 
> I also noticed that you don't use the right indentation in a few files where
> you add some code.

Does your patch fix that?

Comment 88

[Olli Pettay [:smaug][[e-mail]]]

2 space indentation has been in the Gecko coding style as long as I remember...so I'd guess at least
from year 2003. For reasons not clear to me some code use 4 spaces.

Comment 89

[Andrew McCreight [:mccr8]]

> I would like it to be done for all cases, so there is no discrepancy.
Sounds good, I just didn't want to make you do too many extra changes, but you are right, it would be weird to be inconsistent.

> Does your patch fix that?
Yes, I fixed the ones I noticed.

Comment 90

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

Sorry for the long delay; I'll post an update ASAP. Basically Andrew's idea in comment 77 does allow to do something that really works.

Comment 91

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

I meant comment 81.

Comment 92

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

Attachment: part 3: fold the TRAVERSE_NATIVE and AMBIGUOUS cases

This patch also folds Andrew's "part 3 feedback" patch --- I thought it made more sense to fold it in, but it does rob you from due credit. Hope it's OK!

The new idea in this patch basically comes from comment 81 plus 2 observations:
 - The CC innerclass already has a method, Upcast(), that does exactly the job we want to do here.
 - We actually can have a templated ToSupports that calls it, _alongside_ the existing ToSupports overloads. No need to intrude into the basic ToSupports design and make it only a single template with specializations.

Initially I tried to just directly call Upcast(). However that failed when we tried to traverse a field that was a nsISupports, but was not a CC class. I suppose that that is a typical case when we have a field like nsCOMPtr<nsISomeBaseClass> and we expect that the pointee will be some derived type at runtime.

That's where the current non-templated ToSupports(nsISupports*) function works really well: it automatically takes care of all types that inherit nsISupports as a non-ambiguous base. Doing this with more templates would add quite a bit of complexity (another level of template partial specialization and SFINAE).

So instead, as long as that's possible, it's better to keep the existing ToSuports as-is, non-templated, and add another overload of it that is templated and will be used for the cases of CC classes, where we can call Upcast().

See the new code in nsCycleCollectionEdgeName.h (that file is still in need of a renaming):

template <typename T>
nsISupports* ToSupports(T* p, typename T::NS_CYCLE_COLLECTION_INNERCLASS* dummy = 0)
{
  return T::NS_CYCLE_COLLECTION_INNERCLASS::Upcast(p);
}

This works by SFINAE. This template will match if a class is a CC class, that's what the dummy parameter is there for. That is the classical "enable if" idiom, except that here we didn't need to add a dummy typedef to the class, we could use NS_CYCLE_COLLECTION_INNERCLASS.

As long as T is not the same type as nsISupports, my understanding is that this overload will always be preferred to the default ToSupports(nsISupports*). But I have to say that I don't really know the rules there. We could ask a C++ expert (waldo, rafael) if needed.

Comment 93

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

Note that the new part 3 also does what was initially proposed in part 4: fold the AMBIGUOUS case.

Comment 94

[Andrew McCreight [:mccr8]]

Comment on attachment 680469 [details] [diff] [review]
part 3: fold the TRAVERSE_NATIVE and AMBIGUOUS cases

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

Looks reasonable, though it would probably be good for some template expert to vouch for the general approach.

::: content/media/webaudio/BiquadFilterNode.cpp
@@ +16,5 @@
>    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mQ)
>    NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mGain)
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(BiquadFilterNode, AudioNode)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(mFrequency, AudioParam, "frequency value")

These three can shortened to drop unused args. New audio classes are really being added rapidly!

::: xpcom/glue/nsCycleCollectionEdgeName.h
@@ +76,5 @@
> +};
> +
> +template <typename T>
> +inline void CycleCollectionNoteParticipant(nsCycleCollectionTraversalCallback& aCallback,
> +                                           T* p,

The p here and elsewhere in this file don't have a proper aName.  aParticipant or something?

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ -262,4 @@
>  #define NS_CYCLE_COLLECTION_CLASSNAME(_class)                                  \
>          _class::NS_CYCLE_COLLECTION_INNERCLASS
>  
>  #define NS_CYCLE_COLLECTION_INNERNAME                                          \

You can also remove _INNERNAME and _PARTICIPANT from this file.

::: xpcom/glue/nsISupportsImpl.h
@@ +39,5 @@
> +    return p;
> +}
> +
> +inline nsISupports*
> +ToCanonicalSupports(nsISupports* p)

Should this be left in the other file?

Comment 95

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

Attachment: part 1 for landing

Renamed header to nsCycleCollectionHelper.h. Hope you like it.

Comment 96

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

Attachment: part 2 for landing

Comment 97

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

Attachment: part 3: fold the TRAVERSE_NATIVE and AMBIGUOUS cases

This should address feedback comments; ToCanonicalSupports is still there because I thought that it made sense to keep it alongside ToSupports, but this isn't an enlightened opinion.

Comment 98

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

Waldo: you're CC'd on this bug for this reason:

(In reply to Andrew McCreight [:mccr8] from comment #94)
> Looks reasonable, though it would probably be good for some template expert
> to vouch for the general approach.

So the template stuff there is the ToSupports functions. This used to be a non-templated overloaded function, with the default implementation being

inline nsISupports*
ToSupports(nsISupports* p)
{
    return p;
}

and some overloads being provided for specific classes e.g.

inline nsISupports*
ToSupports(Foo* p)
{
    return static_cast<SomeBaseOfFoo*>(p);
}


What these patches change is that the part 3 patch introduces a template overload, using SFINAE to apply only to CC classes:

template <typename T>
nsISupports* ToSupports(T* p, typename T::NS_CYCLE_COLLECTION_INNERCLASS* dummy = 0)
{
  return T::NS_CYCLE_COLLECTION_INNERCLASS::Upcast(p);
}

I think that our main question is: are there cases where a call to ToSupports would be undefined by the C++ standard i.e. it would be undefined which overload is chosen by the compiler, and no compilation error would be generated?

Comment 99

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

Attachment: part 3: fold the TRAVERSE_NATIVE and AMBIGUOUS cases

Comment 100

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

Attachment: part 4: update CC helper macros

This adapts the helper CC macros to use the new UNLINK/TRAVERSE macros (does not actually change anything as the old macros use the new ones anyway) and extends the WRAPPERCACHE macros to support up to 10 fields, which is what I need to port the content/canvas code.

This patch preserves the existing down-to-earth implementation of the CC helper macros. The advantage is it's easy to find out what they are doing. The inconvenient is the number of lines is quadratic in the max number of fields that they support. If you want I have a local patch that is linear, but makes it less obvious to look up what these macros are doing.

Comment 101

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

Attachment: part 5: sample porting: content/canvas

This is where it starts to pay back ;-)

Comment 102

[Andrew McCreight [:mccr8]]

Comment on attachment 680960 [details] [diff] [review]
part 5: sample porting: content/canvas

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

nice

Comment 103

[Jeff Walden [:Waldo]]

(In reply to Benoit Jacob [:bjacob] from comment #98)
> I think that our main question is: are there cases where a call to
> ToSupports would be undefined by the C++ standard i.e. it would be undefined
> which overload is chosen by the compiler, and no compilation error would be
> generated?

I wouldn't claim to be a template expert, exactly.  :-)

But if overload resolution finds two overloads that are equally viable for a function call, the function call's ill-formed.  Since non-template functions are preferred to template functions during overload resolution, I don't think you can ever have two equally-viable overloads in play here, so I believe it's always defined which overload you'll get.  And exact-match argument types are preferred during overload to argument types related by a pointer conversion, so if you're passing a D*, that's a better match for the D* in ToSupports(D*) (picked by template type deduction) than for the nsISupports* in the non-template ToSupports(nsISupports*).

Luke backs me up on this IRL, so that's double-confirmation, too.  :-)

Comment 104

[Jeff Walden [:Waldo]]

so if you're passing a D* and if your D has a D::NS_CYCLE_COLLECTION_INNERCLASS type in it, of course.  :-)

Comment 105

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 680960 [details] [diff] [review]
part 5: sample porting: content/canvas

Nice

Comment 106

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 680958 [details] [diff] [review]
part 3: fold the TRAVERSE_NATIVE and AMBIGUOUS cases

(Ugh, nsCOMArray uses odd coding style)


In general we need some document for NS_IMPL_CYCLE_COLLECTION_TRAVERSE.
Templates make the implementation of macros almost unreadable, so
need to tell to macro users what kind of values can be passed.

Comment 107

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

(In reply to Olli Pettay [:smaug] from comment #106)
> Comment on attachment 680958 [details] [diff] [review]
> part 3: fold the TRAVERSE_NATIVE and AMBIGUOUS cases
> 
> (Ugh, nsCOMArray uses odd coding style)
> 
> 
> In general we need some document for NS_IMPL_CYCLE_COLLECTION_TRAVERSE.
> Templates make the implementation of macros almost unreadable, so
> need to tell to macro users what kind of values can be passed.

We definitely need to document it. We should also add comments to the code so that it doesn't leave that "unreadable" impression --- I think it doesn't have to, actually the macro is just a call to a single templated function now, so we should add a comment there explaining what it is and where it is implemented (in refpointer/array classes).

Comment 108

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

Comment on attachment 680959 [details] [diff] [review]
part 4: update CC helper macros

This patch is boring enough, that it doesn't need two reviews.

Comment 109

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

Big try push that will get me to the top of http://people.mozilla.org/~catlee/highscores/highscores.html :

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

Comment 110

[Andrew McCreight [:mccr8]]

Comment on attachment 680959 [details] [diff] [review]
part 4: update CC helper macros

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

Too late I already reviewed it!

::: dom/base/nsWrapperCache.h
@@ +255,5 @@
>      NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS            \
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END                         \
>    NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(_class)
>  
>  #define NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2(_class, _field1,\

Maybe just go ahead and put _field1 here and in later macros on their own line, given how many lines you are using anyways?  It is fine to just leave it alone if you'd like.

Comment 111

[Andrew McCreight [:mccr8]]

Comment on attachment 680958 [details] [diff] [review]
part 3: fold the TRAVERSE_NATIVE and AMBIGUOUS cases

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

This looks fantastic!

As before, I will upload a patch that addresses my own review comments.

Something has been bothering me for a while about CycleCollectionNoteParticipant, but I wasn't sure what. But today I realized that what was bothering me was that the name isn't really very descriptive. I think CycleCollectionNoteChild is better, because it isn't an arbitrary participant, it is a participant that is a child of another object, and we're really just passing things along to different Note*Child functions, so that will make it more clear what is happening. Does that sound reasonable to you? Sorry, I know it is a bit obnoxious to make suggestions like this late in the process.

It sounds like if later nsHTMLCSSStyleSheet is made into a CC class, it won't use the right ToSupports. But given that there's only a single instance of this, I'd be okay with just putting a comment near the NS_DECL_ISUPPORTS of nsHTMLCSSStyleSheet saying that if it ever becomes a CC class, to delete its ToSupports function at the end of the file. It doesn't seem worth the time to set up some infrastructure to check for it.

> Renamed header to nsCycleCollectionHelper.h. Hope you like it.

Unfortunately, I don't think that's a great name, as it is fairly generic. There's already a file named nsCycleCollectionUtils.h, which is basically a synonym. Maybe nsCycleCollectionNoteChild.h? What this file is really providing is a generic implementation of NoteChild. Is that okay with you?

::: xpcom/glue/nsCycleCollectionHelper.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

c-basic-offset should be 2 also, as apparently that's what my Emacs mode is using to decide indentation.

@@ +59,5 @@
> +
> +template <typename T>
> +struct CycleCollectionNoteParticipantImpl<T, true>
> +{
> +  static void Run(nsCycleCollectionTraversalCallback& aCallback, T* aParticipant)

Contrary to what was probably my prior suggestion, I think all the |aParticipant| should be |aChild|.

Comment 112

[Andrew McCreight [:mccr8]]

Attachment: address my own review comments for part 3, not sure if it builds yet

Comment 113

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

(In reply to Andrew McCreight [:mccr8] from comment #111)
> Comment on attachment 680958 [details] [diff] [review]
> part 3: fold the TRAVERSE_NATIVE and AMBIGUOUS cases
> 
> Review of attachment 680958 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fantastic!
> 
> As before, I will upload a patch that addresses my own review comments.
> 
> Something has been bothering me for a while about
> CycleCollectionNoteParticipant, but I wasn't sure what. But today I realized
> that what was bothering me was that the name isn't really very descriptive.
> I think CycleCollectionNoteChild is better, because it isn't an arbitrary
> participant, it is a participant that is a child of another object, and
> we're really just passing things along to different Note*Child functions, so
> that will make it more clear what is happening. Does that sound reasonable
> to you? Sorry, I know it is a bit obnoxious to make suggestions like this
> late in the process.

No problem at all, I didn't understand before that explanation what Child meant. Making the change was not too hard either:

bjacob:/hack/mozilla-central$ find .hg/patches/ -type f | xargs sed -i 's|nsCycleCollectionHelper|nsCycleCollectionNoteChild|g'
bjacob:/hack/mozilla-central$ find .hg/patches/ -type f | xargs sed -i 's|nsCycleCollectionNoteParticipant|nsCycleCollectionNoteChild|g'


> 
> It sounds like if later nsHTMLCSSStyleSheet is made into a CC class, it
> won't use the right ToSupports. But given that there's only a single
> instance of this, I'd be okay with just putting a comment near the
> NS_DECL_ISUPPORTS of nsHTMLCSSStyleSheet saying that if it ever becomes a CC
> class, to delete its ToSupports function at the end of the file. It doesn't
> seem worth the time to set up some infrastructure to check for it.

OK, will add a comment.

> 
> > Renamed header to nsCycleCollectionHelper.h. Hope you like it.
> 
> Unfortunately, I don't think that's a great name, as it is fairly generic.
> There's already a file named nsCycleCollectionUtils.h, which is basically a
> synonym. Maybe nsCycleCollectionNoteChild.h? What this file is really
> providing is a generic implementation of NoteChild. Is that okay with you?

Sure is OK. See above sed commands. I wanted to fix this in the patch files directly, rather than adding another patch renaming this file at the end, to limit the churn around here.

> Contrary to what was probably my prior suggestion, I think all the
> |aParticipant| should be |aChild|.

OK, sure.

Comment 114

[Andrew McCreight [:mccr8]]

Thanks. You have to fix up the indentation a few places for the nsCycleCollectionNoteParticipant change.

Comment 115

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

Attachment: part 1 for landing

Comment 116

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

Attachment: part 2 for landing

Comment 117

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

Attachment: part 3 for landing

Comment 118

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

(In reply to Benoit Jacob [:bjacob] from comment #113)
> > 
> > It sounds like if later nsHTMLCSSStyleSheet is made into a CC class, it
> > won't use the right ToSupports. But given that there's only a single
> > instance of this, I'd be okay with just putting a comment near the
> > NS_DECL_ISUPPORTS of nsHTMLCSSStyleSheet saying that if it ever becomes a CC
> > class, to delete its ToSupports function at the end of the file. It doesn't
> > seem worth the time to set up some infrastructure to check for it.
> 
> OK, will add a comment.
> 

Per bug 807437 comment 20, I understand that this is no longer a concern. Correct me if you would still like a comment there.

Comment 119

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

Attachment: make an attempt at comments

(In reply to Olli Pettay [:smaug] from comment #106)
> In general we need some document for NS_IMPL_CYCLE_COLLECTION_TRAVERSE.
> Templates make the implementation of macros almost unreadable, so
> need to tell to macro users what kind of values can be passed.

Does this help? It's nothing that couldn't be easily gathered from MXR/DXR.

Comment 120

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/e11ac3ecc685
http://hg.mozilla.org/integration/mozilla-inbound/rev/3d02d5638373
http://hg.mozilla.org/integration/mozilla-inbound/rev/17f8e44632a6
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d4b8369a198
http://hg.mozilla.org/integration/mozilla-inbound/rev/ed94525f5823

Comment 121

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

Bustage fix, as in my enthusiasm I had forgotten about bug 811926:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7468f7af19d5

Commit message:

Bug 806279 - fix bustage due to unusual CC macro usage in Telephony code, see bug 811926 - no review, bustage

This fix consists in taking the old definition of the TRAVERSE_NATIVE_PTR macro and expanding it inline,
as the new macros can't handle that unusual case (see bug 811926).

Because it is expaning inline the old macro, it shouldn't make any difference.

Comment 122

[Peter Van der Beken [:peterv]]

(In reply to Benoit Jacob [:bjacob] from comment #120)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/ed94525f5823

This seems to have changed the line-endings of CanvasRenderingContext2D.cpp to CRLF?

Comment 123

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

Oops. It seems that this file had mixed line endings before and what may possibly have happened is my editor got confused by that.

Comment 124

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

Fixed: http://hg.mozilla.org/integration/mozilla-inbound/rev/c6188b322a68

Comment 125

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 681873 [details] [diff] [review]
make an attempt at comments

I actually would like to have a list of supported cases, so that
in common cases one doesn't need to guess whether something works.

Something like:
Currently this can handle unlinking of at least the following kinds of fields.
nsCOMPtr<nsISupports> (or interfaces inheriting nsISupports),
nsTArray<nsCOMPtr<nsISupports>> ,
nsRefPtr<T> where T is cycle collectable native

Comment 126

[[e-mail]]

Comment on attachment 681592 [details] [diff] [review]
part 1 for landing

>diff --git a/xpcom/glue/nsCycleCollectionParticipant.cpp b/xpcom/glue/nsCycleCollectionParticipant.cpp
>--- a/xpcom/glue/nsCycleCollectionParticipant.cpp
>+++ b/xpcom/glue/nsCycleCollectionParticipant.cpp
>@@ -1,16 +1,22 @@
> /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "nsCycleCollectionParticipant.h"
> #include "nsCOMPtr.h"
> 
>+#ifdef MOZILLA_INTERNAL_API
>+#include "nsString.h"
>+#else
>+#include "nsStringAPI.h"
>+#endif
>+
This is what nsStringGlue.h is for.

Comment 127

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e11ac3ecc685
https://hg.mozilla.org/mozilla-central/rev/3d02d5638373
https://hg.mozilla.org/mozilla-central/rev/17f8e44632a6
https://hg.mozilla.org/mozilla-central/rev/2d4b8369a198
https://hg.mozilla.org/mozilla-central/rev/ed94525f5823
https://hg.mozilla.org/mozilla-central/rev/7468f7af19d5
https://hg.mozilla.org/mozilla-central/rev/c6188b322a68

Comment 128

[Andrew McCreight [:mccr8]]

Hurray!

Comment 129

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

I looked at the existing CC documentation,

https://developer.mozilla.org/en-US/docs/Interfacing_with_the_XPCOM_cycle_collector

and I don't feel competent to edit it. I hope that someone else can do it, and also, I hope wish that it could be expanded a little (the current documentation doesn't mention things like INTERFACE_MAP, or the _INHERITED stuff, or the _WRAPPERCACHE stuff, or the helper macros like NS_IMPL_CYCLE_COLLECTION_3... it would be very useful too to give more complete examples, showing a full class, etc).

Comment 130

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

Hm, half of what I wrote in comment 129 is wrong. But that supports my point that I shouldn't be writing this documentation.

Comment 131

[Andrew McCreight [:mccr8]]

Yeah, I need to work on updating the documentation. I can use your dev.platform post as grist for the mill.

Comment 132

[Andrew McCreight [:mccr8]]

I updated the docs a bit to account for this bug at least, though I mostly just say "ITS MAGIC!".

Comment 133

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

I think the first problem of that page is that it's hard to simultaneously address the needs of people who don't particularly want to know more about CC stuff and just want their code to work ASAP, and the needs of people who want to understand more in detail. Maybe have 2 separate pages, or at least separate sections?