Last Comment Bug 688465 - success callback never fires when a position update is triggered after getCurrentPosition() prompted for permission
: success callback never fires when a position update is triggered after getCur...
Status: RESOLVED FIXED
[mentor=jdm]
:
Product: Core
Classification: Components
Component: Geolocation (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla10
Assigned To: Jiten [:deLta30]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-22 07:47 PDT by Louis-Rémi BABE
Modified: 2011-10-03 16:51 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
addon to trigger a location update (157.09 KB, application/x-xpinstall)
2011-09-22 07:47 PDT, Louis-Rémi BABE
no flags Details
patch (2.71 KB, patch)
2011-10-03 08:22 PDT, Jiten [:deLta30]
no flags Details | Diff | Splinter Review
patch_1 (2.22 KB, patch)
2011-10-03 11:44 PDT, Jiten [:deLta30]
josh: review+
Details | Diff | Splinter Review

Description Louis-Rémi BABE 2011-09-22 07:47:26 PDT
Created attachment 561734 [details]
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] 2011-09-29 23:01:25 PDT
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] 2011-09-29 23:02:53 PDT
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] 2011-10-03 08:22:53 PDT
Created attachment 564192 [details] [diff] [review]
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] 2011-10-03 09:28:47 PDT
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] 2011-10-03 11:44:48 PDT
Created attachment 564276 [details] [diff] [review]
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] 2011-10-03 12:11:25 PDT
Comment on attachment 564276 [details] [diff] [review]
patch_1

Perfect. I'll check this in shortly.
Comment 8 Matt Brubeck (:mbrubeck) 2011-10-03 16:51:45 PDT
https://hg.mozilla.org/mozilla-central/rev/c9af88f9998b

Note You need to log in before you can comment on or make changes to this bug.