459893 - Geolocation prompt stops working if initializer window is closed

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Robert Kaiser]

The geolocation prompt stops working if the window that initialized it is being closed.
STR:
1) Call up http://people.mozilla.org/~dougt/geo.html in a current Minefield nightly
2) Acknowledge the notification bar with either exact location or neighborhood and see the test working fine.
3) Open a new window
4) Close the first window
5) Load http://people.mozilla.org/~dougt/geo.html in the new window

With those steps, you don't get another notification bar.

This is probably related to bug 459413 comment #2, setting a service callback to a window object might just be a bad idea.

Comment 1

[Doug Turner (:dougt)]

the worry here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#915

is that the browser will close, and the service callback that is set on geolocationService.prompt will be gone.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

We should probably set the callback in browserglue somewhere, I guess? We don't want to init the geolocation service earlier than it's needed, though... it would probably cleaner to have the geolocation service get a contract ID via the category manager and instantiate its nsIGeolocationPrompt directly when needed, or something like that. Alternatively we could have it send out a notification when it wants to prompt, and have observers return the nsIGeolocationPrompt via aSubject, though that's hackier.

Comment 3

[Robert Kaiser]

Neil has hacked up something that loads it on ui startup in bug 459413 (nsSuiteGlue is our version of nsBrowserGlue), but your proposal of only loading it when needed sounds even better to me...

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 5

[Doug Turner (:dougt)]

geolocation will just look up a contract id for the nsIGeolocationPrompt and use that.

Comment 6

[Robert Kaiser]

I just tested the patch (needed some manual help to apply, might also work with higher fuzz settings), and it works fine for the case described in comment #0.
I also did test the core changes with a new SeaMonkey patch in bug 459413 and it also works fine there.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 344129 [details] [diff] [review]
patch v.1

You need to fix dom/tests/mochitest/geolocation/geolocation_common.js as well, right?

Is there any reason to keep NS_GEOLOCATION_SERVICE_CONTRACTID?

It would be nice if you could address Neil's second comment from bug 459413 comment 2.

Comment 8

[Doug Turner (:dougt)]

gavin, right.  I do not need the service contract id now.  I was thinking about using it for the extension developers, but for now I can remove it.

I will also fix up the mochitest (and figure out why they all pass despite this changeset!)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Please add some some documentation that nsIGeolocationPrompt must be implemented,
otherwise geolocation API doesn't really work. This is especially needed
for embedders.
Or file a new bug to make geolocation API somehow useable without prompts.
Either way.

Comment 10

[Doug Turner (:dougt)]

pushed 3b47c0e2f6d9.

Comment 11

[Doug Turner (:dougt)]

reopened due to possible leak.

Comment 12

[Doug Turner (:dougt)]

Attachment: same as last patch w/ updated comments and tests cases

Comment 13

[Doug Turner (:dougt)]

that last patch included my merge.  sorry.

Comment 14

[Doug Turner (:dougt)]

Attachment: without merge

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 344549 [details] [diff] [review]
without merge

i am thinking that the way we leak is caused by the way we change the prompt.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: applied over the previous patch

This fixes the leak on linux

Comment 17

[Doug Turner (:dougt)]

thanks smaug.  wfm.

pushed 89c97fd8a881.  Lets see how it goes.

Comment 18

[Doug Turner (:dougt)]

removing blocking1.9.1 flag.  bz -- this actually landed on 1.9.1 either before we created the 1.9.1 branch or sometime there after.  in either case, 1.9.1 _has_ this fix.

Comment 19

[Boris Zbarsky [:bzbarsky]]

That doesn't affect the blocking status.  If it would block if it were to regress tomorrow, it should be marked blocking.  This was discussed at the Gecko meeting on Tuesday.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

(restoring flag)

Comment 21

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3b47c0e2f6d9