452834 - nsGeolocation should be added to cycle collection

Status: RESOLVED FIXED

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

Description

[Doug Turner (:dougt)]

In bug 452762, it was suggested that nsGeolocation should use the CC.

Comment 1

[Doug Turner (:dougt)]

jst, how important is this?

Comment 2

[Johnny Stenback (:jst)]

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

Comment 3

[Doug Turner (:dougt)]

Attachment: wip patch.

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.

Comment 4

[Peter Van der Beken [:peterv]]

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

Comment 5

[Peter Van der Beken [:peterv]]

(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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[Doug Turner (:dougt)]

will take a look tonight/tomorrow.

Comment 8

[Doug Turner (:dougt)]

Attachment: ugly but useful ownership graph

Comment 9

[Doug Turner (:dougt)]

fwiw, i do not claim that this is in any way proper uml. ;-)

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 11

[Doug Turner (:dougt)]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[Doug Turner (:dougt)]

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.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But nsXBLBinding is a non-XPCOM refcounted object that uses NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS and the other *NATIVE macros.

Comment 16

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 357051 [details] [diff] [review]
patch v.3

r+sr=dbaron

Comment 18

[Doug Turner (:dougt)]

mc: 20780ac6ca4a

peterv, could you also take a look to make sure we are doing everything correctly here?

Comment 19

[Peter Van der Beken [:peterv]]

Yeah, this looks fine to me too.

Comment 20

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cabcbb97bb86