Closed Bug 452834 Opened 12 years ago Closed 11 years ago

nsGeolocation should be added to cycle collection

Categories

(Core :: DOM: Geolocation, defect, P2, major)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dougt, Assigned: dougt)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 2 obsolete files)

In bug 452762, it was suggested that nsGeolocation should use the CC.
Assignee: doug.turner → nobody
Component: DOM → Geolocation
QA Contact: general → geolocation
jst, how important is this?
I'd say pretty important, w/o this we could be leaking every window and everything in it if a page is able to create cycles through the geolocation code (which doesn't seem like it'd be that hard).
Severity: normal → major
Attached patch wip patch. (obsolete) — Splinter Review
not really sure what to do about unlinking raw pointers, or what to do about tarray's of nsrefptr's.


any advice would be appreciated.
Assignee: nobody → doug.turner
Unlinking (strong) raw pointers is ok, but traversing weak pointers is not! It will cause crashes, so don't traverse mOwner if it's a weak pointer.

We don't have macros for traversing nsTArray<nsRefPtr<...> >.

You only need to add things to cycle collection if they can cause cycles, nsDOMGeoPositionError doesn't hold anything that can hold something else. No need to traverse timers, they're not cycle collected. Same for URIs. I wouldn't traverse mProvider unless we're fairly sure it'll cause cycles (so maybe don't need to add CC to nsGeolocationService).
(In reply to comment #4)
> It will cause crashes

Actually, it won't cause crashes but it will turn off cycle collection so we'll leak the world.
This seems pretty bad; it's pretty easy to leak the world given the number of geolocation APIs that accept function callbacks, and a simple change to the mochitest harness in bug 466104 caused us to start leaking the world due to the existing geolocation mochitests.  I think it should block 1.9.1.
Blocks: 466104
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9.1?
will take a look tonight/tomorrow.
Status: NEW → ASSIGNED
Attachment #347365 - Attachment is obsolete: true
fwiw, i do not claim that this is in any way proper uml. ;-)
Attached patch patch v.2 (obsolete) — Splinter Review
Comment on attachment 357008 [details] [diff] [review]
patch v.2

dbaron, can you take a look at this?

I am pretty sure that we do not need to add the nsGeolocationService to cc, but could be wrong.
Attachment #357008 - Flags: review?(dbaron)
(In reply to comment #11)
> I am pretty sure that we do not need to add the nsGeolocationService to cc, but
> could be wrong.

You're correct; adding it is not needed, since it lives the lifetime of the app anyway.  (Adding the traversal/unlinking of mService that you have in your patch is equally unneeded, for the same reason.)

(I'm not guaranteeing that there aren't problems with mService owning mLastPosition; I haven't looked enough to know.  But adding the service to cycle collection wouldn't help with any such problems.)

I'll look at the rest in a bit...
Comment on attachment 357008 [details] [diff] [review]
patch v.2

> NS_INTERFACE_MAP_BEGIN(nsGeolocationRequest)
>+  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(nsGeolocationRequest)

You should write these two lines as:

NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsGeolocationRequest)

>+// things we need to worry about:
>+// the mCallback, mErrorCallback, and mOptions which are all from JS
>+// the back pointer mLocator is weak, so we will ignore it

Not sure if you meant to leave this comment in; if you do, it should probably be clear that you *must* not handle mLocator.

> NS_INTERFACE_MAP_BEGIN(nsGeolocation)
>+  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(nsGeolocation)

Ditto comment above.

>+// things that we have to worry about:
>+//  the nsCOMPtr mService
>+//  the nsTArray of nsRefPtr's mPendingCallbacks
>+//  the nsTArray of nsRefPtr's mWatchingCallbacks

Probably no need for this, since it says exactly what the code says.

>+ NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mService)

You can drop this; it's not needed.

>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mService, nsIGeolocationUpdate)

Same for this.

>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_MEMBER(mPendingCallbacks, nsGeolocationRequest)


So this (and the other occurrences of this pattern) are wrong, I think.  This will call NoteNativeChild, when what you really want is something that calls NoteXPCOMChild.

So I think you should avoid the NSTARRAY macros (4 occurrences), and just write out something that looks like a cross between the guts of NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY and NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY (you want to NoteXPCOMChild instead of NoteNativeChild).


Other than that, this looks fine to me, but review- because of the last issue (which I think is roughly what peterv said in comment 4).

That said, peterv might be a better reviewer for this than I am.
Attachment #357008 - Flags: review?(dbaron) → review-
Comment on attachment 357008 [details] [diff] [review]
patch v.2

i was wondering about the tarray macros.  Peter warned against them above.  I was looking at:

http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsBindingManager.cpp#403

and that seems to be doing the same thing as I want to.  Is that a bug?

new patch coming up.
But nsXBLBinding is a non-XPCOM refcounted object that uses NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS and the other *NATIVE macros.
Attached patch patch v.3Splinter Review
Attachment #357008 - Attachment is obsolete: true
Attachment #357051 - Flags: superreview?(dbaron)
Attachment #357051 - Flags: review?
Attachment #357051 - Flags: superreview?(dbaron)
Attachment #357051 - Flags: superreview+
Attachment #357051 - Flags: review?
Attachment #357051 - Flags: review+
mc: 20780ac6ca4a

peterv, could you also take a look to make sure we are doing everything correctly here?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Yeah, this looks fine to me too.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.