Telephony.cpp has this code: NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(tmp->mTelephony->ToISupports(), Telephony, "mTelephony") Here ToISupports() returns nsISupports*. So my understanding is this should use _RAWPTR. Correct? The current code is probably a memory leak, right? If this can actually create cycles. This patch relies on the patches in bug 807437 and 806279. The refactoring of CC macros in bug 806279 breaks the build on B2G as it forbids accidentally traversing a raw pointer without using the _RAWPTR macro. Prior to these patches it was possible to abuse TRAVERSE_NATIVE_PTR to traverse raw nsISupports* pointers which is what the code here was doing. The present bug is sort of proof that the old CC macros were indeed error prone and too hard not only to write, but also to review.

...also, any thoughts on whether it will still be a memory leak with _RAWPTR? Why are we passing a raw pointer here?

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

mTelephony is a nsRefPtr<Telephony>. So why not just traverse it as such?

I prefer this patch, unless there is a very specific reason to traverse a raw pointer here.

This is pretty bizarre (CallEvent does the same thing) but I'm inclined to believe there's some reason bent went out of his way to do things this way, because bent is a cycle collector ninja. This should be okay, as long as Telephony has no subclasses with their own CC participants. We take a ref counted pointer, basically do a DownCast() on it, then tell the cycle collector exactly which participant to use on it. As long as Telephony has no subclasses with participants, this is the participant we'd get anyways. I'm not really sure what's going on here. My best guess is that it looks like NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS is being reinvented. Maybe the QI of Telephony is weird so we can't rely on the usual CC canonicalization? I'll try to figure it out. It looks like Ben is on PTO this week.

The QI of Telephony looks normal to me. My guess is that the existing code does the same as this: NS_IMPL_CYCLE_COLLECTION_TRAVERSE_AMBIGUOUS(mTelephony, nsDOMEventTargetHelper) Thus with your new macros it would just become NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTelephony) You could try that. I haven't found anything yet in the discussion of bug 674726 that explains what is going on here.

(In reply to Andrew McCreight [:mccr8] from comment #7) > Thus with your new macros it would just become > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTelephony) > You could try that. This is what the second patch here does. Is it OK to land? That's the only thing blocking the whole CC macros refactoring from landing at this point.

> This is what the second patch here does. Ah, so it does. > Is it OK to land? I'd like to look at it a little more here, given that we don't have real TBPL coverage of B2G... Doesn't CallEvent.cpp have the same problem?

Olli, do you have any idea what is going on with these NATIVE_PTR traverses?

(In reply to Andrew McCreight [:mccr8] from comment #9) > Doesn't CallEvent.cpp have the same problem? It does! I had just looked only at the error I got on TBPL. I'll make build locally to see all remaining issues.

Meh, CC patches dont apply cleanly to the gecko directory in the B2G repo. However this seemed to be the last occurence. https://tbpl.mozilla.org/?tree=Try&rev=04402f9e40cf

Comment on attachment 681760 [details] [diff] [review] fix these 2 cases Maybe Olli is more confident in understanding what this code is supposed to be doing than me...

Someone with b2g build environment should fix bug 777271. That would take care of CallEvent.

In my enthusiasm to land bug 806279, I forgot about this bug. As a quick bustage fix, I replaced the TRAVERSE_NATIVE_PTR macros in this file by their expansions, so this should not make any difference. I left the original form as comments for reference. https://hg.mozilla.org/integration/mozilla-inbound/rev/7468f7af19d5

Comment on attachment 681760 [details] [diff] [review] fix these 2 cases Uh, what is the old code trying to do.

Hey guys, sorry that was complicated. I looked over it and I think that comment 7 is right. I think I just forgot about the _AMBIGUOUS macro.