Last Comment Bug 727319 - B2G SMS: Notify SMS send failures
: B2G SMS: Notify SMS send failures
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
:
Mentors:
Depends on: 720632 736697
Blocks: b2g-sms 743179
  Show dependency treegraph
 
Reported: 2012-02-14 17:41 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-04-06 11:36 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug 727319 - Part 1: remove handleRequestError() (2.83 KB, patch)
2012-04-02 05:11 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review-
Details | Diff | Splinter Review
Bug 727319 - Part 2: notify SMS send failed (6.10 KB, patch)
2012-04-02 05:12 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 2: notify SMS send failed : V2 (6.20 KB, patch)
2012-04-02 19:10 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 1: remove handleRequestError() : V2 (16.21 KB, patch)
2012-04-05 03:16 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review
Part 2: notify SMS send failed : V3 (7.05 KB, patch)
2012-04-05 03:21 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2012-02-14 17:41:59 PST
In bug 720643 we punted on SMS error notifications, so this bug is about that.
Comment 1 Philipp von Weitershausen [:philikon] 2012-02-14 17:43:08 PST
(The 'sms-send-failed' notification doesn't exist anymore, renaming bug summary accordingly.)
Comment 2 Vicamo Yang [:vicamo][:vyang] 2012-03-29 07:07:41 PDT
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.
Comment 3 Vicamo Yang [:vicamo][:vyang] 2012-03-30 10:15:55 PDT
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.
Comment 4 Kan-Ru Chen [:kanru] (UTC+8) 2012-03-30 16:17:38 PDT
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 :)
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-03-30 16:42:19 PDT
yep, that's why I add him to CC list and mail him in private last night ;)
Comment 6 Price Tseng 2012-03-30 21:46:47 PDT
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.
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-04-02 05:11:43 PDT
Created attachment 611420 [details] [diff] [review]
Bug 727319 - Part 1: remove handleRequestError()
Comment 8 Vicamo Yang [:vicamo][:vyang] 2012-04-02 05:12:12 PDT
Created attachment 611421 [details] [diff] [review]
Bug 727319 - Part 2: notify SMS send failed
Comment 9 Vicamo Yang [:vicamo][:vyang] 2012-04-02 19:10:28 PDT
Created attachment 611690 [details] [diff] [review]
Part 2: notify SMS send failed : V2

add TODO for bug 736702
Comment 10 Philipp von Weitershausen [:philikon] 2012-04-04 21:38:59 PDT
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 11 Philipp von Weitershausen [:philikon] 2012-04-04 21:44:54 PDT
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 :)
Comment 12 Vicamo Yang [:vicamo][:vyang] 2012-04-04 22:45:26 PDT
(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.
Comment 13 Vicamo Yang [:vicamo][:vyang] 2012-04-04 22:51:16 PDT
(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.
Comment 14 Philipp von Weitershausen [:philikon] 2012-04-04 22:55:35 PDT
(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!
Comment 15 Vicamo Yang [:vicamo][:vyang] 2012-04-04 22:58:02 PDT
(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?
Comment 16 Philipp von Weitershausen [:philikon] 2012-04-05 00:16:10 PDT
(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.
Comment 17 Philipp von Weitershausen [:philikon] 2012-04-05 00:35:41 PDT
(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 :)
Comment 18 Vicamo Yang [:vicamo][:vyang] 2012-04-05 03:16:35 PDT
Created attachment 612486 [details] [diff] [review]
Part 1: remove handleRequestError() : V2

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.
Comment 19 Vicamo Yang [:vicamo][:vyang] 2012-04-05 03:21:36 PDT
Created attachment 612489 [details] [diff] [review]
Part 2: notify SMS send failed : V3

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 20 Vicamo Yang [:vicamo][:vyang] 2012-04-05 03:23:05 PDT
Comment on attachment 612489 [details] [diff] [review]
Part 2: notify SMS send failed : V3

fix wrong description pasted
Comment 21 Vicamo Yang [:vicamo][:vyang] 2012-04-05 03:43:05 PDT
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 22 Philipp von Weitershausen [:philikon] 2012-04-05 09:50:18 PDT
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.
Comment 23 Philipp von Weitershausen [:philikon] 2012-04-05 09:55:01 PDT
(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 24 Philipp von Weitershausen [:philikon] 2012-04-05 09:58:22 PDT
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!
Comment 25 Philipp von Weitershausen [:philikon] 2012-04-05 10:36:15 PDT
(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?
Comment 27 Vicamo Yang [:vicamo][:vyang] 2012-04-05 19:15:10 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.