480956 - HTMLDocument and some other dom objects traverse member variables even if the document is in "nsCCUncollectableMarker::InGeneration"

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

HTMLDocument (and others) use NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED 
which first calls the Traverse of the super class. nsDocument does have
nsCCUncollectableMarker::InGeneration(tmp->GetMarkedCCGeneration()) so it returns
early, but the Traverse of HTMLDocument and others don't return early.

It would be good to have some macro for this. Perhaps nsDocument::Traverse
could return non-NS_OK success code if the document shouldn't be
traversed and then the macro could check that and return early.

Same for nsGenericElement and nsGenericDOMDataNode.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I think we should fix this for 1.9.1...

Comment 2

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

Attachment: patch

Here I don't care too much about the success code value. It just need to be
something else than NS_OK. Based on mxr search, 
NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_XPCOM, 2) isn't used for anything else.

I did run mochitest/chrome/browser-chrome without leaks or new assertions

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 364951 [details] [diff] [review]
patch

>diff --git a/xpcom/glue/nsCycleCollectionParticipant.h b/xpcom/glue/nsCycleCollectionParticipant.h

>+#define NS_SUCCESS_INTERRUPTED_TRAVERSE NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_XPCOM, 2)

Long line.
I know there's almost no documentation in this file, but please add some here to explain that a base class' CC participant should return this from Traverse if it wants derived classes to not traverse anything from their CC participant.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could we please make this a error code instead of a success code? And teach the cycle collector about it? Custom success codes are evil and error prone (fails == NS_OK tests). They also won't work if we ever switch to using exceptions.

Comment 5

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

Well, it is *not* an error, so returning an error code would be wrong.
But sure I could change it.

Comment 6

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

Attachment: with peterv's comments

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I would prefer an error yeah, especially given that you technically should do:

rv = NS_CYCLE_COLLECTION_CLASSNAME(_base_class)::Traverse(s, cb);
if (NS_FAILED(rv) || rv == NS_SUCCESS_INTERRUPTED_TRAVERSE) {
  return rv;
}


But i'm not religious about it either.

Comment 8

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

I pushed the patch http://hg.mozilla.org/mozilla-central/rev/4c683cf90ba1

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 364996 [details] [diff] [review]
with peterv's comments

I can't convince myself that this is a blocker, but I'd be fine with taking this patch for 1.9.1.

Comment 10

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c8c6ed1b96a7