655089 - nsXPConnect::ToParticipant calls js_GetGCThingTraceKind twice

Status: RESOLVED FIXED

Description

[Andrew McCreight [:mccr8]]

It looks to me that in nsXPConnect.cpp, nsXPConnect::ToParticipant calls js_GetGCThingTraceKind twice due to macro'age:

420 #define ADD_TO_CC(_kind)    ((_kind) == JSTRACE_OBJECT || (_kind) == JSTRACE_XML)
[...]
533 nsCycleCollectionParticipant *
534 nsXPConnect::ToParticipant(void *p)
535 {
536     if (!ADD_TO_CC(js_GetGCThingTraceKind(p)))
537         return NULL;
538     return this;
539 }

Not a huge deal as this function has no side effects, but it work that doesn't need to be done.

This can be fixed by creating a new variable and assigning the result of the function to it, or by turning ADD_TO_CC into an inline function called addToCCKind or something.  The latter would be a bit more civilized, if we can rely on the inlining.  I used the former fix in my patch for Bug 650519.

Comment 1

[Andrew McCreight [:mccr8]]

Created attachment 535223 [details] [diff] [review]
turn CC_KIND into a call

Comment 2

[Andrew McCreight [:mccr8]]

I ran this on try, and it looks okay, except mobile looks kind of bad.  One of the Maemo builds is burning, which I will retry, and the Android tests are still running, 8 or so hours later.

Comment 3

[Andrew McCreight [:mccr8]]

Created attachment 538561 [details] [diff] [review]
change CC_KIND macro to a function to avoid double call

fixed bitrot

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 538561 [details] [diff] [review]
change CC_KIND macro to a function to avoid double call

Passed a try run.

Comment 5

[Blake Kaplan (:mrbkap)]

Comment on attachment 538561 [details] [diff] [review]
change CC_KIND macro to a function to avoid double call

Good catch.

Comment 6

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

Comment on attachment 538561 [details] [diff] [review]
change CC_KIND macro to a function to avoid double call

Landed on inbound.

Comment 7

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

http://hg.mozilla.org/mozilla-central/rev/0321e3569921