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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #0)
> For example, in bug 1058282, 
Sorry, it shall be bug 1057973 instead.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Thanks Bevis, I'll take a look at this!

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 4

[Jessica Jong [:jessica] (inactive)]

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.

Comment 5

[Jessica Jong [:jessica] (inactive)]

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.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(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 7

[Bevis Tseng[:bevis][:btseng](Exited)]

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?

Comment 8

[Jessica Jong [:jessica] (inactive)]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) 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.

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

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.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: patch (final).

Comment 12

[Jessica Jong [:jessica] (inactive)]

try green!

https://tbpl.mozilla.org/?tree=Try&rev=952b74506cfe

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6345ff78e8d8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6345ff78e8d8