Closed Bug 875409 Opened 7 years ago Closed 7 years ago

Replace nsINode::Trace() with nsWrapperCache::TraceWrapper()


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

Not set





(Reporter: mccr8, Assigned: FishLikeHexagon)




(1 file, 2 obsolete files)

nsINode::Trace() just does wrapper cache tracing, so let's just get rid of it.  I don't think it is very likely we'll add extra JS pointers to all nodes.
Assignee: nobody → FishLikeHexagon
Bug 874691 is going to change this stuff around, so you'll probably want to wait until that lands before doing anything.  Let me know if you have any questions.
Oh I see. Sounds good! I'll keep in touch.
Olli, does it sound reasonable to get rid of nsINode::Trace() in this way?

Zach, with nsINode::Trace() replaced by nsWrapperCache::TraceWrapper(), some additional simplifications can be made.  I was thinking of something like this:

1. nsINode::Trace(a, b, c) becomes a->TraceWrapper(b, c)

2. tmp->TraceWrapper(aCallbacks, aClosure) becomes NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER

3. This:


If you don't want to do 2 or 3, that's okay, I can just file a followup bug for them.

You don't need to look for any places to do 2 and 3 besides those where you are doing 1 anyways.  This patch should give you a list of all of the places 1 can be done:  You can also use MXR ( ) to find places, though it only updates once a day.
Flags: needinfo?(bugs)
Sounds ok
Flags: needinfo?(bugs)
Thanks for this info, maybe I should have waited a bit before working lol =) I already have a patch ready and submitted to the try server but it might not be fully what we want..I am submitting it here anyway so you can tell me if it's on the right track or not. All I did was replace calls of nsINode::Trace() with nsWrapperCache::TraceWrapper() though.
Attachment #754515 - Flags: review?(continuation)
Attachment #754515 - Attachment is patch: true
Attachment #754515 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 754515 [details] [diff] [review]
Replaces nsINode::Trace() with nsWrapperCache::TraceWrapper()

Review of attachment 754515 [details] [diff] [review]:

Rather than having this:
  nsWrapperCache::TraceWrapper(aCallbacks, aClosure);
You need to do this:
  tmp->TraceWrapper(aCallbacks, aClosure);
The set up is a little weird, because it is hidden behind macros, but NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsFoo) ends up defining a local variable |tmp| with type |nsFoo*|, and you want to invoke TraceWrapper on the object |tmp|.  nsINode::Trace is a static method (I'm not sure why) but TraceWrapper isn't.

You should also delete the method nsINode::Trace from nsINode.h and nsINode.cpp, because the idea is that we'll just use TraceWrapper from now on.  If the browser compiles once you've done that, then you'll know that you've gotten rid of all of the uses of nsINode::Trace.

You shouldn't need to include nsWrapperCache.h.

Also, just a few comments about your try run ( ).  It is good to make sure that your patch compiles and runs for at least a little bit of basic browsing before pushing to try, if only to get more immediate feedback.  Also, it is good to push to a single platform first, like Linux64, so you'll use less resources if a patch ends up having some problems across multiple platforms.  The try chooser will show you what platforms currently have the shortest waits.  Also, "debug only" is better to pick than "opt only", because debug builds run many extra checks that can detect problems.  It is rare that problems with opt runs don't show up in debug runs, but it is common for problems in debug runs to not show up in opt runs.

Thanks for the patch.  Please address the above comments and upload a new patch when you get a chance.  Let me know if you have any questions about any of this.
Attachment #754515 - Flags: review?(continuation)
If I wanted to implement #2 and #3 as outlined in your earlier comment, what file do macros like these usually go into?

Thanks for all your help with this so far =)
Oh, sorry, those macros are already defined:
So you would just have to use them.  nsWrapperCache.h should already be included any place you'd want to use these things.
Maybe I'm missing something..but in that file NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER expands into a statement that calls TraceWrapper from the nsContentUtils class. Is this an error? I couldn't find TraceWrapper anywhere in nsContentUtils.h (I don't think it's being inherited from anything, nsContentUtils doesn't extend any class)

I'm guessing since you said: "tmp->TraceWrapper(aCallbacks, aClosure) becomes NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER" we'll need to redfine this macro? If we do this then we can use it in the manner you described.
MXR only updates once a day, so it doesn't include bug 874691.  Look at the definition in your local copy of the file.  This is the danger of working on cutting edge code. :)

That patch made this change:
   23.41 -  nsContentUtils::TraceWrapper(tmp, aCallback, aClosure);
   23.42 +  tmp->TraceWrapper(aCallbacks, aClosure);
I see! I thought I had checked the local copy but I was looking in the wrong spot, I looked at nsContentUtils to see if it had a new TraceWrapper function but I should have looked in nsWrapperCache..okay things make sense now.

I'm uploading a new patch, I wasn't able to shorten the call to this function in nsDocument.cpp because it does some stuff before calling tmp->TraceWrapper(). I'm building it locally now to make sure it compiles and then I'll try it out on the try server.
Attachment #754515 - Attachment is obsolete: true
Attachment #754562 - Flags: review?(continuation)
Comment on attachment 754562 [details] [diff] [review]
Replaces nsINode::Trace() with nsWrapperCache::TraceWrapper()

FYI, you should check the box that says "patch" when uploading a patch. :)
Attachment #754562 - Attachment is patch: true
Attachment #754562 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 754562 [details] [diff] [review]
Replaces nsINode::Trace() with nsWrapperCache::TraceWrapper()

Review of attachment 754562 [details] [diff] [review]:

::: content/base/src/nsDocument.cpp
@@ +1802,5 @@
>    tmp->mCustomPrototypes.Enumerate(CustomPrototypeTrace, &customPrototypeArgs);
>    if (tmp->PreservingWrapper()) {
>    }
> +  tmp->TraceWrapper(aCallbacks, aClosure);

You should be able to replace this line with NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER.

Otherwise, looking much better!  r? me again once you've fixed that and verified locally that it compiles, though I think it should be okay.
Attachment #754562 - Flags: review?(continuation)
Attachment #754562 - Attachment is obsolete: true
Attachment #754612 - Flags: review?(continuation)
This code built locally on my box (windows 7 64 bit), pushed to try for linux 64 bit.
Comment on attachment 754612 [details] [diff] [review]
Replaces nsINode::Trace() with nsWrapperCache::TraceWrapper()

Review of attachment 754612 [details] [diff] [review]:

Looks good, thanks!

Your try run didn't actually run tests, so I'll do a try run just in case and then land your patch.
Attachment #754612 - Flags: review?(continuation) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.