1032063 - Call update_network_state and update_network_availability when network state changes for A-GPS

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Reference:

http://androidxref.com/4.4.3_r1.1/xref/frameworks/base/services/jni/com_android_server_location_GpsLocationProvider.cpp#664

Notify the gps engine of the network availability.

Comment 1

[Chris Peterson [:cpeterson]]

Doug: is this a Dolphin-only bug? If SPRD doesn't provide SUPL servers, how will A-GPS work on Dolphin?

Comment 2

[Chris Peterson [:cpeterson]]

bajaj wants to know if this bug will affect QC devices.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Call update_network_state and update_network_availibility

Tentative patch. Someone has to test and finish this patch since I don't have a 3G SIM card currently. Or I'll do it when I get one.

Comment 4

[Chris Peterson [:cpeterson]]

SPRD says the first launch of Dolphin will not have GPS (or A-GPS). Subsequent launches of Dolphins will have GPS and A-GPS, so we will want this patch for 1.4.

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 8448637 [details] [diff] [review]
Call update_network_state and update_network_availibility

Garvan: do you have a compatible device you can test Kan-Ru's patch with?

Kan-Ru: Is Dolphin the only device with a RIL that initializes the mAGpsRilInterface->update_network_availability function pointer?

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Chris Peterson (:cpeterson) from comment #5)
> Kan-Ru: Is Dolphin the only device with a RIL that initializes the
> mAGpsRilInterface->update_network_availability function pointer?

I tested on a Unagi and it also has those two function pointers.

Comment 7

[:garvan]

Can't build ATM: bug 1033421

Comment 8

[:garvan]

Comment on attachment 8448637 [details] [diff] [review]
Call update_network_state and update_network_availibility

Clearing. Set up a fresh linux install, same build problem. Tried irc, mailing list, no resolution.

Comment 9

[Chris Peterson [:cpeterson]]

Kan-Ru: you wrote the attached patch, so I assume that you are working on this bug. If I am mistaken, please feel free to unassign yourself from this bug. :)

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

To back port this patch to 1.4 we will have to cherry-pick the patch 2.b/6 from bug 978709.

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 1. Bug 978709 - 2.b/6 patch for v1.4

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 2. Call update_network_state patch for v1.4

Comment 13

[Ivan Tsay (:ITsay)]

Hi James,

Could you help to find your engineer to test the patch specified in comment 12 and see if the flow is what you expected. This is the follow-up from the discussion regarding the AGPS flow questions.

Comment 14

[Guang.Yu]

(In reply to Kan-Ru Chen [:kanru] from comment #12)
> Created attachment 8453581 [details] [diff] [review]
> Part 2. Call update_network_state patch for v1.4

Hi Kan-Ru:
could you please send me the source file of GonkGPSGeolocationProvider.cpp and GonkGPSGeolocationProvider.h, then I can replace the old ones, and have a test. I can not git apply your patch sucessfully, which may caused by different baseline.
Thanks.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

I've sent the sources to Guang.

Comment 16

[Guang.Yu]

(In reply to Kan-Ru Chen [:kanru] from comment #15)
> I've sent the sources to Guang.

Hi Kan-Ru:
I have tried this new GonkGPSGeolocatonProvider, but it seems still have problems. The steps I test is below:
0. enable network connection based on SIM1 in settings.
1. turn on GPS app, and trigger GPS module running.
2. disable network connection in settings.
3. wait for a moment
4. enable network connection.
5. turn off GPS app.

Problem:
1.
I think for mozilla design, both step 2 and step 4 should call update_network_state to inform GPS module the network state, but I just see step 2 call this interface but not step 4.

2.
could you please help to check why use a new thread to SetReferenceLocation? For Android, while GPS module request reference location, framework will collect reference location information and set these information to GPS module, then go ahead. All of them is excuted sequentially. But FFOS use a thread to collect information. Is this thread nesscessy, is it can be removed?

Thanks.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8448637 [details] [diff] [review]
Call update_network_state and update_network_availibility

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +876,5 @@
> +      iface->GetType(&type);
> +      bool connected = (state == nsINetworkInterface::NETWORK_STATE_CONNECTED);
> +      bool roaming = false;
> +      int gpsNetworkType = ConvertToGpsNetworkType(type);
> +      if (gpsNetworkType > 0) {

gpsNetworkType >= 0

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 2. Call update_network_state patch for v1.4

Update

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

Hi,

I updated the patch 2, please try again.
 
> Problem:
> 1.
> I think for mozilla design, both step 2 and step 4 should call
> update_network_state to inform GPS module the network state, but I just see
> step 2 call this interface but not step 4.

Should be fixed. Note that the mobile network connection might only up for a short time in order to get SUPL information if wifi is also enabled. However as long as the GPS is running we should get notification of network state change.

> 2.
> could you please help to check why use a new thread to SetReferenceLocation?
> For Android, while GPS module request reference location, framework will
> collect reference location information and set these information to GPS
> module, then go ahead. All of them is excuted sequentially. But FFOS use a
> thread to collect information. Is this thread nesscessy, is it can be
> removed?

It does not use a separate thread. In fact it works similar to Android, all requests are redirected to the main thread so them could be executed sequentially. Let me know if something is not clear :)

Comment 21

[Guang.Yu]

Hi Kan-Ru:
Could you please send me the source files? Then, i'll try it.
Thanks

Comment 22

[Guang.Yu]

(In reply to Kan-Ru Chen [:kanru] from comment #20)
> Hi,
> 
> I updated the patch 2, please try again.
>  
> > Problem:
> > 1.
> > I think for mozilla design, both step 2 and step 4 should call
> > update_network_state to inform GPS module the network state, but I just see
> > step 2 call this interface but not step 4.
> 
> Should be fixed. Note that the mobile network connection might only up for a
> short time in order to get SUPL information if wifi is also enabled. However
> as long as the GPS is running we should get notification of network state
> change.
> 
> > 2.
> > could you please help to check why use a new thread to SetReferenceLocation?
> > For Android, while GPS module request reference location, framework will
> > collect reference location information and set these information to GPS
> > module, then go ahead. All of them is excuted sequentially. But FFOS use a
> > thread to collect information. Is this thread nesscessy, is it can be
> > removed?
> 
> It does not use a separate thread. In fact it works similar to Android, all
> requests are redirected to the main thread so them could be executed
> sequentially. Let me know if something is not clear :)

Hi Kan-Ru：
This time, I can see update_network_state can be called while network state was changeing. but data_conn_open/close and update_network_availability are wrong. 
It seems like both enable and disable network connection is triggered, the condition: 
connectionState == nsINetworkInterface::NETWORK_STATE_CONNECTED 
always false, but connectionState == nsINetworkInterface::NETWORK_STATE_DISCONNECTED is always true.
Please help to check.
Thanks

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Guang.Yu from comment #22)
> (In reply to Kan-Ru Chen [:kanru] from comment #20)
> > Hi,
> > 
> > I updated the patch 2, please try again.
> >  
> > > Problem:
> > > 1.
> > > I think for mozilla design, both step 2 and step 4 should call
> > > update_network_state to inform GPS module the network state, but I just see
> > > step 2 call this interface but not step 4.
> > 
> > Should be fixed. Note that the mobile network connection might only up for a
> > short time in order to get SUPL information if wifi is also enabled. However
> > as long as the GPS is running we should get notification of network state
> > change.
> > 
> > > 2.
> > > could you please help to check why use a new thread to SetReferenceLocation?
> > > For Android, while GPS module request reference location, framework will
> > > collect reference location information and set these information to GPS
> > > module, then go ahead. All of them is excuted sequentially. But FFOS use a
> > > thread to collect information. Is this thread nesscessy, is it can be
> > > removed?
> > 
> > It does not use a separate thread. In fact it works similar to Android, all
> > requests are redirected to the main thread so them could be executed
> > sequentially. Let me know if something is not clear :)
> 
> Hi Kan-Ru：
> This time, I can see update_network_state can be called while network state
> was changeing. but data_conn_open/close and update_network_availability are
> wrong. 
> It seems like both enable and disable network connection is triggered, the
> condition: 
> connectionState == nsINetworkInterface::NETWORK_STATE_CONNECTED 
> always false, but connectionState ==
> nsINetworkInterface::NETWORK_STATE_DISCONNECTED is always true.
> Please help to check.
> Thanks

With my test I got nsINetworkInterface::NETWORK_STATE_CONNECTED then nsINetworkInterface::NETWORK_STATE_DISCONNECTED

Steps:
1. Disable wifi
2. Enable 3G
3. Open Geoloc app
4. Touch 'Start'

I think it is correct. Could you confirm that calling update_network_availability before we call data_conn_open/close is the expected sequence?

Comment 24

[Guang.Yu]

(In reply to Kan-Ru Chen [:kanru] from comment #23)

> 
> With my test I got nsINetworkInterface::NETWORK_STATE_CONNECTED then
> nsINetworkInterface::NETWORK_STATE_DISCONNECTED
> 
> Steps:
> 1. Disable wifi
> 2. Enable 3G
> 3. Open Geoloc app
> 4. Touch 'Start'
> 
> I think it is correct. Could you confirm that calling
> update_network_availability before we call data_conn_open/close is the
> expected sequence?

Hi Kan-Ru:
yes. please call update_network_availability before call data_conn_open/close.

1. I see there is a function named Observe, I think it's used for monitor network state, right?
with my test, I can see Observe works well, and can call update_network_state while network changed.
But, update_network_availability is not called like update_network_state. The two functions update_network_availability and data_conn_open/close is wrapped in SetAGpsDataConn which executed in Handle. I don't know if there is dependency between Observe and Handle.
2. while used 3G sim card to test, I see update_network_availability(false, apn.get()) and data_conn_closed are always to be called even network connection is enabled.
Please help.
Thanks

Comment 25

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Guang.Yu from comment #24)
> yes. please call update_network_availability before call
> data_conn_open/close.

Thanks.

> 1. I see there is a function named Observe, I think it's used for monitor
> network state, right?

Yes.

> with my test, I can see Observe works well, and can call
> update_network_state while network changed.
> But, update_network_availability is not called like update_network_state.
> The two functions update_network_availability and data_conn_open/close is
> wrapped in SetAGpsDataConn which executed in Handle. I don't know if there
> is dependency between Observe and Handle.

Currently we only call SetAGpsDataConn when the GPS engine asks data connection via the callback. Handle() is a callback for the "ril.supl.apn" settings value. We could only setup gps connection when we have known supl apn.

Pseudo code stack:

AGPSStatusCallback()
  provider->RequestDataConnection()
    if (NETWORK_STATE_CONNECTED)
      RequestSettingValue("ril.supl.apn") <==
    else
      SetupDataCallByType("supl")
        Observe(NETWORK_STATE_CONNECTED)
          RequestSettingValue("ril.supl.apn") <==

Handle("ril.supl.apn") <==
  SetAGpsDataConn(apn)

> 2. while used 3G sim card to test, I see update_network_availability(false,
> apn.get()) and data_conn_closed are always to be called even network
> connection is enabled.

That maybe another problem because I could see data_conn_open/closed pair here.

If the call sequence I described above is expected then I intend to land this patch as is. If we found other problem on Dolphin we could fix that in a follow up bug.

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Call update_network_state and update_network_availibility

Comment 27

[Doug Turner (:dougt)]

Comment on attachment 8464595 [details] [diff] [review]
Call update_network_state and update_network_availibility

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +345,5 @@
> +    // 2. The device is connected to a foreign network and data
> +    //    roaming is enabled
> +    // However, whether data roaming is enabled is not controlled by
> +    // gecko so we assume we are allowed to use data when the
> +    // connection state is connected.

The sentence starting with 'However' doesn't make sense.

@@ +346,5 @@
> +    //    roaming is enabled
> +    // However, whether data roaming is enabled is not controlled by
> +    // gecko so we assume we are allowed to use data when the
> +    // connection state is connected.
> +    if (mAGpsRilInterface->size >= sizeof(AGpsRilInterface) &&

Can we move this sizeof safety check up out of these if statements, and bail from SetAgpsDataConn early?

@@ +847,5 @@
> +      return AGPS_RIL_NETWORK_TYPE_MOBILE_SUPL;
> +    case nsINetworkInterface::NETWORK_TYPE_MOBILE_DUN:
> +      return AGPS_RIL_NETWORK_TTYPE_MOBILE_DUN;
> +    case nsINetworkInterface::NETWORK_TYPE_WIFI_P2P:
> +    case nsINetworkInterface::NETWORK_TYPE_MOBILE_IMS:

Do you want to remove the above two lines and just let default take care of it?

@@ +890,5 @@
> +              voice->GetRoaming(&roaming);
> +            }
> +          }
> +        }
> +        mAGpsRilInterface->update_network_state(

do you want to call this if rilface is null?

If not, move the if check at line 902 to line 874.

Comment 28

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Doug Turner (:dougt) from comment #27)
> Comment on attachment 8464595 [details] [diff] [review]
> Call update_network_state and update_network_availibility
> 
> Review of attachment 8464595 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +345,5 @@
> > +    // 2. The device is connected to a foreign network and data
> > +    //    roaming is enabled
> > +    // However, whether data roaming is enabled is not controlled by
> > +    // gecko so we assume we are allowed to use data when the
> > +    // connection state is connected.
> 
> The sentence starting with 'However' doesn't make sense.

How about

"RIL turns on/off data connection automatically when the data roaming setting changes."
 
> @@ +346,5 @@
> > +    //    roaming is enabled
> > +    // However, whether data roaming is enabled is not controlled by
> > +    // gecko so we assume we are allowed to use data when the
> > +    // connection state is connected.
> > +    if (mAGpsRilInterface->size >= sizeof(AGpsRilInterface) &&
> 
> Can we move this sizeof safety check up out of these if statements, and bail
> from SetAgpsDataConn early?

Can't bail out early because we still want to call mAGpsInterface->data_conn_{open,closed}
I could change it to

bool hasUpdateNetworkAvailability = false;
if (mAGpsRilInterface->size >= sizeof(AGpsRilInterface) &&
        mAGpsRilInterface->update_network_availability) {
  hasUpdateNetworkAvailability = true;
}

if (hasUpdateNetworkAvailability) {
  mAGpsRilInterface->update_network_availability(true, apn.get());
}

> @@ +847,5 @@
> > +      return AGPS_RIL_NETWORK_TYPE_MOBILE_SUPL;
> > +    case nsINetworkInterface::NETWORK_TYPE_MOBILE_DUN:
> > +      return AGPS_RIL_NETWORK_TTYPE_MOBILE_DUN;
> > +    case nsINetworkInterface::NETWORK_TYPE_WIFI_P2P:
> > +    case nsINetworkInterface::NETWORK_TYPE_MOBILE_IMS:
> 
> Do you want to remove the above two lines and just let default take care of
> it?

OK.

> @@ +890,5 @@
> > +              voice->GetRoaming(&roaming);
> > +            }
> > +          }
> > +        }
> > +        mAGpsRilInterface->update_network_state(
> 
> do you want to call this if rilface is null?
> 
> If not, move the if check at line 902 to line 874.

Actually I want. From the emails from the partner I think the GPS engine could still do something when the network type is not mobile/ril and the engine want to know the network state.

Comment 29

[Kan-Ru Chen [:kanru] (UTC+9)]

Hi Doug, are you OK with the said changes?

Comment 30

[Doug Turner (:dougt)]

sgtm.  r+ with said changes.

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/051fd57f4fef

Comment 32

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 1. Bug 978709 - 2.b/6 patch for v1.4

refresh

Comment 33

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 2. Call update_network_state patch for v1.4

update

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/051fd57f4fef

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9b74a5a1b0b2
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/cc0a3305b23e

Note that due to the recent policy changes announced on dev-gaia, 1.4 blockers don't have auto-approval to land on v2.0 anymore, so you'll need to explicitly request b2g32 approval on this patch if it's wanted for that release as well. Apologies for the inconvenience :(

Comment 36

[Chris Peterson [:cpeterson]]

Comment on attachment 8468258 [details] [diff] [review]
Part 2. Call update_network_state patch for v1.4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: This bug is a 1.4+ blocker that we'd like to fix on 2.0. This fix will improve geolocation and FMD on non-QC devices. (QC replaces our Gecko geolocation code with their own.)
Testing completed: Dolphin running 1.4
Risk to taking this patch (and alternatives if risky): Possible GPS confusion by network state changes if there are bugs in Kan-Ru's patch or the device's GPS or network hardware?
String or UUID changes made by this patch: None

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This reliably crashes my Nexus 5 when doing GPS with a null deref at http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/GonkGPSGeolocationProvider.cpp#341.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8468258 [details] [diff] [review]
Part 2. Call update_network_state patch for v1.4

Going out on a limb that we don't want this landing anywhere else until the various known regressions are fixed.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I would kind of like it to be backed out of central until the regressions are fixed so I could have working GPS again :P

Comment 40

[Ryan VanderMeulen [:RyanVM]]

I guess this is ready for b2g32 nomination again now if it's still wanted. Be sure to nominate bug 1050899 as well if you do.

Comment 41

[Chris Peterson [:cpeterson]]

Comment on attachment 8468258 [details] [diff] [review]
Part 2. Call update_network_state patch for v1.4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression.
User impact if declined: This bug is a 1.4+ blocker that we'd like to fix on 2.0. This fix will improve geolocation and FMD on non-QC devices. (QC replaces our Gecko geolocation code with their own.)
Testing completed: Dolphin running 1.4
Risk to taking this patch (and alternatives if risky): Possible GPS confusion by network state changes if there are bugs in Kan-Ru's patch or the device's GPS or network hardware?
String or UUID changes made by this patch: None

Comment 42

[bhavana bajaj [:bajaj]]

:Khu, can you please make sure that this gets landed on 2.0M branch ?

Comment 43

[Kai-Chih Hu [:khu]]

Kai-Zhen, can you help to merge this to 2.0M branch? I am not sure if this is something you can do. If not, please tell me. 

Thanks.

Comment 44

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/d0b0ef2135aa

Comment 45

[bhavana bajaj [:bajaj]]

Comment on attachment 8468258 [details] [diff] [review]
Part 2. Call update_network_state patch for v1.4

Clearing the b2g_32 nom given we are very close to shipping 2.0 and this does not block that release, although we've landed this on 2.0 M and 2.1