If we have one data call connected and connect the 2nd data call, the state of the 1st data call will be changed to GECKO_NETWORK_STATE_DISCONNECTED for a short period and changed back to GECKO_NETWORK_STATE_CONNECTED. This temporary state change caused problem to bug 801540. The function _processDataCallList() in ril_worker.js is called twice when setting up a data call. 1) When receiving REQUEST_SETUP_DATA_CALL parcel, _processDataCallList() is called and the datacalls object contains only the new data call been setup. At this time, the state of previous data call will be changed to GECKO_NETWORK_STATE_DISCONNECTED. 2) When receiving UNSOLICITED_DATA_CALL_LIST_CHANGED parcel, _processDataCallList() is called again, and this time the datacalls object contains complete data list. The state of previous data call will be changed back to GECKO_NETWORK_STATE_CONNECTED.

If current data call is not in datacalls list, don't change current data call state. Per testing, this fixed the temporary state change issue.

This bug blocks the other blocking-basecamp+ bug.

(In reply to Shian-Yow Wu from comment #1) > If current data call is not in datacalls list, don't change current data > call state. Yeah, but then how do we ever detect when a datacall is dropped? Do we get a notification for that?

Also, why would the current data call *not* be in the datacalls list? That doesn't make sense to me.

(In reply to Philipp von Weitershausen [:philikon] from comment #3) > > Yeah, but then how do we ever detect when a datacall is dropped? Do we get a > notification for that? Yes, the 2nd update of datacalls list containing 20 data call entries, and the dropped datacall has ifname with null. It will remove the data call from currentDataCalls by: https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#3436 (In reply to Philipp von Weitershausen [:philikon] from comment #4) > Also, why would the current data call *not* be in the datacalls list? That > doesn't make sense to me. Sorry to make you confuse. The 'current data call' here means currentDataCalls list. The list contains previously established data calls. The 1st update of datacalls list from REQUEST_SETUP_DATA_CALL contains only one entry for new established data call. It doesn't intend to remove previously established data calls from currentDataCalls list.

(In reply to Philipp von Weitershausen [:philikon] from comment #3) > (In reply to Shian-Yow Wu from comment #1) > > If current data call is not in datacalls list, don't change current data > > call state. > > Yeah, but then how do we ever detect when a datacall is dropped? Do we get a > notification for that? Slightly update to previous comment. There are two situations to drop a data call. Normally we hit the first situation when a deactivate data call request was sent. 1. In REQUEST_DEACTIVATE_DATA_CALL response https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#3436 2. In UNSOLICITED_DATA_CALL_LIST_CHANGED response https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#4537

Blocks a blocker.

(In reply to Shian-Yow Wu from comment #5) > (In reply to Philipp von Weitershausen [:philikon] from comment #3) > > > > Yeah, but then how do we ever detect when a datacall is dropped? Do we get a > > notification for that? > > Yes, the 2nd update of datacalls list containing 20 data call entries, and > the dropped datacall has ifname with null. It will remove the data call > from currentDataCalls by: > https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker. > js#3436 How does this 2nd update happen? Via UNSOLICITED_DATA_CALL_LIST_CHANGED? > (In reply to Philipp von Weitershausen [:philikon] from comment #4) > > Also, why would the current data call *not* be in the datacalls list? That > > doesn't make sense to me. > > Sorry to make you confuse. The 'current data call' here means > currentDataCalls list. The list contains previously established data calls. > The 1st update of datacalls list from REQUEST_SETUP_DATA_CALL contains only > one entry for new established data call. It doesn't intend to remove > previously established data calls from currentDataCalls list. Ok. Then we should pass a flag to _processDataCallList to indicate whether it's coming from REQUEST_SETUP_DATA_CALL or REQUEST_DATA_CALL_LIST/UNSOLICITED_DATA_CALL_LIST_CHANGED. When it's from REQUEST_SETUP_DATA_CALL, we never drop any current data calls. When it's from the other ones, we do. Does that sound good? (In reply to Shian-Yow Wu from comment #6) > Slightly update to previous comment. There are two situations to drop a > data call. Normally we hit the first situation when a deactivate data call > request was sent. > > 1. In REQUEST_DEACTIVATE_DATA_CALL response > https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker. > js#3436 This when we drop the data call. > 2. In UNSOLICITED_DATA_CALL_LIST_CHANGED response > https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker. > js#4537 This is when the modem drops the data call. I'm worried about no longer handling UNSOLICITED_DATA_CALL_LIST_CHANGED responses correctly.

It sounds good to pass a flag and don't drop current data calls when it's coming from REQUEST_SETUP_DATA_CALL. Thanks for your comments.

Comment on attachment 674161 [details] [diff] [review] V1 Ok, seems we're agreed then! Clearing review for now.

Addressed previous review comments.

Comment on attachment 677351 [details] [diff] [review] V2 Review of attachment 677351 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +3406,5 @@ > } > this.sendDOMMessage(message); > }, > > + _processDataCallList: function _processDataCallList(datacalls, newDataCallOptions, fromSetupDataCall) { Actually, having a non-null value for `newDataCallOptions` already sort of implies that we're being called from REQUEST_SETUP_DATA_CALL, no? @@ +3428,5 @@ > > if (!updatedDataCall) { > + // If datacalls list is coming from REQUEST_SETUP_DATA_CALL response, > + // we do not change state for any currentDataCalls not in datacalls list. > + if (!fromSetupDataCall) { So I think you can just do if (!newDataCallOptions) { here.

(In reply to Philipp von Weitershausen [:philikon] from comment #12) > Comment on attachment 677351 [details] [diff] [review] > V2 > > Review of attachment 677351 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +3406,5 @@ > > } > > this.sendDOMMessage(message); > > }, > > > > + _processDataCallList: function _processDataCallList(datacalls, newDataCallOptions, fromSetupDataCall) { > > Actually, having a non-null value for `newDataCallOptions` already sort of > implies that we're being called from REQUEST_SETUP_DATA_CALL, no? Yes, this is a really neat. Will send a patch immediately.

Use newDataCallOptions to sort out response from REQUEST_SETUP_DATA_CALL.

Comment on attachment 677632 [details] [diff] [review] V3 Review of attachment 677632 [details] [diff] [review]: ----------------------------------------------------------------- Indeed, very neat. 謝謝!