TRAVERSE_NATIVE_PTR used on a nsISupports* pointer in TelephonyCall and CallEvent

RESOLVED FIXED in mozilla19

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla19
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 681723 [details] [diff] [review]
fix CC macros in Telephony.cpp

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.
Attachment #681723 - Flags: review?(continuation)
(Assignee)

Comment 1

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

Comment 2

5 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=eaf2b5c01eeb
(Assignee)

Comment 3

5 years ago
mTelephony is a nsRefPtr<Telephony>. So why not just traverse it as such?
(Assignee)

Comment 4

5 years ago
Created attachment 681728 [details] [diff] [review]
preferred patch: traverse mTelephony as the nsRefPtr that it is. use helper macro to simplify.

I prefer this patch, unless there is a very specific reason to traverse a raw pointer here.
Attachment #681728 - Flags: review?(continuation)
(Assignee)

Comment 5

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b00e8a25c880
Attachment #681723 - Attachment is patch: true
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.
Summary: TRAVERSE_NATIVE_PTR used on a nsISupports* pointer in Telephony.cpp → TRAVERSE_NATIVE_PTR used on a nsISupports* pointer in TelephonyCall and CallEvent
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.
(Assignee)

Comment 8

5 years ago
(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?
Attachment #681723 - Flags: review?(continuation)
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
Created attachment 681760 [details] [diff] [review]
fix these 2 cases
Attachment #681723 - Attachment is obsolete: true
Attachment #681728 - Attachment is obsolete: true
Attachment #681728 - Flags: review?(continuation)
Attachment #681760 - Flags: review?(continuation)
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...
Attachment #681760 - Flags: review?(continuation) → review?(bugs)
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.
Attachment #681760 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1368272550da
QA Contact: bjacob
Target Milestone: --- → mozilla19
Assignee: nobody → bjacob
QA Contact: bjacob
https://hg.mozilla.org/mozilla-central/rev/1368272550da
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.