688465 - success callback never fires when a position update is triggered after getCurrentPosition() prompted for permission

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Louis-Rémi BABE]

Attachment: addon to trigger a location update

Steps to reproduce: 
1. install the attached restartless addon.
2. visit http://html5demos.com/geo or any site using getCurrentPosition()
3. when the prompts ask to share location, right click anywhere in the page instead, and use the "trigger geoloc update" item in the context-menu
4. open the prompt again and share location
5. there is no 5. unfortunately, your faked location is not displayed on the page

If you trigger the geoloc update before being prompted, the problem doesn't occur.
If you use watchPosition instead of getCurrentPosition, the problem doesn't occur

Comment 1

[Josh Matthews [:jdm]]

I took a look at this. The problem is that nsGeolocation::Update correctly updates all pending callbacks (ie. every getCurrentLocation request), but the actual request object sees that it hasn't been allowed yet and bails. nsGeolocation::Update then clears the array of pending callbacks, so the request never gets a chance to be updated in the future. We probably need to make nsGeolocationRequest::Update return a value indicating whether it's actually going to update or not (based on mAllowed), and only remove elements from mPendingCallbacks that will actually be updated.

Comment 2

[Josh Matthews [:jdm]]

I'm going to unassign myself for now, since I'm really busy with school. Comment 1 should give a decent roadmap for how to solve this bug for anybody who's interested, and I'm happy to provide more information. The relevant code is all contained in dom/src/geolocation/nsGeolocation.cpp.

Comment 3

[Jiten [:deLta30]]

Attachment: patch

I couldn't check using the test because the addon isn't compatible with Nightly 10.0a1 but I think this should do it.

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 564192 [details] [diff] [review]
patch

Great! You can test if you install the Addon Compatibility Reporter, which makes addons compatible with any version of Firefox. This patch is almost correct, but I'd like to see another version with the following fixes:

>-  if (mCleared || !mAllowed)
>+  if (mCleared)

Let's not lose this check. I don't want to accidentally give websites geolocation info if someone calls SendLocation without checking mAllowed first.

>+  if (mAllowed) {
>+    nsCOMPtr<nsIRunnable> ev  =
>+        new RequestSendLocationEvent(aPosition, this,
>+                                     mIsWatchPositionRequest ? nsnull : mLocator);
>+    NS_DispatchToMainThread(ev);
>+    return true;
>+  }
>+  return false;

If you do |if (!mAllowed)| and return false immediately, then the rest doesn't need to be indented.

>+    if (mPendingCallbacks[i]->Update(aSomewhere))
>+      mPendingCallbacks.RemoveElementAt(i);

This won't work quite right, because removing an element causes the remaining ones to shift one posisition to the left, so we'll skip elements. Instead, if we use |for (PRUint32 i = mPendingCallbacks.Length(); i > 0; )|, and index using i - 1, then iteration will be safe.

Comment 5

[Jiten [:deLta30]]

Attachment: patch_1

>+    if (mPendingCallbacks[i]->Update(aSomewhere))
>+      mPendingCallbacks.RemoveElementAt(i);

This won't work quite right, because removing an element causes the remaining ones to shift one posisition to the left, so we'll skip elements. Instead, if we use |for (PRUint32 i = mPendingCallbacks.Length(); i > 0; )|, and index using i - 1, then iteration will be safe.

Nice catch.

I have checked this fix and it's working.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 564276 [details] [diff] [review]
patch_1

Perfect. I'll check this in shortly.

Comment 7

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c9af88f9998b

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/c9af88f9998b