683472 - geolocation has trouble dealing with simultaneous calls to getCurrentPosition() and watchPosition()

Status: NEW

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

Description

[Louis-Rémi BABE]

1. When using watchPosition and *then* getCurrentPosition() in the same snippet, only "CURRENT" will be displayed in the console.

navigator.geolocation.watchPosition(function( pos ) {
    console.log( "WATCH", pos );
});
navigator.geolocation.getCurrentPosition(function( pos ) {
    console.log( "CURRENT", pos );
});

2. When using getCurrentPosition() and *then* watchPosition() or getCurrentPosition() again, geolocation will stop working, accross all tabs (sometime you have to rune the snippet twice to see the bug)

navigator.geolocation.getCurrentPosition(function( pos ) {
    console.log( "CURRENT", pos );
});
navigator.geolocation.watchPosition(function( pos ) {
    console.log( "WATCH", pos );
});

3. Using watchPosition() and then watchPosition() again has the same effect as 2.

In any case, only the callback of the last call to watchPosition() or getCurrentPosition() ever fires (if geolocation does not stop working immediately).

Comment 1

[u279076]

Can you provide a minimized testcase which showcases this bug?

Comment 2

[Louis-Rémi BABE]

Here are the test cases:
http://fiddle.jshell.net/louisremi/MypZQ/2/show/
http://fiddle.jshell.net/louisremi/MypZQ/6/show/

I have trouble reproducing the case where geolocation stops working. Sometime I have to refresh http://fiddle.jshell.net/louisremi/MypZQ/2/show/ a dozen of times before it happens, sometime it happens on the first time I open the page. When it happens, I have to restart the browser.

So the first thing to fix is the fact that only the last call to watchPosition() or getCurrentPosition() has its callback executed.

Comment 3

[u279076]

When this starts failing do any errors appear in the Error Console?

Comment 4

[Louis-Rémi BABE]

No, I have checked and I haven't seen any.

Comment 5

[Josh Matthews [:jdm]]

Oh, gross, this happens because two prompts are triggered, but they're collapsed into one prompt for the most recent geolocation request (the watch request). Then when that's allowed and an update occurs, we see the getCurrentPosition request which hasn't been allowed, so we skip over it, and then remove it so it doesn't get any more updates.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Allow or deny all similar pending content permission requests when permissino is granted or denied.

This solves the problem of the first testcase. I'm still digging into why the second one behaves so strangely.

Comment 7

[Josh Matthews [:jdm]]

Mark, I'm interested in your feedback as to whether we should look into collapsing similar content permission prompts on mobile. I believe that with the sample testcase above we'll just display two prompts.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 558299 [details] [diff] [review]
Allow or deny all similar pending content permission requests when permissino is granted or denied.

It seems like this is the "right" thing to do. We could start stacking each prompt in it's own row, but that seems like it could confuse the user and just look amateurish.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Note that mobile has similar code here:
http://mxr.mozilla.org/mozilla-central/source/mobile/components/ContentPermissionPrompt.js

Comment 10

[Louis-Rémi BABE]

Hey Josh, thank you for taking care of this once again. While you are at it, I suggest you have a look at bug 683656. Users should have to click only once on "share location" per browsing session.

Comment 11

[Josh Matthews [:jdm]]

Attachment: Allow or deny all similar pending content permission requests when permissino is granted or denied.

Rebased.

Comment 12

[Josh Matthews [:jdm]]

Comment on attachment 585162 [details] [diff] [review]
Allow or deny all similar pending content permission requests when permissino is granted or denied.

No response from firefox-reviewers for more than a month. Let's try throwing a dart at the map.

Comment 13

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

(In reply to Josh Matthews [:jdm] from comment #12)
> No response from firefox-reviewers for more than a month. Let's try throwing
> a dart at the map.

Sorry if there was some misunderstanding, but that's not a valid use of the alias. The mailing list doesn't get normal bugmail, and this bug isn't in the Firefox product, so that request went to /dev/null. firefox-reviewers get mail when "review" flags are changed in bugs that are in the Firefox product, regardless of the assignee.

Comment 14

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

Comment on attachment 585162 [details] [diff] [review]
Allow or deny all similar pending content permission requests when permissino is granted or denied.

It looks like this can potentially leak request objects indefinitely (including past the time when their associated tabs are closed), which doesn't seem acceptable.

One way to solve that might be to have the requests live on the notification object (which gets cleared out on tab close), rather than in the prompter.

Does the relevant spec mention how to deal with multiple successive prompts? Do you know how other browsers handle this?

Comment 15

[Josh Matthews [:jdm]]

I'm not actively looking into this. However, if someone wanted to tackle this, answering Gavin's question about what other browsers do would be a great first step, and then the notification code lives at http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm . Adapting the patch here to store the request objects inside the notifications instead of the prompt code should not be too difficult.

Comment 16

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 17

[Josh Matthews [:jdm]]

It's unclear to me why this was blocking e10s.