If the |.readInt32()| number isn't known to RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR, then |failCause| will be undefined [1] that leads to potential infinite requests in [2]. [1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#5458 [2] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#3903

nominate as 2.0M because the duplicate one is 2.0M+.

Comment on attachment 8537785 [details] [diff] [review] 1112550.patch Review of attachment 8537785 [details] [diff] [review]: ----------------------------------------------------------------- Hi Aknow, Please see comment 0 for the issue analysis. Thank you!

Comment on attachment 8537785 [details] [diff] [review] 1112550.patch Review of attachment 8537785 [details] [diff] [review]: ----------------------------------------------------------------- What's the number we got from readInt32()? Why is the number not defined in RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR? In my opinion, all the possible values of failCause are listed in ril.h. We should make sure that they are covered in ril_consts.js. Although the patch provides a robust way to avoid the issue, we still need to figure out the root cause. I expect to add the missing failCause and have this patch. ::: dom/system/gonk/ril_worker.js @@ +5462,5 @@ > + let causeNum = Buf.readInt32(); > + // To make _processCalls work as design, failCause couldn't be "undefined." > + // See Bug 1112550 for details. > + failCause = RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[causeNum] ? > + RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[causeNum] : null; shorter version failCause = RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[causeNum] || null

(In reply to Szu-Yu Chen [:aknow] from comment #5) > Comment on attachment 8537785 [details] [diff] [review] > 1112550.patch > > Review of attachment 8537785 [details] [diff] [review]: > ----------------------------------------------------------------- > > What's the number we got from readInt32()? Why is the number not defined in > RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR? > In my opinion, all the possible values of failCause are listed in ril.h. We > should make sure that they are covered in ril_consts.js. Although the patch > provides a robust way to avoid the issue, we still need to figure out the > root cause. > > I expect to add the missing failCause and have this patch. > > ::: dom/system/gonk/ril_worker.js > @@ +5462,5 @@ > > + let causeNum = Buf.readInt32(); > > + // To make _processCalls work as design, failCause couldn't be "undefined." > > + // See Bug 1112550 for details. > > + failCause = RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[causeNum] ? > > + RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[causeNum] : null; > > shorter version > failCause = RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[causeNum] || null Hi Aknow, The failCauseCode reported is 31, which is a valid number in 24.008 Annex H. The meaning is "This cause is used to report a normal event only when no other cause in the normal class applies." In addition to my previous patch, which I think we still need it to make the _processCall robust, I will add those missed in ril_consts.js according to 24.008 Annex H in revision. What do you think? Thanks!

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6) > (In reply to Szu-Yu Chen [:aknow] from comment #5) > > Comment on attachment 8537785 [details] [diff] [review] > > 1112550.patch > > > > Review of attachment 8537785 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > What's the number we got from readInt32()? Why is the number not defined in > > RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR? > > In my opinion, all the possible values of failCause are listed in ril.h. We > > should make sure that they are covered in ril_consts.js. Although the patch > > provides a robust way to avoid the issue, we still need to figure out the > > root cause. > > > > I expect to add the missing failCause and have this patch. > > > > ::: dom/system/gonk/ril_worker.js > > @@ +5462,5 @@ > > > + let causeNum = Buf.readInt32(); > > > + // To make _processCalls work as design, failCause couldn't be "undefined." > > > + // See Bug 1112550 for details. > > > + failCause = RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[causeNum] ? > > > + RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[causeNum] : null; > > > > shorter version > > failCause = RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[causeNum] || null > > Hi Aknow, > The failCauseCode reported is 31, which is a valid number in 24.008 Annex H. > The meaning is "This cause is used to report a normal event only when no > other cause in the normal class applies." > > In addition to my previous patch, which I think we still need it to make the > _processCall robust, I will add those missed in ril_consts.js according to > 24.008 Annex H in revision. What do you think? Thanks! Yes, that's exactly what I meant in the comment: having your fix + adding missing cause. Thank you.

1) Add missing failCause values 2) Make code robust by checking if |RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[failcause]| is undefined

Comment on attachment 8538415 [details] [diff] [review] 1112550-v2.patch Review of attachment 8538415 [details] [diff] [review]: ----------------------------------------------------------------- Hi Aknow, your comments have been addressed. I add the values defined in 24.008 Annex H and ril.h. The mapping b/w CALL_FAIL_* and GECKO_CALL_ERROR_* isn't 100% 1-by-1. CALL_FAIL_NORMAL and CALL_FAIL_NORMAL_UNSPECIFIED are mapped to |NormalCallClearingError|. CALL_FAIL_DIAL_MODIFIED_TO_* mapped to |ModifiedDialError|. Please let me know if you have different opinions, thank you.

Comment on attachment 8538415 [details] [diff] [review] 1112550-v2.patch some typos...

Hi Aknow, your comments have been addressed. I add the values defined in 24.008 Annex H and ril.h. The mapping b/w CALL_FAIL_* and GECKO_CALL_ERROR_* isn't 100% 1-by-1. CALL_FAIL_NORMAL and CALL_FAIL_NORMAL_UNSPECIFIED are mapped to |NormalCallClearingError|. CALL_FAIL_DIAL_MODIFIED_TO_* mapped to |ModifiedDialError|. Please let me know if you have different opinions, thank you.

Comment on attachment 8538421 [details] [diff] [review] 1112550-v2.patch Review of attachment 8538421 [details] [diff] [review]: ----------------------------------------------------------------- Awesome!! Thank you for the complete list.

Thanks for the review. I'll land it while https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3f92b4579a20 result comes.

Hi Hsin-Yi, Is this patch also good for 2.0M? Thanks for the patch!

Hsin-Yi, Uplift to 2.0m got conflicts in the following files, Could you provide a patch for 2.0m? Thanks! -- dom/system/gonk/ril_consts.js.rej dom/system/gonk/ril_worker.js.rej

(In reply to Kai-Zhen Li [:seinlin] from comment #17) > Hsin-Yi, Uplift to 2.0m got conflicts in the following files, Could you > provide a patch for 2.0m? Thanks! > -- > dom/system/gonk/ril_consts.js.rej > dom/system/gonk/ril_worker.js.rej Sure, let me do that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=962c08a0349d try on 2.0m Will ask for check-in while result comes.

patch for v2.0m

Note: this patch is a follow-up of bug 1088690, which isn't in v2.1. So this is 2.1 unaffected.

sorry, my bad ...

Hi Seinlin, please help merge attachment 8541401 [details] [diff] [review][v2.0m] [follow-up] 1112550-20m-followup.patch, thank you very much!