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

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Ya-Chieh Wu (inactive)]

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.

Comment 1

[Ya-Chieh Wu (inactive)]

Attachment: 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.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

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."

Comment 3

[Ya-Chieh Wu (inactive)]

Attachment: 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

Comment 4

[Carsten Book [:Tomcat]]

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

Comment 5

[Ya-Chieh Wu (inactive)]

Attachment: 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

Comment 6

[Ya-Chieh Wu (inactive)]

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.

Comment 7

[Josh Matthews [:jdm]]

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.

Comment 8

[Ya-Chieh Wu (inactive)]

Attachment: Handle the bug that if we take cached data, we might not get any update later.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a24be316086f

Comment 10

[Carsten Book [:Tomcat]]

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