497032 - "ASSERTION: Oops! You're asking for a weak reference to an object that doesn't support that" on bankrate.com (geolocation on stack)

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Jesse Ruderman]

Attachment: stack trace

Steps to reproduce:
1. Load http://www.bankrate.com/ in a debug Firefox build.
2. Quit Firefox.

Result:

###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that doesn't support that.: 'factoryPtr', file nsWeakReference.cpp, line 109

Jonas, how can I debug this?

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You need to dump the JS stack. There's some function somewhere in XPConnect that will dump the current JS stack to the console I think.

Jst knows iirc.

Comment 2

[Jesse Ruderman]

(gdb) call DumpJSStack()
0 [native frame]
1 anonymous() ["file:///Users/jruderman/central/debug-obj/dist/MinefieldDebug.app/Contents/MacOS/components/NetworkGeolocationProvider.js":179]
    os = [xpconnect wrapped nsIObserverService @ 0x1e6228b0 (native @ 0x71dcf0)]
    prefBranch = [xpconnect wrapped nsIPrefBranch @ 0x1906e2e0 (native @ 0x71e4f0)]
    this = [object Object]

NetworkGeolocationProvider.js:179 is the following line in the shutdown method of WifiGeoPositionProvider:
    os.removeObserver(this, "private-browsing");

Comment 3

[Jesse Ruderman]

mrbkap says weak references aren't available during shutdown, so Geolocation should not call removeObserver (or call it earlier than shutdown).

Comment 4

[Doug Turner (:dougt)]

as so long as we don't leak, i have no objection to removing the removeObserver call.

Are others also doing the wrong thing?

http://mxr.mozilla.org/mozilla-central/search?string=os.removeObserver%28this%2C+%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 5

[Nochum Sossonko [:Natch]]

The others usually do it on xpcom-shutdown or some other equivalent, not sure if geo can use that though...

Comment 6

[Doug Turner (:dougt)]

i have no problem with simply not removing the observer as so long as it doesn't cause a leak (which it probably doesn't).

Comment 7

[Nochum Sossonko [:Natch]]

I think that causes the assertion in bug 481317, at least if I understood that correctly.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that since the QI to weaksupports currently fails, the call to removeObserver is a no-op.

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 10

[Nochum Sossonko [:Natch]]

Comment on attachment 397191 [details] [diff] [review]
patch v.2

I'm totally not a reviewer for this, but afaik you're responsible to remove any observers you add. All the other places use xpcom-shutdown (even those observing xpcom-shutdown itself) for this, is there any reason you can't?

Even though it probably isn't a memory leak per se (your really using this for the life of the app, which xpcom-shutdown signals), convention in current code dictates that it's good practice.

Comment 11

[Doug Turner (:dougt)]

I thought i would get your input Natch.  As jonas mentioned, it is a noop and therefore probably not needed.

jonas, do you have an opinion one way or the other?

Comment 12

[Nochum Sossonko [:Natch]]

Hrm, on second thought, this is component js, rules are somewhat different here. See nsSessionStore, which afaict doesn't release _any_ of its observers (and it has a bunch).

Component code is supposed to live for the lifetime of the app so using xpcom-shutdown to remove the observers is kind of redundant, I guess.

Comment 13

[(no longer active)]

Attachment: Patch (v3)

If we make this object support weak references, and not call RemoveObserver, the observer will be released when the observer service dies, and nothing will crash if WifiGeoPositionProvider is shut down before that time.

Comment 14

[Nochum Sossonko [:Natch]]

You have to also change the third argument of addObserver to true or this change won't take affect since you're still asking for a strong reference.

Comment 15

[(no longer active)]

Landed with comment 14 addressed:

http://hg.mozilla.org/mozilla-central/rev/376f6bf359c8

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 397524 [details] [diff] [review]
Patch (v3)

For branches would prefer to approve a patch that reflects what got checked in.

Comment 17

[(no longer active)]

Attachment: As checked in

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 401715 [details] [diff] [review]
As checked in

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 401715 [details] [diff] [review]
As checked in

past code-freeze for 1.9.1.4, removing non-blocker approval.

Comment 20

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/eed34cad90ea

Comment 21

[Daniel Veditz [:dveditz]]

QA's insists on a testcase if we're going to take this on the increasingly conservative 1.9.1 branch

Comment 22

[Samuel Sidler (old account; do not CC)]

Comment on attachment 401715 [details] [diff] [review]
As checked in

We need a testcase before we take this on 1.9.1.

Comment 23

[(no longer active)]

Is this really wanted for 1.9.1?  I want to make sure it's needed before I invest time in creating a test case for it.

Alternatively, Natch, are you willing to write a test for this?

Comment 24

[Samuel Sidler (old account; do not CC)]

Yes, this is wanted on 1.9.1. (And really, it should've had a testcase before landing on trunk or 1.9.2, but it's a bit too late for that now.)

Comment 25

[(no longer active)]

Doug, do you have any idea how to write a test case for this bug?  Should I just write an xpcshell test which calls nsIGeolocationProvider::shutdown?  Are assertions considered as failures in debug build tests?