909638 - B2G RIL: callError and dataCallError sent from ril_worker is not processed in radioInterfaceLayer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

We could not simply use whether we have rilMessageToken in message from ril_worker to determine the type (unsolicited / callback-based) of message.

Using dial as example.

http://goo.gl/33r5Ws
dial() in RadioInterfaceLayer.js
Send the message through workerMessenger.send w/o callback

http://goo.gl/pKWKpj
w/o, it is not put into tokenCallbackMap

http://goo.gl/wzy60y
dial() in ril_worker.js
When onerror triggered, we send back "callError" with options. options contains all the property when we send the message from RadioInterfaceLayer to ril_worker, including rilMessageToken.

http://goo.gl/huk0Sq
When RadioInterfaceLayer receives this "callError", it seems that
(1) It contains token, not an unsolicited message. Thus it will not trigger the unsolicited message handler.
(2) It has no callback for this message. 

Then, we miss the handling for this "callError" message.
Problem may occur elsewhere, if we
(1) send the message from RadioInterfaceLayer w/o callback and
(2) direct use the message data (options) and send it back from ril_worker
  * it contains rilMessageToken and cause problem

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Another problem
"setupDataCall" sent from RadioInterfaceLayer might get the response "dataCallError" which will not be process in current design.

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Let me describe some details.

After we introduce callback-based worker messaging. We would like
(1) RadioInterface -> worker --(solicited response)--> RadioInterface
    the response is directed to the callback and handled
(2) worker --(unsolicited response)--> RadioInterface
    the response is handled in another unsolicited handler

So, our purpose is to have a mechanism to identify whether the response from worker is solicited or unsolicited. Then, we could address case (1) and (2) correctly.

In current design, we use rilMessageToken for the purpose.
(1) if the response message contains token => solicited
(2) no token => unsolicited

Then, the problem is caused by message object reuse.
RadioInterface --(dial)--> worker --(callError)--> RadioInterface

Worker reuses the message and change the messageType to callError. So the response (contains token) is treated as a solicited response and it's wrong.

Proposal 0:

It's easy to remove the token field when we send the callError or dataCallError. Then everything works. However, we could not guarantee there is no missing. Moreover, we should have a systematic way to avoid writing the wrong code in the future.

Proposal 1:

We identify solicited/unsolicited in ril_worker when sending the response. Ensure the following properties.
(1) solicited (message contains token, but no messageType)
    messageType is not needed because we already maintain a mapping from token to callback in RadioInterface.
(2) unsolicited (message contains messageType, but no token)
    messageType is used to distinguish the message and dispatch to the proper handler.
(3) token and messageType are mutually exclusive. could not have both of them in the same time.

We could separate sendChromeMessage() in worker into two specific functions, sendSolicitedResponse(), sendUnsolicitedResponse(). In these two functions, we could do the proper operations to maintain the above 3 properties.

Effort/Drawback: Should modify each sendChromeMessage carefully. In some case, it might not be easier identified w/o tracing the code flow.

Proposal 2:

Solicited/unsolicited is determined by messageType. In RadioInterface, we could maintain a list of unsolicited messageType. When receiving an response from worker:
(1) check token and callback. If callback existed, run it.
(2) If this is a known unsolicited messageType, direct it to unsolicited handler.
(3) Otherwise, do nothing

Effort: Maintain a list, should be the same as all switch/case values in unsolicited handler [1].
Drawback: Need extra runtime effort for the identification.

[1] http://goo.gl/viJRA4

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Hsinyi, Yoshi, what is your opinion for this one?

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Can you ask Vicamo?
He's just a few meters from you.

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Already discuss it with vicamo before I wrote on bugzilla.
We just want to know your opinions for a better final solution

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Things we want to do here is to explicitly forbid abuse of solicited/unsolicited responses because we, as human, just missed the callError case in bug 899885.  From this, proposal-0 just won't help.  The difference between proposal-1 and proposal-2 for me is proposal-1 defers jobs that should be done now (determine which messages are solicited/unsolicited) to runtime.  Proposal-1 doesn't really help clarifying whether a message is going to be solicited or unsolicited in ril_worker because we often reuse |options|.

Comment 7

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> Things we want to do here is to explicitly forbid abuse of
> solicited/unsolicited responses because we, as human, just missed the
> callError case in bug 899885.  From this, proposal-0 just won't help.  The

Agree with Vicamo about proposal-0.

> difference between proposal-1 and proposal-2 for me is proposal-1 defers
                                                        ^^^^^^^^^^^^ typo here? you meant proposal-2 ?

> jobs that should be done now (determine which messages are
> solicited/unsolicited) to runtime.  Proposal-1 doesn't really help
> clarifying whether a message is going to be solicited or unsolicited in
> ril_worker because we often reuse |options|.

I haven't got time to think about the possibility of other proposals. But among these 3, proposal-2 looks better to me from the view of debugging and code maintaining.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Szu-Yu Chen [:aknow] from comment #5)
> Already discuss it with vicamo before I wrote on bugzilla.
> We just want to know your opinions for a better final solution

If you want others to discuss with you, you need to be respectful here.
You cannot ask for others review/opinion,
then one day you say, hey we should do other things in this bug, 
and throw away previous comments/suggestions/reviews,
and tell me , "Your idea is moved to other places. Now please review my new patch."

You need to know how to communicate with people on the bugzilla/ML first.

We do have other more important jobs to do, so do appreciate the time/effort that others spend on your bug.

Comment 9

[Ken Chang[:kenkai]]

We should have a offline discussion for the review procedure after discussing Yoshi. Let's keep discussing the solution for this bug.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> > difference between proposal-1 and proposal-2 for me is proposal-1 defers
>                                                         ^^^^^^^^^^^^ typo
> here? you meant proposal-2 ?

Thank you, that's a typo :)

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #8)

Aknow didn't mean that I think.  We had a short discuss about the options, which one fits the final look of ril_worker most, etc.  But we all contribute to ril_worker a lot, so your opinions are equally important.  Maybe you feel like other ways, and that's why we're asking for more thoughts.

Like Ken said, face to face talk by our native language is always much easier. :)

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

Suggest to apply proposal 0 first to fix the issue. Otherwise it break current 'callError', 'dataError' handling.
Then, we could think about the long term solution.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Szu-Yu Chen [:aknow] from comment #12)
> Suggest to apply proposal 0 first to fix the issue. Otherwise it break
> current 'callError', 'dataError' handling.
> Then, we could think about the long term solution.

Go ahead!

Comment 14

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Add test case for dialing in radio off

Add a test case.
It will fail in current code base but pass with the patch in this bug.

Comment 15

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Remove token in callError/dataCallError message

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 801503 [details] [diff] [review]
Part 1: Add test case for dialing in radio off

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

::: dom/telephony/test/marionette/test_outgoing_radio_off.js
@@ +66,5 @@
> +      // Wait until card state changes to "not-ready" after turning off radio.
> +      if ((enabled && icc.cardState == "ready") ||
> +          (!enabled && icc.cardState != "ready")) {
> +        icc.removeEventListener("cardstatechange", handler);
> +      callback();

nit: indentation.

@@ +106,5 @@
> +  };
> +}
> +
> +function cleanUp() {
> +  is(pendingEmulatorCmdCount, 0, "has pending emulator command");

Should explicitly wait until |pendingEmulatorCmdCount| turns 0, not just check it here.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 801504 [details] [diff] [review]
Part 2: Remove token in callError/dataCallError message

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

::: dom/system/gonk/ril_worker.js
@@ -1360,5 @@
> -    let onerror = (function onerror(errorMsg) {
> -      options.callIndex = -1;
> -      options.rilMessageType = "callError";
> -      options.errorMsg = errorMsg;
> -      this.sendChromeMessage(options);

So you can simply have:

  this.sendChromeMessage({ rilMessageType: "callError",
                           callIndex: -1,
                           errorMsg: errorMsg });

This way, we'll be more certain to the fact "the attributes we need for 'callError' messages are 'callIndex' and 'errorMsg'", and we don't bloat the response with extra attributes and cause similar bugs like this.

@@ +3659,3 @@
>    _sendDataCallError: function _sendDataCallError(message, errorCode) {
> +    // Should not include token for unsolicited response.
> +    message.rilMessageToken = null;

|delete message.rilMessageToken|

@@ -4988,5 @@
>        break;
>      default:
> -      options.rilMessageType = "callError";
> -      options.errorMsg = RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[failCause];
> -      this.sendChromeMessage(options);

ditto.

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1: Add test case for dialing in radio off. r=vicamo

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#2: Remove token in callError/dataCallError message

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 802033 [details] [diff] [review]
Part 2#2: Remove token in callError/dataCallError message

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

Thank you :)

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1#2: Add test case for dialing in radio off. r=vicamo

rebase

Comment 22

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 2: Remove token in callError/dataCallError message. r=vicamo

Comment 23

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1#3: Add test case for dialing in radio off. r=vicamo

correct an indent error

Comment 24

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=8a3bc549dd0e

Comment 25

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

https://hg.mozilla.org/integration/b2g-inbound/rev/dbf7b8418bd2
https://hg.mozilla.org/integration/b2g-inbound/rev/144f672e64d1

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dbf7b8418bd2
https://hg.mozilla.org/mozilla-central/rev/144f672e64d1