Closed
Bug 480956
Opened 15 years ago
Closed 15 years ago
HTMLDocument and some other dom objects traverse member variables even if the document is in "nsCCUncollectableMarker::InGeneration"
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
Details
(Keywords: fixed1.9.1, perf)
Attachments
(1 file, 1 obsolete file)
9.68 KB,
patch
|
jst
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
I think we should fix this for 1.9.1...
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → Olli.Pettay
Assignee | ||
Comment 2•15 years ago
|
||
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
Attachment #364951 -
Flags: superreview?(peterv)
Attachment #364951 -
Flags: review?(peterv)
Comment 3•15 years ago
|
||
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.
Attachment #364951 -
Flags: superreview?(peterv)
Attachment #364951 -
Flags: superreview+
Attachment #364951 -
Flags: review?(peterv)
Attachment #364951 -
Flags: review+
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.
Assignee | ||
Comment 5•15 years ago
|
||
Well, it is *not* an error, so returning an error code would be wrong. But sure I could change it.
Assignee | ||
Comment 6•15 years ago
|
||
Attachment #364951 -
Attachment is obsolete: true
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.
Assignee | ||
Comment 8•15 years ago
|
||
I pushed the patch http://hg.mozilla.org/mozilla-central/rev/4c683cf90ba1
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 9•15 years ago
|
||
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.
Attachment #364996 -
Flags: approval1.9.1+
Updated•15 years ago
|
Flags: blocking1.9.1? → blocking1.9.1-
Assignee | ||
Comment 10•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c8c6ed1b96a7
Keywords: fixed1.9.1
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•