Closed
Bug 678528
Opened 13 years ago
Closed 13 years ago
Consolidate node CC code
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: peterv, Assigned: peterv)
References
Details
Attachments
(1 file)
25.00 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
Right now we need to remember to add any members of nsINode that should be traversed/traced/unlinked to the CC participant of all the implementations of nsINode. I'd like to add nsINode::Traverse/Trace/Unlink to share that code, and same for slots.
Attachment #552676 -
Flags: review?(jst)
Comment 1•13 years ago
|
||
Is this the first case of an abstract class having a Traverse/Unlink? Dealing with these adds a little wrinkle to my static analysis. Nothing too bad, but yeah I could see how it might be annoying to remember all of the time.
Assignee | ||
Comment 2•13 years ago
|
||
I'm not sure I understand what you mean, the Traverse/Unlink methods I add are just helper functions that are called from the participants for nsGenericElement/nsDocument/... We certainly have helper functions already.
Comment 3•13 years ago
|
||
Ah, I see what you mean. At first glance, I thought you were defining cycle collector inner classes, but you are just adding a helper method to explicitly call in the children.
Comment 4•13 years ago
|
||
Does this sound like the right rule for this change? In a class C, if all classes that are superclasses of C and subclasses of nsINode are abstract, then the traverse of C should invoke nsINode::Traverse (and similarly for Unlink and Trace). I can implement this in my static analysis. That will certainly be better than looking for all of the various bits and pieces that are needed. Right now, I only look for a Traverse/Unlink of mNodeInfo, I think.
Assignee | ||
Comment 5•13 years ago
|
||
Sure, that'll work. Although I guess it's ok if there's a class between nsINode and C that has members that C's participant notes itself. Also, would it miss the problem if nsINode::Traverse didn't note all the necessary members from nsINode? Is it hard to have the analysis just add everything that's noted from functions called from C's participant::Traverse as if it's directly noted from C's participant::Traverse?
Comment 6•13 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #5) > Sure, that'll work. Although I guess it's ok if there's a class between > nsINode and C that has members that C's participant notes itself. It is okay for C, but if the in-between class is concrete, then it would need to note those members itself, right? What I do currently is assume that a participant's parents note all of the relevant fields of the parent. > Also, > would it miss the problem if nsINode::Traverse didn't note all the necessary > members from nsINode? Is it hard to have the analysis just add everything > that's noted from functions called from C's participant::Traverse as if it's > directly noted from C's participant::Traverse? The analysis is done on a per-cpp-file basis. nsINode::Traverse is defined in nsGenericElement.cpp file, so it won't be visible to the analysis in any other file. What I am planning to do is to handle this in the same way as I handle any participant's parent Traverse (though of course this isn't actually a participant Traverse): locally (in nsGenericElement.cpp in this case), I check that nsINode::Traverse visits all relevant fields. On any use of nsINode::Traverse, I assume that the Traverse visits all relevant fields of nsINode. Thus, if nsINode::Traverse fails to visit a field, then the analysis will produce an error in nsGenericElement.cpp, but not anywhere else.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #6) > It is okay for C, but if the in-between class is concrete, then it would > need to note those members itself, right? What I do currently is assume > that a participant's parents note all of the relevant fields of the parent. Hmm, I see what you mean. I think we might have cases where the class is concrete but only ever used as a base class. Doing the traversal from the derived classes' participants avoids some overhead in that case. > What I am planning to do is to handle this in the same way as I handle any > participant's parent Traverse (though of course this isn't actually a > participant Traverse): locally (in nsGenericElement.cpp in this case), I > check that nsINode::Traverse visits all relevant fields. Ok. Unfortunate, but probably the only sensible way if it's per-cpp-file.
Updated•13 years ago
|
Attachment #552676 -
Flags: review?(jst) → review+
Comment 8•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/500c2ddb52c1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Target Milestone: --- → mozilla9
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
•