1200932 - B2G RIL: consider reporting unavailable state or retriggering (non-default) data call

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

This is a follow-up to Bug 1179097.

There are cases when non-default data call is not able to recover from "disconnected" state, e.g.:
- after retry times reaches NETWORK_APNRETRY_MAXRETRIES (10)
- when data call is dropped, and data registration is unregistered [1]

We should consider reporting "unavailable" state to upper layer or discuss whether re-triggering data call is needed.

Note that we will face the same problem when doing NetworkManager Enhancement: when should we reconnect non-default data automatically? (e.g. when radio turns off and on, when data registration is lost and back). If there are cases that we do not reconnect them automatically, then we must report an "unavailable" state.

[1] http://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/file/03dd969850a0/dom/system/gonk/RadioInterfaceLayer.js#l2944

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

In this patch, we'll report 'unavailable' state after reaching the maximum retry times (10).
Currently, after data registration is recovered, only default data call will be retriggered, in this patch, we've also added a way to retrigger non-default data calls.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8658625 [details] [diff] [review]
patch, v1.

Edgar, may I have your review on this? Thanks.

Comment 3

[Edgar Chen [:edgar]]

Comment on attachment 8658625 [details] [diff] [review]
patch, v1.

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

Looks good. Just one thing, is it possible to add some tests for data registration changes (if emulator can support such behaviour)?
Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3213,5 @@
>    get connected() {
>      return this.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED;
>    },
>  
> +

Nit: Remove this empty line.

::: dom/system/gonk/nsINetworkManager.idl
@@ +31,5 @@
>    const long REASON_NONE              = 0;
>    const long REASON_RADIO_SHUTDOWN    = 1;
>    const long REASON_APN_CHANGED       = 2;
>    const long REASON_SERVICEID_CHANGED = 3;
> +  const long REASON_RETRY_FAILED      = 4;

Need an uuid bump?

Comment 4

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar!

(In reply to Edgar Chen [:edgar][:echen] from comment #3)
> Comment on attachment 8658625 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8658625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Just one thing, is it possible to add some tests for data
> registration changes (if emulator can support such behaviour)?
> Thank you.

Actually, we do test this in test_data_call_state_events.js [1], however this wasn't caught due to some timing issues. If we want to test this properly, we'll need to add some delay (using setTimeout() maybe) between tests, but I am not sure if it's a good thing to do.

[1] http://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/file/86954a963254/dom/datacall/tests/marionette/test_data_call_state_events.js#l102

> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +3213,5 @@
> >    get connected() {
> >      return this.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED;
> >    },
> >  
> > +
> 
> Nit: Remove this empty line.

Will do.

> 
> ::: dom/system/gonk/nsINetworkManager.idl
> @@ +31,5 @@
> >    const long REASON_NONE              = 0;
> >    const long REASON_RADIO_SHUTDOWN    = 1;
> >    const long REASON_APN_CHANGED       = 2;
> >    const long REASON_SERVICEID_CHANGED = 3;
> > +  const long REASON_RETRY_FAILED      = 4;
> 
> Need an uuid bump?

yes, thanks for the reminder.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: patch. r=echen

- Address review comment 3
- Added new reason (REASON_PERMANENT_FAILURE) which is set when data call setup fails with permanent fail cause

Comment 6

[Jessica Jong [:jessica] (inactive)]

This is a follow-up to Bug 1179097, so should be merged into v2.2r.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

2.2r+ due to comment 6

Comment 8

[Jessica Jong [:jessica] (inactive)]

try looks good except for some known failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a083acb6d2a&group_state=expanded

checkin-needed for mozilla-b2g37_v2_2r.

Comment 9

[Carsten Book [:Tomcat]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #8)
> try looks good except for some known failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9a083acb6d2a&group_state=expanded
> 
> checkin-needed for mozilla-b2g37_v2_2r.

done :) https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/68a9516194fe