1035097 - Network location providers sends multiple different radio types

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Hanno Schlichting]

Attachment: geo-debug.log

+++ This bug was initially created as a clone of Bug #1010345 +++

This is a follow-up from the last comment from Bug #1010345.

We found a new issue in current FxOS master on a Flame device. We recently added code to scan for cells on all available SIM cards. This can lead to a situation where one SIM card (radio interface) returns a wcdma cell and the other a gsm cell. In the future this could be different combinations of gms, wcdma, lte or even cdma cells.

The official API only allows sending one radioType per request, so sending two of them isn't actually supported.

We have a workaround for #1010345 which allows sending the radio type in each cell entry. The service code currently checks that we get a consistent radio type across all cell entries and rejects a request with inconsistent radio types.

The current result is that geolocation lookups on Flame are often broken.

Things to figure out:

1. Does this only apply to Flame / master? Or when did we ship the "multi-SIM" support code? Does this affect Dolphin / 1.4?

Possible solutions:

1. Extend the service side workaround code even more and allow inconsistent radio types. This could be a workaround if this affects 1.4.
2. Fix #1010345 which needs to deal with consolidating multiple radio types in some way.

Comment 1

[Hanno Schlichting]

As a short-term workaround, I extended the service-side workaround to allow multiple different radio types. This was committed (https://github.com/mozilla/ichnaea/commit/af00d742a6c26be3ad81defaa7d5f6d123f0555e) and pushed live already.

This should make sure Flame devices continue to work for the moment. But it's still a workaround.

Comment 2

[:garvan]

This isn't a terrible workaround by any means, and is only a workaround in the sense that it no longer matches the non-canonical json spec for Google geolocation queries. We should either:
1) be explicit that we aren't following the spec, and follow Hanno's suggestion of clearly labeling the JSON query with a version/type header
2) Communicate with Google on whether they are updating their JSON queries to support the this case (which is a rather common case, so they may have some unpublished spec they are following to support this).

Comment 3

[Hanno Schlichting]

The workaround is actually fine, we and other location services are using and supporting this now. There's just one minor fix left. At https://dxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#447 instead of:

result.push({ radio: radioTechFamily,

it should use:

result.push({ radioType: radioTechFamily,

Comment 4

[Josh Matthews [:jdm]]

Very simple fix as described in comment 3. No easy way to test it without a device and extra logging code, as far as I can tell.

Comment 5

[Anup Kumar [:reznord]]

Attachment: Changed the radio type from "radio" to "radioType"

Hi, 

I have changed the radio type from "radio" to "radioType" in NetworkGeolocationProvider.js. 

Can you please assign this bug to me?

Comment 6

[Hanno Schlichting]

Comment on attachment 8636559 [details] [diff] [review]
Changed the radio type from "radio" to "radioType"

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

Looks good from a service side perspective.

Comment 7

[Anup Kumar [:reznord]]

(In reply to Hanno Schlichting [:hannosch] from comment #6)
> Comment on attachment 8636559 [details] [diff] [review]
> Changed the radio type from "radio" to "radioType"
> 
> Review of attachment 8636559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good from a service side perspective.

Can you just put up a pull request for the patch?

Comment 8

[Hanno Schlichting]

(In reply to Anup Kumar from comment #7)
> Can you just put up a pull request for the patch?

I don't know how to do that myself.

Also we need a review from one of the Core:Geo module owners or peers. I can just give a feedback+ (f+) but not a review+ (r+).

I'm hoping jdm wakes up later and can take care of it. Otherwise we'll have to wait on Garvan, who is out this week.

Comment 9

[Josh Matthews [:jdm]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/ddfcd5c87f76700e388f712ecd4825f1d6170775
changeset:  ddfcd5c87f76700e388f712ecd4825f1d6170775
user:       "Anup Allamsetty"
date:       Tue Jul 21 06:43:00 2015 -0400
description:
Bug 1035097 - Changed the type from 'radio' to 'radioType'. r=jdm

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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