826102 - Geolocation maximumAge = 0 returns a cached fix

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect, P1)

Description

[Michael Vines [:m1] [:evilmachines]]

STR:
1. Issue a geolocation request with maximumAge = 0
2. Wait for a fix
3. Do #1 again
4. Observe that the cached fix from #1 is returned again immediately rather than waiting for a new fix from the gps HAL.

Comment 1

[Josh Matthews [:jdm]]

I'm not convinced that this is a fault of the general geolocation service code. I've stepped through it in GDB and I never see cached positions being used in the content process, nor do I see cached positions being sent from the chrome process. Maybe this is a problem inside the GPS provider?

Comment 2

[Dietrich Ayala (:dietrich)]

Assigning to Doug for investigation.

Comment 3

[Michael Vines [:m1] [:evilmachines]]

The provider does not cache.

Throwing a cheesy trace message in nsGeolocation.cpp [1]:
  fprintf(stderr, "XXX: Allow() --- pid:%d  maximumAge:%d\n", getpid(), maximumAge);

seems to imply that maximumAge is good in the content process but gets reset to the default (30ms) in the system process:

  XXX: Allow() --- pid:667  maximumAge:0
  XXX: Allow() --- pid:573  maximumAge:30000

---
$ adb shell b2g-ps
APPLICATION      USER     PID   PPID  VSIZE  RSS     WCHAN    PC         NAME
b2g              root      573   571   178316 52944 ffffffff 400bf430 S /system/b2g/b2g
Homescreen       app_641   641   573   66396  24632 ffffffff 400ed430 S /system/b2g/plugin-container
GpsTest          app_667   667   573   59080  19952 ffffffff 4008b430 S /system/b2g/plugin-container
---

[1] http://mxr.mozilla.org/mozilla-beta/source/dom/src/geolocation/nsGeolocation.cpp#428

Comment 4

[Josh Matthews [:jdm]]

I grant you that nsGeolocationRequest::Allow (which is the only place the caching code is present) can and does run in the system process. In particular, any geolocation calls made sit pending until the geolocation settings can be determined, at which point nsGeolocationRequest::Allow runs for each pending call, and once they are resolved there is no reason that code should run in the parent (and GDB doesn't show it doing so, either). Furthermore, when it runs, there is no cached position.

Comment 5

[Josh Matthews [:jdm]]

Hum, my mistake, it can certainly be called if the IPC listener is removed and added again.

Comment 6

[Josh Matthews [:jdm]]

The least-invasive fix here might be to pass an options object to the WatchPosition call in ContentParent that includes a maximumAge property of 0 to ensure that no cached positions are ever forwarded to content processes, which can handle caching properly on their own.

Comment 7

[Doug Turner (:dougt)]

jdm is working on this.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Make IPC geolocation listener never use cached values and never skip a value.

Comment 9

[Josh Matthews [:jdm]]

There were several issues addressed:
* the content process was at the mercy of receiving cached positions from the chrome process. Solved by passing a maximumAge of 0 for the IPC listener.
* the wifi geolocation position serialization was completely broken by bug 727233. Gonk GPS provider works fine.
* the check for first update could produce incorrect results for multiple listeners from a content process, since they're all handled by a single listener. Fixed by readding a new listener every time a new geolocation request is allowed, since proper first time checks will be run for the actual listeners in the content process.

Writing the test exposed this delightful confluence of problems.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 698223 [details] [diff] [review]
Make IPC geolocation listener never use cached values and never skip a value.

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

::: dom/ipc/ContentParent.cpp
@@ +1961,5 @@
> +  jsval value = INT_TO_JSVAL(0);
> +  bool ok = JS_SetProperty(cx, options, "maximumAge", &value);
> +  NS_ENSURE_TRUE(ok, true);
> +
> +  geo->WatchPosition(this, nullptr, OBJECT_TO_JSVAL(options), cx, &mGeolocationWatchID);

What do you think about treating a JSVAL_VOID options as having a maximumAge of zero?

Or adding a new no-script API off of nsIDOMGeoGeolocation?

::: dom/system/NetworkGeolocationProvider.js
@@ +14,5 @@
>  const Cc = Components.classes;
>  
>  let gLoggingEnabled = false;
>  let gTestingEnabled = false;
> +let gUseScanning = true;

Can we just use gTestingEnabled?

::: dom/tests/unit/xpcshell.ini
@@ +11,5 @@
>  [test_geolocation_timeout_wrap.js]
>  skip-if = os == "mac"
> +[test_multiple_geo_listeners.js]
> +[test_multiple_geo_listeners_wrap.js]
> +skip-if = os == "mac"

can we drop this last: skip-if = os == "mac"

Comment 11

[Josh Matthews [:jdm]]

(In reply to Doug Turner (:dougt) from comment #10)
> ::: dom/system/NetworkGeolocationProvider.js
> @@ +14,5 @@
> >  const Cc = Components.classes;
> >  
> >  let gLoggingEnabled = false;
> >  let gTestingEnabled = false;
> > +let gUseScanning = true;
> 
> Can we just use gTestingEnabled?

No. There are multiple different assertions that occur when using gTestingEnabled under xpcshell which do not look easy to resolve (also DBUS crashes if we use the actual scanner).
 
> ::: dom/tests/unit/xpcshell.ini
> @@ +11,5 @@
> >  [test_geolocation_timeout_wrap.js]
> >  skip-if = os == "mac"
> > +[test_multiple_geo_listeners.js]
> > +[test_multiple_geo_listeners_wrap.js]
> > +skip-if = os == "mac"
> 
> can we drop this last: skip-if = os == "mac"

No. Content process xpcshell tests have a habit of not working on OS X for reasons that are filed in bugzilla and challenging to fix.

Comment 12

[Josh Matthews [:jdm]]

Attachment: Make IPC geolocation listener never use cached values and never skip a value.

Churn churn churn. Now with APIs that take less JS muckity-muck.

Comment 13

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc561810a74

Comment 14

[Josh Matthews [:jdm]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9c3293df6d95 for test failures.

Comment 15

[Josh Matthews [:jdm]]

https://hg.mozilla.org/mozilla-central/rev/683f2f763316
https://hg.mozilla.org/releases/mozilla-b2g18/rev/58ae70a2bd8c