Last Comment Bug 678528 - Consolidate node CC code
: Consolidate node CC code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks: 648801
  Show dependency treegraph
 
Reported: 2011-08-12 09:36 PDT by Peter Van der Beken [:peterv]
Modified: 2011-08-28 13:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (25.00 KB, patch)
2011-08-12 09:36 PDT, Peter Van der Beken [:peterv]
jst: review+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2011-08-12 09:36:18 PDT
Created attachment 552676 [details] [diff] [review]
v1

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.
Comment 1 Andrew McCreight [:mccr8] 2011-08-12 09:45:15 PDT
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.
Comment 2 Peter Van der Beken [:peterv] 2011-08-16 06:58:14 PDT
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 Andrew McCreight [:mccr8] 2011-08-16 09:06:36 PDT
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 Andrew McCreight [:mccr8] 2011-08-16 09:24:48 PDT
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.
Comment 5 Peter Van der Beken [:peterv] 2011-08-17 06:24:40 PDT
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 Andrew McCreight [:mccr8] 2011-08-17 09:34:18 PDT
(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.
Comment 7 Peter Van der Beken [:peterv] 2011-08-20 06:28:17 PDT
(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.
Comment 8 Ed Morley [:emorley] 2011-08-28 13:29:18 PDT
http://hg.mozilla.org/mozilla-central/rev/500c2ddb52c1

Note You need to log in before you can comment on or make changes to this bug.