Closed
Bug 909638
Opened 11 years ago
Closed 11 years ago
B2G RIL: callError and dataCallError sent from ril_worker is not processed in radioInterfaceLayer
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: aknow, Assigned: aknow)
References
Details
Attachments
(2 files, 5 obsolete files)
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
Assignee | ||
Updated•11 years ago
|
Summary: B2G RIL: RadioInterfaceLayer.js, problem caused by using rilMessageToken to determine unsolicited or callback -based message → B2G RIL: RadioInterfaceLayer.js, problem caused by using rilMessageToken to determine unsolicited or callback-based message
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → szchen
Assignee | ||
Comment 1•11 years ago
|
||
Another problem "setupDataCall" sent from RadioInterfaceLayer might get the response "dataCallError" which will not be process in current design.
Assignee | ||
Comment 2•11 years ago
|
||
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
Flags: needinfo?(htsai)
Flags: needinfo?(allstars.chh)
Can you ask Vicamo? He's just a few meters from you.
Flags: needinfo?(allstars.chh)
Assignee | ||
Comment 5•11 years ago
|
||
Already discuss it with vicamo before I wrote on bugzilla. We just want to know your opinions for a better final solution
Comment 6•11 years ago
|
||
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•11 years ago
|
||
(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.
Flags: needinfo?(htsai)
(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•11 years ago
|
||
We should have a offline discussion for the review procedure after discussing Yoshi. Let's keep discussing the solution for this bug.
Comment 10•11 years ago
|
||
(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•11 years ago
|
||
(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. :)
Assignee | ||
Comment 12•11 years ago
|
||
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•11 years ago
|
||
(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!
Assignee | ||
Updated•11 years ago
|
Summary: B2G RIL: RadioInterfaceLayer.js, problem caused by using rilMessageToken to determine unsolicited or callback-based message → B2G RIL: callError and dataCallError sent from ril_worker is not processed in radioInterfaceLayer
Assignee | ||
Comment 14•11 years ago
|
||
Add a test case. It will fail in current code base but pass with the patch in this bug.
Attachment #801503 -
Flags: review?(vyang)
Comment 16•11 years ago
|
||
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.
Attachment #801503 -
Flags: review?(vyang) → review+
Comment 17•11 years ago
|
||
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.
Attachment #801504 -
Flags: review?(vyang)
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #801504 -
Attachment is obsolete: true
Attachment #802033 -
Flags: review?(vyang)
Comment 20•11 years ago
|
||
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 :)
Attachment #802033 -
Flags: review?(vyang) → review+
Updated•11 years ago
|
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Version: Trunk → unspecified
Assignee | ||
Comment 23•11 years ago
|
||
correct an indent error
Attachment #802777 -
Attachment is obsolete: true
Assignee | ||
Comment 24•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8a3bc549dd0e
Keywords: checkin-needed
Comment 25•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/dbf7b8418bd2 https://hg.mozilla.org/integration/b2g-inbound/rev/144f672e64d1
Keywords: checkin-needed
Comment 26•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dbf7b8418bd2 https://hg.mozilla.org/mozilla-central/rev/144f672e64d1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•