Closed Bug 826102 Opened 7 years ago Closed 7 years ago

Geolocation maximumAge = 0 returns a cached fix

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- fixed
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: m1, Assigned: jdm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [cr 436252])

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [cr 436252]
Component: General → Geolocation
Product: Boot2Gecko → Core
Version: unspecified → Trunk
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?
blocking-basecamp: ? → +
Assigning to Doug for investigation.
Assignee: nobody → doug.turner
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
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.
Hum, my mistake, it can certainly be called if the IPC listener is removed and added again.
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.
jdm is working on this.
Assignee: doug.turner → josh
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 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"
(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.
Churn churn churn. Now with APIs that take less JS muckity-muck.
Attachment #698223 - Attachment is obsolete: true
Attachment #698223 - Flags: review?(doug.turner)
Attachment #698670 - Flags: review?(doug.turner)
Attachment #698670 - Flags: review?(doug.turner) → review+
You need to log in before you can comment on or make changes to this bug.