Closed Bug 1238873 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ywu, Assigned: ywu)

Details

Attachments

(1 file, 3 obsolete files)

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.
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: nobody → ywu
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+
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
fix the conflicts and here is try server

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d6740a0472f
Flags: needinfo?(ywu)
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)
Attachment #8715787 - Flags: review?(josh)
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+
Flags: needinfo?(josh)
Attachment #8715608 - Attachment is obsolete: true
Attachment #8715787 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a24be316086f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.