In bug 720643 we punted on SMS error notifications, so this bug is about that.

(The 'sms-send-failed' notification doesn't exist anymore, renaming bug summary accordingly.)

Error handling in ril_worker now goes `handleRequestError`, which post a message back to RadioInterfaceLayer. When sending out a SMS message, it might involve multiple requests like concatenation, status report requests. The error handling can be complex that can't be a single notification to upper layer. Besides, there might be additional steps to take before notifying RadioInterfaceLayer. For example, some SMS transaction might delay rild acknowledgement until the procedure is fully completed or NAK upon errors. I suggest we should not handle all request errors in one `handleRequestError`, but delegate it to per request handler. For SMS, that will be in REQUEST_SEND_SMS, UNSOLICITED_RESPONSE_NEW_SMS_STATUS_REPORT, even REQUEST_SIM_IO for (U)SIM download PDUs.

Per discussion with Philipp on irc, we should remove `handleRequestError()` and delegate error handling to each handler function. The RadioInterfaceLayer side won't change much, but there won't be any type=error message. Instead, handlers in ril_worker should perform some first-aid upon error, then post error objs with their own types and RadioInterfaceLayer processes them separately. Note that this behavior change affects all ril_worker requests, not only those related to SMS. However, this issue may still focus on SMS error handling.

Price is also attacking the error handling in bug 712944. You may want to discuss with him too so you don't stomp on each other's code :)

yep, that's why I add him to CC list and mail him in private last night ;)

Hi Vicamo and KanRu: I agree with you on comment 2 and 3. It is also the problem that I encounter now Looking forward to discussing you later on.

add TODO for bug 736702

Comment on attachment 611420 [details] [diff] [review] Bug 727319 - Part 1: remove handleRequestError() Review of attachment 611420 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ -476,5 @@ > - request_type); > - } > - RIL.handleRequestError(options); > - return; > - } We shouldn't remove this unconditionally. Parcel handlers will now be invoked when errors occur but none of them (apart from the REQUEST_SEND_SMS one in Part 2) will know how to handle this case. The parcel length guards we put in place in bug 723244 will prevent us from reading garbage data, but I still feel very uneasy about this. I know it's a bit tedious, but I would strongly prefer if we added something like this to all parcel handlers: if (options.rilRequestError) { return; }

Comment on attachment 611690 [details] [diff] [review] Part 2: notify SMS send failed : V2 Review of attachment 611690 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me apart from the nit below and the fact that it's based on bug 736697 and the "envelop" (sic!) stuff which I don't think we need for now. r=me with those addressed. ::: dom/system/gonk/ril_worker.js @@ +1897,5 @@ > RIL[REQUEST_SEND_SMS] = function REQUEST_SEND_SMS(length, options) { > + if (options.rilRequestError) { > + switch (options.rilRequestError) { > + case ERROR_SMS_SEND_FAIL_RETRY: > + if (options.retryCount < 3) { Please const this value! e.g. SMS_RETRY_MAX or something like that. Also, do you have any particular reason for why it's 3? I'm not disagreeing with the number, just curious :)

(In reply to Philipp von Weitershausen [:philikon] from comment #10) > Comment on attachment 611420 [details] [diff] [review] > Bug 727319 - Part 1: remove handleRequestError() > > Review of attachment 611420 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ -476,5 @@ > > - request_type); > > - } > > - RIL.handleRequestError(options); > > - return; > > - } > > We shouldn't remove this unconditionally. Parcel handlers will now be > invoked when errors occur but none of them (apart from the REQUEST_SEND_SMS > one in Part 2) will know how to handle this case. The parcel length guards > we put in place in bug 723244 will prevent us from reading garbage data, but > I still feel very uneasy about this. I don't understand you. This function will not be referenced in any other functions, why keep it? > I know it's a bit tedious, but I would strongly prefer if we added something > like this to all parcel handlers: > > if (options.rilRequestError) { > return; > } To keep expected behaviors, I agree with you. I'll add these lines to every existing handlers.

(In reply to Philipp von Weitershausen [:philikon] from comment #11) > Comment on attachment 611690 [details] [diff] [review] > Part 2: notify SMS send failed : V2 > > Review of attachment 611690 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good to me apart from the nit below and the fact that it's based > on bug 736697 and the "envelop" (sic!) stuff which I don't think we need for > now. r=me with those addressed. > ::: dom/system/gonk/ril_worker.js > @@ +1897,5 @@ > > RIL[REQUEST_SEND_SMS] = function REQUEST_SEND_SMS(length, options) { > > + if (options.rilRequestError) { > > + switch (options.rilRequestError) { > > + case ERROR_SMS_SEND_FAIL_RETRY: > > + if (options.retryCount < 3) { > > Please const this value! e.g. SMS_RETRY_MAX or something like that. > > Also, do you have any particular reason for why it's 3? I'm not disagreeing > with the number, just curious :) That's an implementation specific value. 3GPP 23.040 clause 9.2.3.6 TP-Message-Reference(TP-MR) "The number of times the MS automatically repeats the SMS-SUBMIT shall be in the range 1 to 3 but the precise number is an implementation matter." I'll make these sentences into comments of that constant.

(In reply to Vicamo Yang [:vicamo] from comment #12) > > We shouldn't remove this unconditionally. Parcel handlers will now be > > invoked when errors occur but none of them (apart from the REQUEST_SEND_SMS > > one in Part 2) will know how to handle this case. The parcel length guards > > we put in place in bug 723244 will prevent us from reading garbage data, but > > I still feel very uneasy about this. > > I don't understand you. This function will not be referenced in any other > functions, why keep it? Um, I wasn't saying we shouldn't remove it. I was saying we shouldn't remove it *unconditionally*. The condnition being that we add the discussed check to every parcel handler. > > I know it's a bit tedious, but I would strongly prefer if we added something > > like this to all parcel handlers: > > > > if (options.rilRequestError) { > > return; > > } > > To keep expected behaviors, I agree with you. I'll add these lines to every > existing handlers. Great, thanks!

(In reply to Philipp von Weitershausen [:philikon] from comment #11) > Comment on attachment 611690 [details] [diff] [review] > Part 2: notify SMS send failed : V2 > > Review of attachment 611690 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good to me apart from the nit below and the fact that it's based > on bug 736697 and the "envelop" (sic!) stuff which I don't think we need for > now. r=me with those addressed. I can rebase this patch to m-c tip without bug 736697 at the price of a merge conflict in part 4 of bug 736697. Shall I?

(In reply to Vicamo Yang [:vicamo] from comment #15) > I can rebase this patch to m-c tip without bug 736697 at the price of a > merge conflict in part 4 of bug 736697. Shall I? Yes please. This bug is way more important than bug 736697 IMHO.

(In reply to Philipp von Weitershausen [:philikon] from comment #16) > (In reply to Vicamo Yang [:vicamo] from comment #15) > > I can rebase this patch to m-c tip without bug 736697 at the price of a > > merge conflict in part 4 of bug 736697. Shall I? > > Yes please. This bug is way more important than bug 736697 IMHO. Actually, given my previous misconception about bug 736697, I don't think we have to rebase. Just please update the "envelope" spelling, thanks :)

review comment #10. Note that unsolicited responses do not have `options.rilRequestError`. In fact, `options` passed for unsolicited responses is undefined. Some unsolicited responses invoke their corresponding solicited responses, fixed as well.

1) introduce SMS_RETRY_MAX 2) rebase to latest change of bug 736697 for misspelling of `envelope`. 3) report `sms-send-failed` in REQUEST_GET_SMSC_ADDRESS as well

Comment on attachment 612489 [details] [diff] [review] Part 2: notify SMS send failed : V3 fix wrong description pasted

Comment on attachment 612489 [details] [diff] [review] Part 2: notify SMS send failed : V3 Review of attachment 612489 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1562,5 @@ > delete this._pendingSentSmsMap[message.messageRef]; > > if ((status >>> 5) != 0x00) { > + // It seems unlikely to get a result code for a failure to deliver. > + // Even if, we don't want to do anything with this. These lines come from embedding/android/GeckoSmsManager.java. However, I think we might possiblely need the deliver-failed error in some situation. For example, when you text to a wrong number, the UI may never know that's a wrong number. We does get a PDU_ST_2_INTERWORKING_UNAVALIABLE in this case, and might be useful for SMS app to prompt an error dialog or the like. `deliver-failed` is currently not available in mozSms. Android saves delivery status of every outgoing message. Just a notice for possible future refinement.

Comment on attachment 612486 [details] [diff] [review] Part 1: remove handleRequestError() : V2 Nice! Good point about the unsolicited parcels. Doing something like `if (options && options.rilRequestError)` would also have been an acceptable solution, but this works for now.

(In reply to Vicamo Yang [:vicamo] from comment #21) > > if ((status >>> 5) != 0x00) { > > + // It seems unlikely to get a result code for a failure to deliver. > > + // Even if, we don't want to do anything with this. > > These lines come from embedding/android/GeckoSmsManager.java. However, I > think we might possiblely need the deliver-failed error in some situation. > For example, when you text to a wrong number, the UI may never know that's a > wrong number. We does get a PDU_ST_2_INTERWORKING_UNAVALIABLE in this case, > and might be useful for SMS app to prompt an error dialog or the like. > > `deliver-failed` is currently not available in mozSms. Excellent observation. It seems that we should at least add a delivery failed notification to the SMS API. Can you please file a bug for this? > Android saves delivery status of every outgoing message. As for storing the delivery status of each text message, I would very much like to do it, though Mounir hasn't been a big fan of that idea. But we can discuss it for sure.

Comment on attachment 612489 [details] [diff] [review] Part 2: notify SMS send failed : V3 Review of attachment 612489 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_consts.js @@ +243,5 @@ > > +// 3GPP 23.040 clause 9.2.3.6 TP-Message-Reference(TP-MR): > +// The number of times the MS automatically repeats the SMS-SUBMIT shall be in > +// the range 1 to 3 but the precise number is an implementation matter. > +const SMS_RETRY_MAX = 3; Nice!

(In reply to Philipp von Weitershausen [:philikon] from comment #23) > (In reply to Vicamo Yang [:vicamo] from comment #21) > > > if ((status >>> 5) != 0x00) { > > > + // It seems unlikely to get a result code for a failure to deliver. > > > + // Even if, we don't want to do anything with this. > > > > These lines come from embedding/android/GeckoSmsManager.java. However, I > > think we might possiblely need the deliver-failed error in some situation. > > For example, when you text to a wrong number, the UI may never know that's a > > wrong number. We does get a PDU_ST_2_INTERWORKING_UNAVALIABLE in this case, > > and might be useful for SMS app to prompt an error dialog or the like. > > > > `deliver-failed` is currently not available in mozSms. > > Excellent observation. It seems that we should at least add a delivery > failed notification to the SMS API. Can you please file a bug for this? Seems like Tim already filed one that seems related to this problem: bug 742790. Perhaps we should just morph that bug?

https://hg.mozilla.org/integration/mozilla-inbound/rev/25b1bf1420ab https://hg.mozilla.org/integration/mozilla-inbound/rev/52643779c4d8

(In reply to Philipp von Weitershausen [:philikon] from comment #25) > (In reply to Philipp von Weitershausen [:philikon] from comment #23) > > Excellent observation. It seems that we should at least add a delivery > > failed notification to the SMS API. Can you please file a bug for this? > > Seems like Tim already filed one that seems related to this problem: bug > 742790. Perhaps we should just morph that bug? Great! Looks like there is a real need for this missing function. Let's discuss details there.

https://hg.mozilla.org/mozilla-central/rev/25b1bf1420ab https://hg.mozilla.org/mozilla-central/rev/52643779c4d8