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

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Zach Easterbrook]

Oh I see. Sounds good! I'll keep in touch.

Comment 3

[Andrew McCreight [:mccr8]]

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:
NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsFoo)
  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
NS_IMPL_CYCLE_COLLECTION_TRACE_END

becomes:
NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(nsFoo)

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: https://hg.mozilla.org/mozilla-central/rev/76321fce71e7  You can also use MXR ( http://mxr.mozilla.org/mozilla-central/ident?i=&filter= ) to find places, though it only updates once a day.

Comment 4

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

Sounds ok

Comment 5

[Zach Easterbrook]

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.

Comment 6

[Zach Easterbrook]

Attachment: Replaces nsINode::Trace() with nsWrapperCache::TraceWrapper()

Comment 7

[Andrew McCreight [:mccr8]]

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 ( https://tbpl.mozilla.org/?tree=Try&rev=c58dd8997b3c ).  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. http://trychooser.pub.build.mozilla.org/  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.

Comment 8

[Zach Easterbrook]

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 =)

Comment 9

[Andrew McCreight [:mccr8]]

Oh, sorry, those macros are already defined:
  http://mxr.mozilla.org/mozilla-central/source/dom/base/nsWrapperCache.h#238
So you would just have to use them.  nsWrapperCache.h should already be included any place you'd want to use these things.

Comment 10

[Zach Easterbrook]

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.

Comment 11

[Andrew McCreight [:mccr8]]

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.40  #define NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER \
   23.41 -  nsContentUtils::TraceWrapper(tmp, aCallback, aClosure);
   23.42 +  tmp->TraceWrapper(aCallbacks, aClosure);

Comment 12

[Zach Easterbrook]

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.

Comment 13

[Zach Easterbrook]

Attachment: Replaces nsINode::Trace() with nsWrapperCache::TraceWrapper()

Comment 14

[Andrew McCreight [:mccr8]]

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. :)

Comment 15

[Andrew McCreight [:mccr8]]

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()) {
>      NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mExpandoAndGeneration.expando);
>    }
> +  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.

Comment 16

[Zach Easterbrook]

Attachment: Replaces nsINode::Trace() with nsWrapperCache::TraceWrapper()

Comment 17

[Zach Easterbrook]

This code built locally on my box (windows 7 64 bit), pushed to try for linux 64 bit.

https://tbpl.mozilla.org/?tree=Try&rev=eedcec3f2f76

Comment 18

[Andrew McCreight [:mccr8]]

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.

Comment 19

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=aeb58a18f1c9

Comment 20

[Andrew McCreight [:mccr8]]

Thanks for your patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/c41885d5aa3d

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c41885d5aa3d