1.google map in browser can't work in either user build or userdebug build. 2. market app zMaps can work in both user build and userdebug build. 3. dolphin v2.0 branch : google map can work in both user build and user debug build. It seems gps works well, but something wrong in browser.

Evelyn, Can you kick this investigation off? Thomas is going to try if there is a difference between MOZILLA_OFFICIAL=0 and MOZILLA_OFFICIAL=1.

if don't export MOZILLA_OFFICIAL=1 in v1.4, google map can work in browser.

Works on FxOS 1.3. Questions: 1. What devices are you seeing this on? 2. What user agents are being sent on each of these devices? I don't see how MOZILLA_OFFICIAL would play a factor here btw - google maps should only be affected by changes to the UA, which I don't think MOZILLA_OFFICIAL affects.

(In reply to Jason Smith [:jsmith] from comment #3) > Works on FxOS 1.3. > > Questions: > > 1. What devices are you seeing this on? Per bug title, it's on dolphin. > 2. What user agents are being sent on each of these devices? > > I don't see how MOZILLA_OFFICIAL would play a factor here btw - google maps > should only be affected by changes to the UA, which I don't think > MOZILLA_OFFICIAL affects. I feel the same. Yi-fan, I think it's a good chance to know more UA stuff on Firefox OS, would you like to investigate this bug?

It's not just google maps that doesn't work. I think browser in general is not providing location info when official=1. Thomas just tested with google maps as an example. Thomas to confirm.

ported the latest gecko for Dolphin, ### Push Done. Gaia 9ec45071c9c6bf921061580363257907e0465e16 Gecko https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/7280e2146451 BuildID 20140703160201 Version 30.0 ro.build.version.incremental=89 ro.build.date=Tue May 13 06:09:31 CST 2014 ### Shallow Flash Done! and then tested with the latest v1.4 gaia on Dolphin with PRODUCTION=0 MOZILLA_OFFICIAL=0 PRODUCTION=0 MOZILLA_OFFICIAL=0 PRODUCTION=1 MOZILLA_OFFICIAL=1 PRODUCTION=1 MOZILLA_OFFICIAL=1 all combinations pass.

(In reply to Yi-Fan Liao [:yifan][:yliao] from comment #6) > ported the latest gecko for Dolphin, > > ### Push Done. > Gaia 9ec45071c9c6bf921061580363257907e0465e16 > Gecko https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/7280e2146451 > BuildID 20140703160201 > Version 30.0 > ro.build.version.incremental=89 > ro.build.date=Tue May 13 06:09:31 CST 2014 > > ### Shallow Flash Done! > > and then tested with the latest v1.4 gaia on Dolphin with PRODUCTION=0 MOZILLA_OFFICIAL=0 PRODUCTION=0 MOZILLA_OFFICIAL=1 PRODUCTION=1 MOZILLA_OFFICIAL=0 PRODUCTION=1 MOZILLA_OFFICIAL=1 > all combinations pass. Sorry, typo.

yi-fan, did you identify what the problem was?

We found that previous geolocation test only succeeded once, after flashing gaia onto the device. Then subsequent geolocation request fails in both browser and geolocation API test in UI Test app. It will take several minutes to get another successful geolocation request.

Function broken / regression on v1.4.

We wrote a test for geolocation API. The link is http://jsbin.com/wivaf/1 . Our tests reveal that geolocation API responds unexpectedly with neither OFFICIAL=0 nor OFFICIAL=1, in both browser and UI Test app. As for zMap app, after we digged in its source code, we found that it'll cache previous successful geolocation request.

Thanks. Kaizhen, I discussed with Yi-Fan on this and it seems that you two have agreed that location API does work 1.4 gecko but only intermittently. Can you please follow up on this investigation?

James, can you ask someone to check if GPSEngine does work properly. I notice that GPSEngine will consume up to about 30% cpu. I also verified, where GPSEngine is not running, location service is provided by MLS. That's why I think the location is always return from MLS not from GPS.

Kai-Zhen: when you say GPSengine is not running (in comment 13), are you seeing bug 1031751 about gps.default.so library failing to load? > E/HAL (307): load: module=/system/lib/hw/gps.default.so > E/HAL (307): dlopen failed: could not load library "libandroid_runtime.so" needed by "gps.default.so"; caused by library "libandroid_runtime.so" not found

Also, do you have two SIMs inserted? Or one SIM inserted in slot #2? Bug 878748 describes some GPS problems related to SIM slots.

I have google maps working on dolphin, but not reliably. With geolocation debugging on, I see a problem: http://pastebin.com/raw.php?i=Qv0Xceqw Wifi scans only happen when I first load the page, then after that, the wifi scanning layer no longer responds. So my correct location with longitude -79.43896 is reported, from then on, the cell based lookup (longitude -79.47xxx) is reported. This is reproducible for me. The patch for bug 1018383 will mask this problem if we can't get it fixed, as it caches the more accurate location (wifi+cell in this case).

To clarify my previous comment, Google Maps is always loading and showing a location. Just not the most accurate location available.

Some logs would help to investigate this bug, but I just found bug 1036110, which means MLS geolocation logs are difficult to get. If you have source-level access you can turn on gLoggingEnabled in NetworkGeolocationProvider.js.

(In reply to Garvan Keeley [:garvank] from comment #18) > > If you have source-level access you can turn on gLoggingEnabled in > NetworkGeolocationProvider.js. If I turned on gLoggingEnabled, I can see below logs when try to get position. -- E/GeckoConsole( 112): *** WIFI GEO: startup called. I/Gecko ( 112): *** WIFI GEO: startup called.

> If I turned on gLoggingEnabled, I can see below logs when try to get > position. How is it turned on? Until bug 1036110 is fixed, commenting out this if condition appears to be the best way: http://mxr.mozilla.org/mozilla-b2g30_v1_4/source/dom/system/NetworkGeolocationProvider.js#35

(In reply to Garvan Keeley [:garvank] from comment #18) > If you have source-level access you can turn on gLoggingEnabled in > NetworkGeolocationProvider.js. Oops, I posted this before I looked at the bug in detail, just doing this won't work

(In reply to Chris Peterson (:cpeterson) from comment #14) > Kai-Zhen: when you say GPSengine is not running (in comment 13), are you > seeing bug 1031751 about gps.default.so library failing to load? > > > E/HAL (307): load: module=/system/lib/hw/gps.default.so > > E/HAL (307): dlopen failed: could not load library "libandroid_runtime.so" needed by "gps.default.so"; caused by library "libandroid_runtime.so" not found No, I didn't see the error as bug 1031751. I just removed these file from system and then try to get the position. -- /system/bin/GPSenseEngine /system/lib/hw/gps.default.so

(In reply to Garvan Keeley [:garvank] from comment #20) > > If I turned on gLoggingEnabled, I can see below logs when try to get > > position. > > How is it turned on? I add dev-pref.js and set a pref to turn it on. pref("geo.wifi.logging.enabled", true);

I was told the GPS was not hooked up on Dolphin devices, although a chip is present. If that is correct, this bug should focus on Wifi and Cell geolocation. Also, due to bug 1036110, pref("geo.wifi.logging.enabled", true) will only show a "startup called" message, and then gets ignored.

I'm not familiar with GPS. I turn on the gLoggingEnabled in NetworkGeolocationProvider.js and get the same result as comment 19. WIFI works properly.

Garvan: what next step do you recommend for Zhenqing to debug this problem?

Get a build with bug 1036110 patch, turn on the logging as they have been trying. This time, lots of info will spit out, and we will be able to see what is happening

Marking this wontfix for 2.0 at this point given we do not know what the fix is and the risk is unknown at this point. Keeping in mind stability concerns on 2.0 at this point, we wanted to avoid any code-churn.

Since Kai-Zhen is working on this case. Reassign to Kai-Zhen -- Keven

Kai-Zhen: have you been able to reproduce this problem in Google Maps? This bug is marked as a Dolphin 1.4 blocker.

Yes, I can still reproduce it on Dolphin. But I can't reproduce this issue, when 'geo.provider.use_mls' is set to true, do not use the platform GPS and use only MLS.

(In reply to Kai-Zhen Li from comment #32) > Yes, I can still reproduce it on Dolphin. But I can't reproduce this issue, > when 'geo.provider.use_mls' is set to true, do not use the platform GPS and > use only MLS. Depends on following two bugs? 878748 - [dolphin] B2G GPS: acquire correct RadioInterface instance in MultiSIM configuration 1032063 - Call update_network_state and update_network_availability when network state changes for A-GPS -- Keven

Filtering “CG_libgps” in the log: After GPS starts successfully for 1s，"fixStatus=V" is periodically printed till a position is fixed. 3611 12-18 20:11:32.790 D/CG_libgps( 108): zhouxw:Start server 3627 12-18 20:11:32.840 E/CG_libgps( 108): gps_start: called 3765 12-18 20:11:34.400 E/CG_libgps( 108): in RMC, fixStatus=V 3766 12-18 20:11:35.400 E/CG_libgps( 108): in RMC, fixStatus=V 3767 12-18 20:11:36.400 E/CG_libgps( 108): in RMC, fixStatus=V ...... ...... 4059 12-18 20:11:51.400 E/CG_libgps( 108): in RMC, fixStatus=V

Kai-Zhen, based on Thomas' feedback in comment 34, it sounds like his GPS starts successfully and obtains a good position fix in the log, but Google Maps in the browser is still failing. Is that the same problem you saw in comment 32 when using platform GPS (if you don't force geo.provider.use_mls = true)? Does this look like a Firefox OS browser bug or Dolphin GPS problem?

After GPS start successfully and get a good position (Do not force geo.provider.use_mls = true), Firefox OS browser can get a location in short time, but from the second time it could be failed to get the location or take a very longer time. I think this more looks like a Dolphin GPS problem.

Tested on Flame and it has the same symptom. The first getLocation succeed but the second timed out.

Garvan: if this problem is reproducible on the Flame (Kan-Ru's comment 38), it sounds like a bug in the B2G browser. Have you been able to reproduce this problem on the Flame?

On Flame m-c, I still cannot repro this bug. I have tried manually clearing the AGPS data, still didn't repro the bug. It would be very helpful to repro this outside of google maps, basic pages like: http://remi-grumeau.com/test/geolocation/watchPosition.html

I think I have this reproduced. - FLUSH_AIDE_DATA set in GonkGPSProvider, so that the GPS will not work immediately - run with gps debug logging on and do 'adb logcat | grep GGA' to watch the location messages from the GPS - Do a geolocation request, first time we get a network location, close the page ( you will see message A as described below) - Open again, this time I see that geolocation only kicks in once the GPS has a location (you will see message B as described below) (A) This is no-location nmea: $GPGGA,,,,,,0,,,,,,,,*66 (B) And this is when the GPS has a location: $GPGGA,195253,4339.632281,N,07926.343424,W,2,08,1.5,146.0,M,-41.0,M,,*70 In summary, the first time getting geolocation, when the GPS does not have a location, the Gonk provider uses network location, the second time, it waits for the GPS. It should not be waiting for the GPS in the second case, the network location has a good position. From what I recall of the code, this makes no sense, so I'll have to go re-read and see what is happening.

Hi, Garvan: will you take this bug? thanks!

I have been looking at it when I have time, I think one of the authors of GonkGPSGeolocationProvider will have to step in. SETUP: Set FLUSH_AIDE_DATA (what is AIDE should be AID maybe?) to 1. Load a page with a watch request, and reload a few times. Here is a log from a non-working session, where I hit reload on a page: (I have additional logging on) https://pastebin.mozilla.org/5945687 We see, MLS is responding with a location, we see the critical callback happen with a location: provider->mLocationCallback->Update(position); https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#735 And javascript on the web page never gets the update message. Which makes no sense, there doesn't seem to be any path I can see where the location is not passed up to the JS on the page. I suspect that in order to repro this bug, we just need to disable the GPS on the device in any form (comment out the code where it returns a location, or set the AGPS to flush as I did and watch the NMEA messages to ensure it is properly flushed). I think this bug is the root of a number of other bugs where folks are complaining of intermittent location in GPS-enabled devices.

(In reply to Garvan Keeley [:garvank] from comment #43) > I have been looking at it when I have time, I think one of the authors of > GonkGPSGeolocationProvider will have to step in. > > SETUP: Set FLUSH_AIDE_DATA (what is AIDE should be AID maybe?) to 1. > Load a page with a watch request, and reload a few times. > > Here is a log from a non-working session, where I hit reload on a page: > (I have additional logging on) https://pastebin.mozilla.org/5945687 > > We see, MLS is responding with a location, we see the critical callback > happen with a location: > provider->mLocationCallback->Update(position); > https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > GonkGPSGeolocationProvider.cpp#735 Is this line corresponding to the log line: I/Gecko ( 294): GonkGPSGeolocationProvider::Update(nsIDOMGeoPosition *position) 43.660485,-79.438964 If not then maybe the MLS location was filtered out because it hadn't changed. > And javascript on the web page never gets the update message. Which makes no > sense, there doesn't seem to be any path I can see where the location is not > passed up to the JS on the page. If so it might be that somehow the content callback wasn't registered to the main nsGeolocationService. > I suspect that in order to repro this bug, > we just need to disable the GPS on the device in any form (comment out the > code where it returns a location, or set the AGPS to flush as I did and > watch the NMEA messages to ensure it is properly flushed). > > I think this bug is the root of a number of other bugs where folks are > complaining of intermittent location in GPS-enabled devices. I'll take a look.

> > We see, MLS is responding with a location, we see the critical callback > > happen with a location: > > provider->mLocationCallback->Update(position); > > https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > > GonkGPSGeolocationProvider.cpp#735 > > Is this line corresponding to the log line: > > I/Gecko ( 294): GonkGPSGeolocationProvider::Update(nsIDOMGeoPosition > *position) 43.660485,-79.438964 Yes. > I'll take a look. Thanks Kan-Ru. The easiest repro step for me is to just disable GPS responding (i.e. just unhook GonkGPSGeolocationProvider::LocationCallback(GpsLocation* location)). Then if I reload a watch location page 2-3x, it will stop updating. As you mentioned, seems to be unrelated to GPS, rather a content callback problem.

Is this an issue on Nokia Maps too? Or is it just apps on browser?

per comment #44.

It happens in martket apps, too, but if the app cache the first fixed GPS position , you won't notice the later location miss in the app.

The strangest thing is, the second call to nsGeolocationService::Update via nsIGeolocationUpdate interface doesn't make any effect. I'm not sure how this could happen.. https://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#517

I'm not sure how this happened but I get exceptions when testing MLS.

This is one of the reason we weren't receiving updates from the MLS. We should report MLS locations even when the location aren't changing otherwise we will timeout. Truth table: | Seen GPS? | MLS Changed? || Use MLS? | |-----------+--------------++----------| | F | T || T | | F | F || T | | T | T || T | | T | F || F |

This is the second reason we weren't receiving updates. Because we were comparing NaN to numbers. For some input we expect acos(1) = 0 but we end up calculating acos(1.0000000000000002) = NaN because the limitation of the double precision presentation. Remove the acos() from the formula to avoid this. I also wonder if 10m is too small a distance. Maybe 50m to 100m is better given the precision of the MLS database?

Comment on attachment 8474941 [details] [diff] [review] Part 1. Add try catch around this.wifiService Review of attachment 8474941 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/NetworkGeolocationProvider.js @@ +307,5 @@ > + try { > + self.wifiService = Cc["@mozilla.org/wifi/monitor;1"].getService(Ci.nsIWifiMonitor); > + self.wifiService.startWatching(self); > + } catch(ex) { > + LOG("Can't create wifi service"); Do you know if the exception is thrown from getService() or from startWatching()? We should log the exception info so we can getting a better idea of what's going wrong. Maybe we should also wrap getService() and startWatching() in separate try/catch statements so we can better pinpoint the source of the problem?

Comment on attachment 8474946 [details] [diff] [review] Part 4. Don't use acos() to avoid numeric instability Review of attachment 8474946 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp @@ +710,4 @@ > // Use spherical law of cosines to calculate difference > // Not quite as correct as the Haversine but simpler and cheaper > // Should the following be a utility function? Others might need this calc. > const double radsInDeg = 3.14159265 / 180.0; If we're going to use C++ math functions, we should probably use the double-precision `M_PI` constant, too. @@ +724,5 @@ > sLastMLSPosLon = lon; > > // if the MLS coord change is smaller than this arbitrarily small value > // assume the MLS coord is unchanged, and stick with the GPS location > const double kMinMLSCoordChangeInMeters = 10; kMinMLSCoordChangeInMeters is now unused and should be removed. Please add a comment below explaining where the 10m magic number comes from in the calculation of kMaxMLSCoordChangeInCosRads. :)

Thanks Kan-Ru! I verified this fixes the bug. Also agree on the acos() bug, as the code would frequently calculate values around 1.0 as input to that function. I misinterpreted my logging and thought the JS callback was triggered (well it was sometimes, but that was after timeout, argh).

Comment on attachment 8474944 [details] [diff] [review] Part 3. Always use MLS location if not seen anything from the GPS device for 10s Review of attachment 8474944 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp @@ +733,5 @@ > const int kMaxGPSDelayBeforeConsideringMLS = 10000; > int64_t diff = PR_Now() - provider->mLastGPSDerivedLocationTime; > + if (provider->mLocationCallback && > + (diff > kMaxGPSDelayBeforeConsideringMLS || > + delta > kMinMLSCoordChangeInMeters)) { This will override the GPS location too often. The ideal would be to never override the GPS location with MLS, while the GPS is getting locations. The minimum coord change was an extra guard to further reduce the likelihood of accidental override. It isn't related to MLS accuracy. I would recommend removing delta > kMinMLSCoordChangeInMeters, as it was there as a secondary guard, and now produces the opposite result of increased likelihood of MLS override. One case that guard was handling is that after having lost GPS location, if MLS location is changing, only then it is useful to show MLS location. If MLS is not updating for your movements, then we risk overriding GPS with inaccurate MLS (such as GeoIP). I'll try to look at this code and look for some condition we can hook into for this case.

(In reply to Garvan Keeley [:garvank] from comment #57) > Comment on attachment 8474944 [details] [diff] [review] > Part 3. Always use MLS location if not seen anything from the GPS device for > 10s > > Review of attachment 8474944 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp > @@ +733,5 @@ > > const int kMaxGPSDelayBeforeConsideringMLS = 10000; > > int64_t diff = PR_Now() - provider->mLastGPSDerivedLocationTime; > > + if (provider->mLocationCallback && > > + (diff > kMaxGPSDelayBeforeConsideringMLS || > > + delta > kMinMLSCoordChangeInMeters)) { > > This will override the GPS location too often. The ideal would be to never > override the GPS location with MLS, while the GPS is getting locations. The > minimum coord change was an extra guard to further reduce the likelihood of > accidental override. It isn't related to MLS accuracy. > > I would recommend removing delta > kMinMLSCoordChangeInMeters, as it was > there as a secondary guard, and now produces the opposite result of > increased likelihood of MLS override. Given the table in comment 52, when we have GPS location we only use MLS if MLS is changing. > One case that guard was handling is that after having lost GPS location, if > MLS location is changing, only then it is useful to show MLS location. If > MLS is not updating for your movements, then we risk overriding GPS with > inaccurate MLS (such as GeoIP). The problem is if we lost GPS location then there is no way to tell if the last GPS location is good or bad. One way is to calculate the distance of last know GPS location and the MLS location. If they differ too much then use MLS. Note that we should always report something otherwise it will timeout.

(In reply to Chris Peterson (:cpeterson) from comment #54) > Comment on attachment 8474941 [details] [diff] [review] > Part 1. Add try catch around this.wifiService > > Review of attachment 8474941 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/NetworkGeolocationProvider.js > @@ +307,5 @@ > > + try { > > + self.wifiService = Cc["@mozilla.org/wifi/monitor;1"].getService(Ci.nsIWifiMonitor); > > + self.wifiService.startWatching(self); > > + } catch(ex) { > > + LOG("Can't create wifi service"); > > Do you know if the exception is thrown from getService() or from > startWatching()? > > We should log the exception info so we can getting a better idea of what's > going wrong. Maybe we should also wrap getService() and startWatching() in > separate try/catch statements so we can better pinpoint the source of the > problem? The exception is [Exception... "Component returned failure code: 0x80570019 (NS_ERROR_XPC_CANT_CREATE_WN) [nsIJSCID.getService]" nsresult: "0x80570019 (NS_ERROR_XPC_CANT_CREATE_WN)" location: "JS frame :: jar:file:///system/b2g/omni.ja!/components/NetworkGeolocationProvider.js :: WifiGeoPositionProvider.prototype.startup/settingsCallback.handle :: line 307" data: no]

(In reply to Kan-Ru Chen [:kanru] from comment #60) > (In reply to Chris Peterson (:cpeterson) from comment #54) > > Comment on attachment 8474941 [details] [diff] [review] > > Part 1. Add try catch around this.wifiService > > > > Review of attachment 8474941 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/system/NetworkGeolocationProvider.js > > @@ +307,5 @@ > > > + try { > > > + self.wifiService = Cc["@mozilla.org/wifi/monitor;1"].getService(Ci.nsIWifiMonitor); > > > + self.wifiService.startWatching(self); > > > + } catch(ex) { > > > + LOG("Can't create wifi service"); > > > > Do you know if the exception is thrown from getService() or from > > startWatching()? > > > > We should log the exception info so we can getting a better idea of what's > > going wrong. Maybe we should also wrap getService() and startWatching() in > > separate try/catch statements so we can better pinpoint the source of the > > problem? > > The exception is > > [Exception... "Component returned failure code: 0x80570019 > (NS_ERROR_XPC_CANT_CREATE_WN) [nsIJSCID.getService]" nsresult: "0x80570019 > (NS_ERROR_XPC_CANT_CREATE_WN)" location: "JS frame :: > jar:file:///system/b2g/omni.ja!/components/NetworkGeolocationProvider.js :: > WifiGeoPositionProvider.prototype.startup/settingsCallback.handle :: line > 307" data: no] This is weird since we have nsWifiMonitorGonk.cpp implementing this. Maybe this deserves another bug.

Comment on attachment 8474944 [details] [diff] [review] Part 3. Always use MLS location if not seen anything from the GPS device for 10s Review of attachment 8474944 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp @@ -733,4 @@ > const int kMaxGPSDelayBeforeConsideringMLS = 10000; > int64_t diff = PR_Now() - provider->mLastGPSDerivedLocationTime; > - if (provider->mLocationCallback && diff > kMaxGPSDelayBeforeConsideringMLS > - && delta > kMinMLSCoordChangeInMeters) I recommend: // We want to distiguish between the GPS being inactive completely // and temporrarily inactive. In the former case, we would use a low // accuracy MLS, in the latter, we want an MLS location that appears to be // updating with movement, and to the user provides a GPS-like experience. const bool isGpsFullyInactive = diff > 1000 * 60 * 2; // two mins const bool isGpsTempInactive = diff > 1000 * 10; // 10 secs if (provider->mLocationCallback && (isGpsFullyInactive || isGpsTempInactive && delta > kMinMLSCoordChangeInMeters)) { provider->mLocationCallback->Update(position); }

Comment on attachment 8474946 [details] [diff] [review] Part 4. Don't use acos() to avoid numeric instability Review of attachment 8474946 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp @@ +739,5 @@ > + // Our delta cos(D) is bound to [-1,1] > + // > + // Any value smaller than cos(10/6378137) is larger than 10m when > + // converted. > + const double kMaxMLSCoordChangeInCosRads = 0.9999999999987709; Having the delta in meters is more human-readable, why change it?

(In reply to Garvan Keeley [:garvank] from comment #63) > Comment on attachment 8474946 [details] [diff] [review] > Part 4. Don't use acos() to avoid numeric instability > > Review of attachment 8474946 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp > @@ +739,5 @@ > > + // Our delta cos(D) is bound to [-1,1] > > + // > > + // Any value smaller than cos(10/6378137) is larger than 10m when > > + // converted. > > + const double kMaxMLSCoordChangeInCosRads = 0.9999999999987709; > > Having the delta in meters is more human-readable, why change it? To avoid the acos() call. That's why I put this big block of comment here!

> To avoid the acos() call. That's why I put this big block of comment here! Thanks :), I didn't look closely, I thought you had replaced the delta calculation with an equivalent that was still convertible to meters through multiplication.

(In reply to Garvan Keeley [:garvank] from comment #62) > if (provider->mLocationCallback && > (isGpsFullyInactive || > isGpsTempInactive && > delta > kMinMLSCoordChangeInMeters)) > { > provider->mLocationCallback->Update(position); > } This temporarily inactive case needs more investigation, it requires that MLS is able to detect movement in the last 2 received positions in order to replace a prev GPS. This could be over a longer span. If the MLS is *not* detecting movement over a reasonable timespan, it would be better to report an old GPS location than to use MLS.

Comment on attachment 8474946 [details] [diff] [review] Part 4. Don't use acos() to avoid numeric instability Review of attachment 8474946 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp @@ +716,5 @@ > const double rOldLat = sLastMLSPosLat * radsInDeg; > const double rOldLon = sLastMLSPosLon * radsInDeg; > + // cos(D) = sin(a)sin(b) + cos(a)cos(b)cos(C) > + delta = (std::sin(rNewLat) * std::sin(rOldLat)) + > + (std::cos(rNewLat) * std::cos(rOldLat) * std::cos(rOldLon - rNewLon)); Is not simpler to do: const double cosDelta = <as above> if (cosDelta > 1.0) cosDelta = 1.0; else if (cosDelta < -1.0) cosDelta = -1.0; delta = acos(cosDelta) * earthRadius;

> If we're going to use C++ math functions, we should probably use the > double-precision `M_PI` constant, too. This was not part of the standard historically, so I have always avoided it in cross-platform code.

(In reply to Garvan Keeley [:garvank] from comment #67) > Comment on attachment 8474946 [details] [diff] [review] > Part 4. Don't use acos() to avoid numeric instability > > Review of attachment 8474946 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp > @@ +716,5 @@ > > const double rOldLat = sLastMLSPosLat * radsInDeg; > > const double rOldLon = sLastMLSPosLon * radsInDeg; > > + // cos(D) = sin(a)sin(b) + cos(a)cos(b)cos(C) > > + delta = (std::sin(rNewLat) * std::sin(rOldLat)) + > > + (std::cos(rNewLat) * std::cos(rOldLat) * std::cos(rOldLon - rNewLon)); > > Is not simpler to do: > > const double cosDelta = <as above> > if (cosDelta > 1.0) cosDelta = 1.0; > else if (cosDelta < -1.0) cosDelta = -1.0; > delta = acos(cosDelta) * earthRadius; Ha! How come I didn't think of this? It's so much simpler :)

(In reply to Garvan Keeley [:garvank] from comment #68) > > If we're going to use C++ math functions, we should probably use the > > double-precision `M_PI` constant, too. > > This was not part of the standard historically, so I have always avoided it > in cross-platform code. Bug 776429 added a cross-platform definition of M_PI in mfbt/Constants.h.

Kan-Ru, if you don't mind, I am going to expand on your patch. The logic I had previously added to NetworkGeolocationProvider Update was specifically for short/temporary loss of GPS signal. I didn't realize at the time the code was being relied upon to produce a result in other circumstances. Chris + Kan-Ru, let me know if you think we are moving into a separate bug here. Maybe that depends on how the patch will look. I see now that the GonkGPSProv. needs more logic added to make a decision about whether to return a cached GPS (which we don't store yet) or an MLS position.

(In reply to Garvan Keeley [:garvank] from comment #71) > Chris + Kan-Ru, let me know if you think we are moving into a separate bug > here. Maybe that depends on how the patch will look. Garvan: let's open a new bug (blocking this bug) for your new patch. This bug is already very long and bug numbers are free. :)

I don't think this bug is fixable without a few more lines of logic. What I see fixed is my test case where I disabled GPS explicitly on startup (the no GPS fix case). In the case where GPS drops out after having a fix, the patch as it stands will introduce location jumping, as it needs more conditions before using the MLS location. By jumping I mean: the GPS stops for 10 seconds and the location dot on a map app jumps 5 - 50KM away.

(In reply to Garvan Keeley [:garvank] from comment #62) > > if (provider->mLocationCallback && > (isGpsFullyInactive || > isGpsTempInactive && > delta > kMinMLSCoordChangeInMeters)) > { > provider->mLocationCallback->Update(position); > } My brackets were wrong: if (provider->mLocationCallback && (isGpsFullyInactive || (isGpsTempInactive && delta > kMinMLSCoordChangeInMeters))) { provider->mLocationCallback->Update(position); } We still have the problem that after 10 seconds of no GPS, and an unchanging MLS location, there will be no location response. A solution would be: if (provider->mLocationCallback) { if (isGPSFullyInactive || (isGPSTempInactive && delta > kMinMLSCoordChangeInMeters))) { provider->mLocationCallback->Update(position); } else if (mLastGPSPosition) { // the code doesn't store this yet provider->mLocationCallback->Update(mLastGPSPosition); } } This is not ideal in design, because it relies on the NetworkGeolocationProvider as a timer mechanism (however I think that *is* the intent of the authors of this code). This also assumes that the provider will correctly consider the maximumAge of the mLastGPSPosition, I'll look at that.

This implements Kan-Ru's fixes to Gonk GPS provider, and adds the logic I have been describing for when to use a network location.

> > Bug 776429 added a cross-platform definition of M_PI in mfbt/Constants.h. Forgot to add this, just tried now to update my patch with this, but neither #include "Constants.h" and #include "mfbt/Constants.h" are found during compile.

Comment on attachment 8475571 [details] [diff] [review] Alternate Patch: Step 1, added logic to Gonk GPS provider Review of attachment 8475571 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp @@ +17,5 @@ > #include "GonkGPSGeolocationProvider.h" > > #include <pthread.h> > #include <hardware/gps.h> > +#include <limits> Oops, not used.

(In reply to Garvan Keeley [:garvank] from comment #76) > > > > Bug 776429 added a cross-platform definition of M_PI in mfbt/Constants.h. > > Forgot to add this, just tried now to update my patch with this, but neither > #include "Constants.h" and #include "mfbt/Constants.h" are found during > compile. Sorry. Source file path is mfbt/Constants.h, but #include path is probably "mozilla/Constants.h".

The previous patch required that the Gonk GPS Provider send a cached GPS location. However, nsGeolocationRequest::SendLocation() was not considering the PositionOptions.maximumAge. This was not required previously, because the GPS provider was not sending previous older locations. In the current position request case, the Allow() function will look at the PositionOptions, but in the watch position case, Allow() is only triggered once, and there is no further filtering of positions based on maximumAge.

> > Forgot to add this, just tried now to update my patch with this, but neither > > #include "Constants.h" and #include "mfbt/Constants.h" are found during > > compile. Thanks Chris, will update after review. I am going to try to repro some more failure cases tomorrow (I have to locally patch the code to fake random GPS failures), and review my code more closely. My day is ending, and Kan-Ru's is beginning, so I wanted to get these patches in to get his opinion. Maybe I am missing something in the developer options to turn on and off the GPS. This would help me greatly.

Comment on attachment 8475571 [details] [diff] [review] Alternate Patch: Step 1, added logic to Gonk GPS provider Review of attachment 8475571 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp @@ +740,5 @@ > + // We want to distinguish between the GPS being inactive completely > + // and temporarily inactive. In the former case, we would use a low > + // accuracy MLS, in the latter, we would only want an MLS that appears to > + // updating with movement. > + s/MLS/network location/g @@ +756,5 @@ > + } else if (provider->mLastGPSPosition) { > + if (gGPSDebugging) { > + printf_stderr("geo: Using old GPS age:%fs\n", diff_ms / 1000.0); > + } > + provider->mLocationCallback->Update(provider->mLastGPSPosition); I should have commented more here. The function is called every few seconds. This is a fallback case so that the GPS provider responds with its last location rather than sitting idle waiting for: - a current GPS location, or - a network location that is reporting positional movement Not responding means location timeouts, which is incorrect provider behaviour if we have a recent location. It is up to the service, not the provider, to decide if this old GPS location is relevant (based on the PositionOptions.maximumAge).

Comment on attachment 8475581 [details] [diff] [review] Part 2: SendLocation is not considering GPS age Review of attachment 8475581 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Maybe we should also take the enableHighAccuracy option into account like in Allow() case. Our use of the enableHighAccuracy option isn't optimal. For example we could disable GPS completely if enableHighAccuracy is false. We may also want separated cache for normal and highAccuracy last position.

Comment on attachment 8475571 [details] [diff] [review] Alternate Patch: Step 1, added logic to Gonk GPS provider Review of attachment 8475571 [details] [diff] [review]: ----------------------------------------------------------------- Good! Use nsContentUtils::LogMessageToConsole for logs.

Comment on attachment 8474941 [details] [diff] [review] Part 1. Add try catch around this.wifiService I'll open a new bug for this.

Comment on attachment 8474944 [details] [diff] [review] Part 3. Always use MLS location if not seen anything from the GPS device for 10s I prefer Garvan's solution.

> Looks good. Maybe we should also take the enableHighAccuracy option into account like in Allow() case. The high accuracy is a setup option, rather than a location filter option (I am using SendLocation() as a place to filter locations). In the Allow() case, the service's cached location is considered, and checked to see if it is a high or low accuracy cached position. > Our use of the enableHighAccuracy option isn't optimal. For example we could > disable GPS completely if enableHighAccuracy is false. I agree. I don't think a bug got filed (to say 'implement this!') when this stub code was added https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#836 I'll file one if I can't find a match. > We may also want separated cache for normal and highAccuracy last position. Agreed, I'll file a bug for that. Of course, the Gonk provider doesn't handle accuracy yet.

This code is not working correctly, because GpsUtcTime is 20-30 seconds behind the PR_NOW() time. GPS locations are coming in timestamped 30+ seconds old, and then not meeting the requirement of maximumAge as set in the javascript (I think a reasonable use case for a JS API user would be setting low values for maximumAge like 30sec, which all GPS locations would fail to meet). ni myself, to investigate this tomorrow, and add a bug. Also add bugs for items in comment 86.

(In reply to Garvan Keeley [:garvank] from comment #87) > This code is not working correctly, because GpsUtcTime is 20-30 seconds > behind the PR_NOW() time. GPS locations are coming in timestamped 30+ > seconds old, and then not meeting the requirement of maximumAge as set in > the javascript (I think a reasonable use case for a JS API user would be > setting low values for maximumAge like 30sec, which all GPS locations would > fail to meet). FYI: GPS time is not adjusted for leap seconds, so it is slightly different from UTC time. The diff should be around 16 seconds today (http://leapsecond.com/java/gpsclock.htm). But relying on the device time might also have other problems, as the clocks in phones are frequently out-of-sync.

(In reply to Hanno Schlichting [:hannosch] from comment #88) > But relying on the device time might also have other problems, as the clocks > in phones are frequently out-of-sync. Yes, if we rely on GPS and device time to be in sync, we'll have GPS fail permanently for a ton of people. Also, this might explain why GPS is not working well for quite a few people on Flame recently, as with bug 1048154, time might often be wrong. BTW, I keep wondering if we should provide an option to actually adjust the system time based on GPS signals (corrected for leap seconds, of course).

> working well for quite a few people on Flame recently, as with bug 1048154, > time might often be wrong. Its ok as long as the time is internally consistent, if everything in the chain uses current system time. The time issue came when I was fixing this bug, and in the process, introduced code that looked at the GPS provider's location timestamp.

An update to the previous patch to add comments, address my comments (remove limits.h, use M_PI), most importantly to not use the GPS satellite timestamp on the location provider's current position, and use the system time instead (which is the correct thing to use to be compared to the DOMTimestamp used in the service level). Kan-Ru, thanks for the previous r+, I'll r? you for this updated patch.

Comment on attachment 8477024 [details] [diff] [review] Part 2: SendLocation is not considering GPS age This line got changed from the previous patch: if (mIsWatchPositionRequest && mOptions && mOptions->mMaximumAge > 0) Removed the mIsWatchPositionRequest. I was thinking that in the currentPosition case this would already have been filtered for max age, but that is not the case. Any position handled by SendLocation should be filtered for max age if it is set.

See previous comment 91, uploaded the wrong patch, this is the right one.

Comment on attachment 8477035 [details] [diff] [review] Part 1 -dded-mls-gps-logic.patch Review of attachment 8477035 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp @@ +751,5 @@ > + // location that appears to updating with movement. > + > + const bool isGPSFullyInactive = diff_ms > 1000 * 60 * 2; // two mins > + const bool isGPSTempInactive = diff_ms > 1000 * 10; // 10 secs > + nit: trailing whitespace

Thanks for the review Kan-Ru! (removed extra ws, carrying over r+)

Is there a recent Try link for this? :)

Thanks for the reminder, trys are green: https://tbpl.mozilla.org/?tree=Try&rev=9a5ee5639aec https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9a5ee5639aec https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/gkeeley@mozilla.com-9a5ee5639aec

Previous comment was the build Trys, for good measure, running the geo tests for browser+mochitests (this patch is for GPS code, which we don't have automated tests for, but good to try geo tests anyway): https://tbpl.mozilla.org/?tree=Try&rev=b041950e109f

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd0b358f86fc https://hg.mozilla.org/integration/mozilla-inbound/rev/87f9cc55a569

https://hg.mozilla.org/mozilla-central/rev/cd0b358f86fc https://hg.mozilla.org/mozilla-central/rev/87f9cc55a569

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ea26b8bd548c https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9e9d45159989 Note that this still needs to go through the regular b2g32 approval process if it's wanted for v2.0.

Ryan: I can't find a b2g32 approval request flag in bugzilla, what do I set for this? [Feature/regressing bug #]: triggered by bug 996998 (required to stabilize the GPS location from being overwritten with a less-accurate network location). There was insufficient logic when deciding whether GPS or Network-based location should be used, and as a result no location gets sent. [User impact if declined]: On devices with GPS and Mozilla geo stack: this bug, bug 1042779. I need to notify the FMD group of this bug landing, so they can retest outstanding Find My Device geo bugs, because I think this bug is the root cause. [Describe test coverage new/current, TBPL]: Manual. Load google maps and reload the page a few times, with the bug, the blue location dot will disappear. I use the added debugging feature from bug 1056857 (not landed yet) in my build to provide full coverage of the logic that was added in this code. [Risks and why]: Mozilla geo stack devices that have GPS will have intermittent geolocation without this fix. Medium risk because of lack of automated testing: we lack a GPS simulation test that would replace the hardware layer with a proxy interface, and manual GPS testing is not deterministic (indoor vs. outdoor, city vs. country, etc.). [String/UUID change made/needed]: none

approval‑mozilla‑b2g32? on the patch(es)

Comment on attachment 8477413 [details] [diff] [review] Part 1: added mls+gps logic.patch see comment 105

Comment on attachment 8477024 [details] [diff] [review] Part 2: SendLocation is not considering GPS age see comment 105

bajaj: we need this fix in 2.0 because Flame devices running 2.0 use Mozilla's geo stack, not CAF's.

Discussed offline with cpeterson/garvank, we are going to land this on 2.0M as the possibility of hitting these bugs on 2.0 production device are close to null and given we are close to completion on 2.0 and not taking any non-blockers. If QA finds a FMD/geo-location based issue, we recommend trying 2.1/2.0M to see if those are affected since these branches have our full geo stack.

:khu can you please ensure this lands on 2.0M? Not sure if we have a dedicated approval flag for that or what the process is, so can you help on those?

Chris already marked this with blocking-b2g:2.0M? Clean the ni here. Thank you, Chris.

(In reply to Kevin Hu [:khu] from comment #112) > Chris already marked this with blocking-b2g:2.0M? Clean the ni here. > Thank you, Chris. Hi Kevin, Does moving a bug to 2.0M? guarantee landing on the branch ? Can you please share the branch details that this will get uplifted on ?

(In reply to bhavana bajaj [:bajaj] from comment #113) > (In reply to Kevin Hu [:khu] from comment #112) > > Chris already marked this with blocking-b2g:2.0M? Clean the ni here. > > Thank you, Chris. > > Hi Kevin, Does moving a bug to 2.0M? guarantee landing on the branch ? Can > you please share the branch details that this will get uplifted on ? Not exactly. Just like what we did before, we will have triage for 2.0M? bugs, and decide what should be +'ed. Once a bug is +'ed, it should be landed to 2.0M.

according to triage result, 2.0M+

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/c5cb8e2eef21 http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/fef39bbff319

Hi Thomas, Could you please provide the detailed reproduce steps for me to verify this bug.

Hi Hubert, Please help confirm whether these steps correctly, thank you. STR: 1. Open browser and input "google maps". 2. Choose "Share" in App Permission. **GPS works normally in Browser/google maps. 3. Download the "zMaps" from Marketplace app and then open it. 4. Choose "Share" in App Permission. **GPS works normally in zMaps app. This issue has been successfully verified on Flame 2.1&2.0 through the above steps. See attachment: verified_v2.1.mp4. Reproduce rate: 0/5 Flame 2.1 build: Gaia-Rev 38e17b0219cbc50a4ad6f51101898f89e513a552 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a Build-ID 20141205001201 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141205.035305 FW-Date Fri Dec 5 03:53:16 EST 2014 Bootloader L1TC00011880 Flame 2.0 build: Gaia-Rev 856863962362030174bae4e03d59c3ebbc182473 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e40fe21e37f1 Build-ID 20141207000206 Version 32.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141207.034341 FW-Date Sun Dec 7 03:43:52 EST 2014 Bootloader L1TC00011880

Hi Reporter, Could you help confirm whether the steps are correct? Thank you.

Hi Kai-Zhen: Please help. Thanks!

STR: - open a online map in browser, e.g: google map - click my location - accept and share location Expected result: your location should be displayed on map and do not disappear.

(In reply to Kai-Zhen Li [:seinlin] from comment #122) > STR: > - open a online map in browser, e.g: google map > - click my location > - accept and share location > > Expected result: your location should be displayed on map and do not > disappear. Thanks!

This issue has been successfully verified on Flame v2.1 eng build and Woodduck 2.0 eng build. See attachment: verified_v2.1.mp4. Reproduce rate: 0/4. STR: 1.Connect wifi. 2.Open a online map in Browser, such as "google map". 3.Click my location. 4.Accept and share location. **My location displays on map and does not disappear. 5.Open zmaps downloaded from marketplace. **My location displays on map and does not disappear. 6.When user open Flame's data network -> open a map(e.g:"Gaode map") in Browser -> tap "Nearby" -> Select a place -> tap "Go here",the location displays on map. Note:The Woodduck's data network "2G" is so slowly that map can't display location. Woodduck 2.0 build: Gaia-Rev add38992bbfb2bafca52ac1ce7f6231ac702675f Gecko-Rev 8d951d9c4988e218ec08c6118a35c7faeb70af49 Build-ID 20141225143517 Version 32.0 Device-Name jrdhz72_w_ff FW-Release 4.4.2 FW-Incremental 1419478053 FW-Date Thu Dec 25 11:27:57 CST 2014 Flame 2.1 build: Gaia-Rev 73be51f998031f06db0cd660c0e388fa621c9f4c Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ea426e47bfc4 Build-ID 20141228001253 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141228.034827 FW-Date Sun Dec 28 03:48:38 EST 2014