810687 - nsDocument tries to TRAVERSE a nsHTMLCSSStyleSheet which is not a CC class

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

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

In nsDocument.cpp we have:

NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mStyleAttrStyleSheet, nsIStyleSheet)

Here mStyleAttrStyleSheet is declared in nsDocument.h as:

nsRefPtr<nsHTMLCSSStyleSheet> mStyleAttrStyleSheet;

nsHTMLCSSStyleSheet is not a CC class, it's just a plain nsISupports.

So my understanding is that this TRAVERSE does nothing and we could have a leak if a cycle was created by this reference.

Comment 1

[Andrew McCreight [:mccr8]]

These are the data fields of nsHTMLCSSStyleSheet:

74   nsCOMPtr<nsIURI> mURL;
75   nsIDocument*     mDocument;
76   nsDataHashtable<nsStringHashKey, MiscContainer*> mCachedStyleAttrs;

URIs are acyclic, mDocument is not an owning reference.

I'm not sure what is going on with MiscContainer, but at first glance it doesn't appear to have an owning reference to what it holds, though the container itself has some kind of reference count, sometimes.

Comment 2

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

OK, removing [MemShrink]. Still making a patch.

Comment 3

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

Attachment: don't traverse this pointer

This makes us no longer traverse that pointer, based on comment 1.

Let me know if you rather think that we should keep traversing it and declare this class to the CC.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 680466 [details] [diff] [review]
don't traverse this pointer

Somebody who knows more about this class should do the review. There was a similar instance in bug 810103, but bz wanted to leave the useless Traverse around to make sure it would get picked up when the class got converted to being a CC class.

Comment 5

[Andrew McCreight [:mccr8]]

Is this blocking your CC refactoring work because this weird case isn't handled properly? Or is it just something odd you noticed while working on it?

Comment 6

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

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Is this blocking your CC refactoring work because this weird case isn't
> handled properly? Or is it just something odd you noticed while working on
> it?

This is blocking it in the sense that I came across this because of a compilation error: with my patches on bug 806279, specifically part 3, we start relying on the assumption that if a class is the target type of a TRAVERSE, and that class has nsISupports as an ambiguous base, then we rely on it being a CC class (so that we can use the Upcast() method in its CC innerclass to implement ToSupports).

Comment 7

[Andrew McCreight [:mccr8]]

If bz is concerned about forgetting to add this back in once it is converted, then one solution would be to (dynamically?) check in debug builds that mStyleAttrStyleSheet does not QI to nsWrapperCache (or I guess more directly, to the CC participant class). That will start failing when it gets converted to a CCable class, without causing problems for you now.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Right.  So as I understood, traversing things that are not CCed yet preemptively is OK.  It'll just do nothing.  Why is not traversing preferable?

Comment 9

[Andrew McCreight [:mccr8]]

Benoit's patches in bug 806279 make TRAVERSE macros generic, so you don't have to pick NSCOMPTR vs RAWPTR vs whatever, but in the case of an nsISupports class with an ambiguous base it relies on cycle collector infrastructure (the Upcast function in the CC support class) to decide how to upcast. So any nsISupports class that is not actually a CC class will fail to compile when you try to TRAVERSE it.

Comment 10

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

Chatted IRL with Boris, I think I understand better the misunderstanding now.

Boris' viewpoint IIUC, which I now agree with, is that when one writes a new class and declares it to the CC, we should just TRAVERSE all its fields without worrying about their specific types and whether they are currently CC types. If they aren't, the TRAVERSE will be a no-op and that's fine.

Before that conversation I thought that was very dangerous; but now I agree that that's a good approach because it means that when a class is changed in a way that makes it susceptible of creating cycles and we make it a cycle collected class, we don't have to go over all its existing users and start TRAVERSEing their fields of that type.

So the right way to handle this case, that will fix the compilation error I was seeing in bug 806279, is to provide a ToSupports overload for this class.

In short:
 - if a class has nsISupports as a non-ambiguous base, then the default ToSupports function will work for it.
 - otherwise, if a class is a CC class, the ToSupports overload in the part 3 patch in bug 806279 will work for it.
 - otherwise, i.e. if a class has nsISupports as an ambiguous base and is not a CC class, one has to provide a ToSupports overload for it. In the current mozilla code, the present class is the only example.

Comment 11

[Andrew McCreight [:mccr8]]

That sounds reasonable. What happens when you later convert the class to a CC class? Will there be a compilation error? Will it still use the custom defined ToSupports?

Comment 12

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

(In reply to Andrew McCreight [:mccr8] from comment #11)
> That sounds reasonable. What happens when you later convert the class to a
> CC class? Will there be a compilation error? Will it still use the custom
> defined ToSupports?

I believe it will be a compilation error (call to ToSupports is ambiguous). If you think that it would be unacceptable that there would be a risk that it would still silently choose the custom defined ToSupports, we can check.

Comment 13

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

One question about that rule of thumb, "don't worry, just traverse all your fields". Does it have a nonzero runtime cost to traverse a field that is a non-CC type (so traversing it is a no-op)? E.g. if it's an array type, do we still run a for loop over all array elements and call a NoteXPCOMChild function for each array element, only to have that function turn out to be a no-op? I believe that's what's happening now, and if that's correct, that makes it not too simple to tell people to not worry. In that case we should file a bug and fix it as soon as bug 806279 lands. It would block the desirable goal of updating CC documentation telling people how to declare their classes to the CC.

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Benoit Jacob [:bjacob] from comment #12)
> I believe it will be a compilation error (call to ToSupports is ambiguous).
> If you think that it would be unacceptable that there would be a risk that
> it would still silently choose the custom defined ToSupports, we can check.

My main concern here would be that it gets converted, then somehow the base class of choice changes (probably not super likely?), and the ToSupports isn't updated, and then we're picking the wrong one and end up incorrectly converting. I'm not sure how bad that would be (maybe we'd end up hitting asserts anyways?).

(In reply to Benoit Jacob [:bjacob] from comment #13)
> One question about that rule of thumb, "don't worry, just traverse all your
> fields". Does it have a nonzero runtime cost to traverse a field that is a
> non-CC type (so traversing it is a no-op)? E.g. if it's an array type, do we
> still run a for loop over all array elements and call a NoteXPCOMChild
> function for each array element, only to have that function turn out to be a
> no-op? I believe that's what's happening now, and if that's correct, that
> makes it not too simple to tell people to not worry. In that case we should
> file a bug and fix it as soon as bug 806279 lands. It would block the
> desirable goal of updating CC documentation telling people how to declare
> their classes to the CC.

Right, what happens is that we NoteXPCOMChild anything in the traverse (like every array element in the case of an array...), then it gets QId to the CC participant class, which returns null, then the CC ignores the null.

Comment 15

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

(In reply to Andrew McCreight [:mccr8] from comment #14)
> (In reply to Benoit Jacob [:bjacob] from comment #13)
> > One question about that rule of thumb, "don't worry, just traverse all your
> > fields". Does it have a nonzero runtime cost to traverse a field that is a
> > non-CC type (so traversing it is a no-op)? E.g. if it's an array type, do we
> > still run a for loop over all array elements and call a NoteXPCOMChild
> > function for each array element, only to have that function turn out to be a
> > no-op? I believe that's what's happening now, and if that's correct, that
> > makes it not too simple to tell people to not worry. In that case we should
> > file a bug and fix it as soon as bug 806279 lands. It would block the
> > desirable goal of updating CC documentation telling people how to declare
> > their classes to the CC.
> 
> Right, what happens is that we NoteXPCOMChild anything in the traverse (like
> every array element in the case of an array...), then it gets QId to the CC
> participant class, which returns null, then the CC ignores the null.

So wait, since we need to QI each field (in the case of an array, each array element) before we can even determine whether NoteXPCOMChild will be a no-operation for it, then does that mean that we can't do anything to make this faster in the case where it is a no-operation?

In that case, do we still want to tell people to traverse all their fields regardless of whether they are CC classes or not?

Comment 16

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

Filed bug 811213 about that.