The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
Geolocation
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Louis-Rémi BABE, Assigned: deLta30)

Tracking

Trunk
mozilla10
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jdm])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

Updated

6 years ago
Assignee: nobody → josh

Comment 1

6 years ago
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

6 years ago
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.
Assignee: josh → nobody
Whiteboard: [mentor=jdm]
(Assignee)

Comment 3

6 years ago
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.
Attachment #564192 - Flags: review?(josh)

Comment 4

6 years ago
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.
Attachment #564192 - Flags: review?(josh)
(Assignee)

Comment 5

6 years ago
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.
Assignee: nobody → jitenmt
Attachment #564192 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #564276 - Flags: review?(josh)

Comment 6

6 years ago
Comment on attachment 564276 [details] [diff] [review]
patch_1

Perfect. I'll check this in shortly.
Attachment #564276 - Flags: review?(josh) → review+

Comment 7

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/c9af88f9998b
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/c9af88f9998b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.