Closed Bug 1059110 Opened 5 years ago Closed 5 years ago

[B2G][RIL] Ensure the consequence of setupDataCallByType() will be returned asynchronously

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S5 (26sep)

People

(Reporter: bevis, Assigned: jessica)

Details

Attachments

(1 file, 2 obsolete files)

The positive result of setupDataCallByType() comes in async-way in most of the scenario.
However, when shared APN is applied and the connection has been setup for one type, then 
the result of setupDataCallByType() for another type comes in sync-way.

This introduces inconsistent behaviors to the caller when using this API.
For example, in bug 1058282, 
MmsService will clear the cached networkInterface  and initiate connectTimer after 
setupDataCallByType("mms"). However, in Shared APN case, all this actions are done 
unexpectedly after all the MMS transactions are done. [1]

[1] http://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsService.js#431
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #0)
> For example, in bug 1058282, 
Sorry, it shall be bug 1057973 instead.
Thanks Bevis, I'll take a look at this!
Assignee: nobody → jjong
Attached patch patch, v1. (obsolete) — Splinter Review
Comment on attachment 8482573 [details] [diff] [review]
patch, v1.

I've verified by reverting the patches in bug 1057973. May I have your feedback/review? Thanks.
Attachment #8482573 - Flags: review?(htsai)
Attachment #8482573 - Flags: feedback?(btseng)
I wonder if we should do the same in |disconnect()|, however, |networkInterface.notifyRILNetworkInterface()| is called synchronously both for shared and non-shared apns, so they behave the same way.
(In reply to Jessica Jong [:jjong] [:jessica] from comment #5)
> I wonder if we should do the same in |disconnect()|, however,
> |networkInterface.notifyRILNetworkInterface()| is called synchronously both
> for shared and non-shared apns, so they behave the same way.

See http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#4943
Comment on attachment 8482573 [details] [diff] [review]
patch, v1.

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

Shall we add some protection for the following scenario?
1. There is already a disconnected event notified from ril_worker and queue in the event queue of the main thread.
2. Caller(like MmsService) ask to setupDataCallByType().

This will cause the notifyRILNetworkInterface() be invoked multiple times.
Some additional state-checking seems needed to me.
How do you think?
Attachment #8482573 - Flags: feedback?(btseng)
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #7)
> Comment on attachment 8482573 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8482573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shall we add some protection for the following scenario?
> 1. There is already a disconnected event notified from ril_worker and queue
> in the event queue of the main thread.
> 2. Caller(like MmsService) ask to setupDataCallByType().
> 
> This will cause the notifyRILNetworkInterface() be invoked multiple times.
> Some additional state-checking seems needed to me.
> How do you think?

As discussed, notifyRILNetworkInterface() be invoked multiple times will not cause wrong state to be propagated since RILNetworkInterface is stateless. However, it may cause redundant notifications to be sent. I'll see what can be done. Thanks, Bevis.
Attached patch patch, v2. (obsolete) — Splinter Review
Forwarding review to Edgar since Hsinyi has a lot on her plate.

Changes since v1:
- Do not call notifyRILNetworkInterface() if state changed while the event was being dispatched.
Attachment #8482573 - Attachment is obsolete: true
Attachment #8482573 - Flags: review?(htsai)
Attachment #8484072 - Flags: review?(echen)
Attachment #8484072 - Flags: feedback?(btseng)
Comment on attachment 8484072 [details] [diff] [review]
patch, v2.

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +4202,5 @@
> +        // the state probably was notified already or need not to be notified.
> +        if (networkInterface.state == state) {
> +          networkInterface.notifyRILNetworkInterface();
> +        }
> +      }.bind(this, RIL.GECKO_NETWORK_STATE_CONNECTED), Ci.nsIEventTarget.DISPATCH_NORMAL);

nit: 'this' reference seems not used.
      .bind(null, RIL.xxx)

@@ +4316,5 @@
> +        // the state probably was notified already or need not to be notified.
> +        if (networkInterface.state == state) {
> +          networkInterface.notifyRILNetworkInterface();
> +        }
> +      }.bind(this, RIL.GECKO_NETWORK_STATE_DISCONNECTED), Ci.nsIEventTarget.DISPATCH_NORMAL);

ditto.
Attachment #8484072 - Flags: feedback?(btseng) → feedback+
Attachment #8484072 - Flags: review?(echen) → review+
Attached patch patch (final).Splinter Review
Attachment #8484072 - Attachment is obsolete: true
Attachment #8495881 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6345ff78e8d8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
You need to log in before you can comment on or make changes to this bug.