Closed Bug 826102 Opened 7 years ago Closed 7 years ago
Age = 0 returns a cached fix
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.
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?
Assigning to Doug for investigation.
Assignee: nobody → doug.turner
The provider does not cache. Throwing a cheesy trace message in nsGeolocation.cpp : 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 ---  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 #698670 - Flags: review?(doug.turner) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9c3293df6d95 for test failures.
You need to log in before you can comment on or make changes to this bug.