Closed Bug 794767 Opened 7 years ago Closed 7 years ago

B2G RIL: Handle data call error without APN name

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: swu, Assigned: swu)

Details

Attachments

(2 files, 1 obsolete file)

In some cases we may receive _processDataCallList() without 'newDataCallOptions', this will cause problem for data call error handling.  Without APN name, data call error handling cannot point to right APN connection.

This can be reproduced in latest gecko, by enabling data call, then go to airplane mode. 

To fix it, we can get APN name from currentDataCalls{} by comparing cid.
This bug also stops to resume data call from airplane mode off, must be blocking-basecamp.
blocking-basecamp: --- → ?
We have to resume data after airplane mode is shut off so agreed, it's a blocker.
blocking-basecamp: ? → +
Shian-Yow, can you take this?
Assignee: nobody → swu
Hi Shian-Yow,
When I tried to reproduce this issue, I found something from log.
When the data call was built and then enabled Airplane mode, I could see the below parcel in worker.

10-01 13:29:14.356 I/Gecko   (  106): RIL Worker: Handling parcel as REQUEST_DEACTIVATE_DATA_CALL

But when I disabled Airplane mode, I couldn't see REQUEST_SETUP_DATA_CALL was shown in the log. The reason why I expected to see this parcel is that it was a pair when I tried to enable/disable Data Connection. Furthermore, When I saw the the definition RIL_REQUEST_SETUP_DATA_CALL in ril.h in /hardware/ril/include/telephony/, it was also written "See also: RIL_REQUEST_DEACTIVATE_DATA_CALL".

Please correct me if I have anything wrong. I will try more on this issue.
Attached patch WIP V1 (obsolete) — Splinter Review
Add null checking on 'newDataCallOptions'.
(In reply to vliu from comment #4)
> Hi Shian-Yow,
> When I tried to reproduce this issue, I found something from log.
> When the data call was built and then enabled Airplane mode, I could see the
> below parcel in worker.
> 
> 10-01 13:29:14.356 I/Gecko   (  106): RIL Worker: Handling parcel as
> REQUEST_DEACTIVATE_DATA_CALL
> 
> But when I disabled Airplane mode, I couldn't see REQUEST_SETUP_DATA_CALL
> was shown in the log. The reason why I expected to see this parcel is that
> it was a pair when I tried to enable/disable Data Connection. Furthermore,
> When I saw the the definition RIL_REQUEST_SETUP_DATA_CALL in ril.h in
> /hardware/ril/include/telephony/, it was also written "See also:
> RIL_REQUEST_DEACTIVATE_DATA_CALL".
> 
> Please correct me if I have anything wrong. I will try more on this issue.

When enabled airplane mode, we first got UNSOLICITED_DATA_CALL_LIST_CHANGE followed by calling _processDataCallList().  The previous connected data call in the data call list will be status:65535, therefore it will be handled this._sendDataCallError().  However due to 'newDataCallOptions' is null, it will cause JavaScript error and .  We should fix this issue first.

The second problem is, when REQUEST_DEACTIVATE_DATA_CALL been handled, it never get response when the phone went into airplance mode.  So the data call will be staying in DISCONNECTING state, which blocked further RIL_REQUEST_SETUP_DATA_CALL after airplane mode disabled.

Is it same as your testing?
(In reply to Shian-Yow Wu from comment #6)
> (In reply to vliu from comment #4)
> > Hi Shian-Yow,
> > When I tried to reproduce this issue, I found something from log.
> > When the data call was built and then enabled Airplane mode, I could see the
> > below parcel in worker.
> > 
> > 10-01 13:29:14.356 I/Gecko   (  106): RIL Worker: Handling parcel as
> > REQUEST_DEACTIVATE_DATA_CALL
> > 
> > But when I disabled Airplane mode, I couldn't see REQUEST_SETUP_DATA_CALL
> > was shown in the log. The reason why I expected to see this parcel is that
> > it was a pair when I tried to enable/disable Data Connection. Furthermore,
> > When I saw the the definition RIL_REQUEST_SETUP_DATA_CALL in ril.h in
> > /hardware/ril/include/telephony/, it was also written "See also:
> > RIL_REQUEST_DEACTIVATE_DATA_CALL".
> > 
> > Please correct me if I have anything wrong. I will try more on this issue.
> 
> When enabled airplane mode, we first got UNSOLICITED_DATA_CALL_LIST_CHANGE
> followed by calling _processDataCallList().  The previous connected data
> call in the data call list will be status:65535, therefore it will be
> handled this._sendDataCallError().  However due to 'newDataCallOptions' is
> null, it will cause JavaScript error and .  We should fix this issue first.
> 
> The second problem is, when REQUEST_DEACTIVATE_DATA_CALL been handled, it
> never get response when the phone went into airplYance mode.  So the data
> call will be staying in DISCONNECTING state, which blocked further
> RIL_REQUEST_SETUP_DATA_CALL after airplane mode disabled.
> 
> Is it same as your testing?

Yes, I saw the second case. What I saw is the this.dataNetworkInterface.state kept in GECKO_NETWORK_STATE_DISCONNECTING state and then it couldn't issue RIL_REQUEST_SETUP_DATA_CALL again after airplane mode disabled. I thought it is the reason why the data call couldn't back to alive. I think the problem is this.dataNetworkInterface.state couldn't change to GECKO_NETWORK_STATE_DISCONNECTED when Airplane mode is enabled. Is it another issue we should file?
After problem 1 ('newDataCallOptions' is null) addressed, data call can be resumed after airplane mode off.  

But there's another issue we should fix.

When airplane mode disabled, device will:
1. Changing radio power to off
2. Receive data call list update with status:65535 for current data call, then it changed the state of the current data call from GECKO_NETWORK_STATE_CONNECTED to GECKO_NETWORK_STATE_UNKNOWN.
4. Gaia system app apps/system/js/airplane_mode.js turns off "ril.data.enabled", which triggers an deactivate data call in Gecko side.
5. Due to data call debouncing function (bug 785982), device will try to setup date call again and got error (because radio power is turning off).
6. Receive radio power state changed to off.

IMHO, during radio changing, we should not allow any data call setup/deactivate, because it either got error or no response.

Vincent, could you file another issue?
Hi Shian-Yow,

Sure!! I will file another issue for Comment 8.
Attached patch V1Splinter Review
We can get APN name from currentDataCalls{} by comparing cid, but if the error case is not for setup data call, it's better to simply leave newDataCall.apn empty.  Otherwise it will trigger unwanted data call setup by reset() in RILNetworkInterface.

Only add null checking on 'newDataCallOptions' should be better.
Attachment #666453 - Attachment is obsolete: true
Attachment #666875 - Flags: review?(vyang)
Comment on attachment 666875 [details] [diff] [review]
V1

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

Since there is similar expression just below the code chunk modified here, I assume the check was just missed before. But I really want to have a complete study about how Android handles all these data call events. It seems we keep having small troubles like APN name is null, newDataCallOptions is null, blahblah. Can we have a full review on this?
Attachment #666875 - Flags: review?(vyang) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> Since there is similar expression just below the code chunk modified here, I
> assume the check was just missed before. But I really want to have a
> complete study about how Android handles all these data call events. It
> seems we keep having small troubles like APN name is null,
> newDataCallOptions is null, blahblah. Can we have a full review on this?

Surely we need to have a full review.  I've created a follow up bug 796868 to review null checking for 3G data call.
https://hg.mozilla.org/mozilla-central/rev/7897fb9e2563
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.