If a geolocation request takes cached data, it might not get any updated data later.

RESOLVED FIXED in Firefox 47

Status

()

Core
Geolocation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ya-Chieh Wu, Assigned: Ya-Chieh Wu)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Below is the code in nsGeolocation.cpp when we initialize a request.

  if (canUseCache) {
    // okay, we can return a cached position
    // getCurrentPosition requests serviced by the cache
    // will now be owned by the RequestSendLocationEvent
    Update(lastPosition.position);
  } else {
    // Kick off the geo device, if it isn't already running
    nsresult rv = gs->StartDevice(GetPrincipal());

    if (NS_FAILED(rv)) {
      // Location provider error
      NotifyError(nsIDOMGeoPositionError::POSITION_UNAVAILABLE);
      return NS_OK;
    }
  }

It looks like a problem that if we take the cached data and without other requests which setup the connection in the parent side, then we can't get any later update.
(Assignee)

Updated

2 years ago
Summary: If geolocation request takes cached data, it might not get any updated data later. → If a geolocation request takes cached data, it might not get any updated data later.
(Assignee)

Updated

2 years ago
Assignee: nobody → ywu
(Assignee)

Comment 1

2 years ago
Created attachment 8710887 [details] [diff] [review]
Bug-1238873 Handle the bug that if we take cached data, we  might not get any update later.

Only the request, which is getCurrectPosition and is able to take cached data, can skip the StartDevice.
Attachment #8710887 - Flags: review?(kchen)
Comment on attachment 8710887 [details] [diff] [review]
Bug-1238873 Handle the bug that if we take cached data, we  might not get any update later.

Review of attachment 8710887 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/geolocation/nsGeolocation.cpp
@@ +513,4 @@
>  
> +    // We haven't put getCurrentPosition into the callback array yet
> +    // so we don't need to remove it. And after Update is called,
> +    // getCurrentPosition finishes it's job.

I think this comment needs some clarification. How about only the second sentence? "getCurrentPosition finishes its job after Update is called."
Attachment #8710887 - Flags: review?(kchen) → review+
(Assignee)

Comment 3

2 years ago
Created attachment 8715608 [details] [diff] [review]
fix the problem that if watchpositon takes cached data, then it might not get updated position later.

try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1ec792e3d94&selectedJob=16249788
Attachment #8710887 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
failed to apply:

renamed 1238873 -> 0001-Bug-1238873-Handle-the-bug-that-if-we-take-cached-da.patch
applying 0001-Bug-1238873-Handle-the-bug-that-if-we-take-cached-da.patch
patching file dom/geolocation/nsGeolocation.cpp
Hunk #1 FAILED at 504
1 out of 1 hunks FAILED -- saving rejects to file dom/geolocation/nsGeolocation.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Bug-1238873-Handle-the-bug-that-if-we-take-cached-da.patch
Flags: needinfo?(ywu)
Keywords: checkin-needed
(Assignee)

Comment 5

2 years ago
Created attachment 8715787 [details] [diff] [review]
0001-Bug-1238873-Handle-the-bug-that-if-we-take-cached-da.patch

fix the conflicts and here is try server

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d6740a0472f
Flags: needinfo?(ywu)
(Assignee)

Comment 6

2 years ago
Hey josh,

Could you please help to review the newest patch? Because you helped to review the "else" part recently on Bug 858827 which cause the conflict to my previous patch.  

Thank you so much.
Flags: needinfo?(josh)
(Assignee)

Updated

2 years ago
Attachment #8715787 - Flags: review?(josh)

Comment 7

2 years ago
Comment on attachment 8715787 [details] [diff] [review]
0001-Bug-1238873-Handle-the-bug-that-if-we-take-cached-da.patch

Review of attachment 8715787 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks for fixing this!

::: dom/geolocation/nsGeolocation.cpp
@@ +552,3 @@
>    }
>  
> +

nit: remove this extra blank line.
Attachment #8715787 - Flags: review?(josh) → review+

Updated

2 years ago
Flags: needinfo?(josh)
(Assignee)

Comment 8

2 years ago
Created attachment 8719629 [details] [diff] [review]
Handle the bug that if we take cached data, we might not get any update later.
Attachment #8715608 - Attachment is obsolete: true
Attachment #8715787 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a24be316086f
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a24be316086f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.