1175430 - [SMS] Expose More Network-Specific Error Cause for Various Error Handling in App Layer.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Do not allow anymore retries after three attempts and mark the message as failed with this displaying Unable to send message and there be an Ok to have the user x000D
acknowledge. User can go back into the Sent folder and retry sending the message again and the same will apply for 3 retries.

Comment 1

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1175428#c3
Same reason as the above case, very likely we'll need partner to customize from GAIA.

Comment 2

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Youngjun -

Seems like this is more a Gaia side efforts. Could you help to check this one first? Feel free to ni Bevis and I if you want to discuss further about the possible way to implement this feature in Gaia side

Thanks

Comment 3

[Youngjun Kim]

Same with Bug-1175428. I will let you know our opinion.

Comment 4

[Youngjun Kim]

Dear Rian Kim, 

Please add your comment about sending the class 2 error to Gaia. 

Thank you.

Comment 5

[Rian Kim]

To. Mozilla team
Hello~ This is Rian Kim, SMS Telephony enginner

Device receives a SMS ACK from network after sending SMS to network.
If SMS sending is failed, device receives a SMS ACK that contains two kinds of error information.
The two kind of error information is Error Class and Cause Code.
Error class indicates whether an SMS error has occurred and if so, whether the condition is considered temporary or permalnent.
Error class has 4 kinds of values like belows:
===================================
No error: 0
Temporary error: 2 -> Class 2 error
Permanent error: 3 -> Class 3 error
Reserved: 1
===================================
Class 2 error means the "Temporary error" which value is 2 on above
Class 3 error means the "Permanent error" which value is 3 on above

Verizon requirement is like belows:
[1] Only allow three manual retries when error class is 2,
    user must be given the option of "Message failed would you like to retry?" with a soft key option of "yes or no".
    and do not allow anymore retries after three attempts and mark the message as failed with this displaying "Unable to send message" 
    and there be an "Ok" to have the user acknowledge.
[2] Do not allow retries when there is an error class is 3.
    And the device must also prompt user with Delivery Failed: (Reason given below) With soft key option of ok at the bottom of screen.
    Reason is that "Network Problems", "General Problems", "Service Not Available", "Invalid destination", "Message Too Long for Network", "Service Not Supported".

In current firefox OS source for verizon(Red-tie), SMS telephony does not send a error code to Gecko layer in case that Error class is 2.
Just in case that Error class is 3, SMS telephony sends error code to Gecko layer.
SMS Telephony team is implementing to send an error code to Gecko layer in case that Error class is 2.
Although SMS telephony sends an error code to Gecko layer, Mozilla team have to implement to add all SMS related error including class 2 error, class 3 error.
SMS Telephony team has modified the Gekco source to implement this.
Please refer to attached before/after files, modified file name and file path like belows:
b2g\gecko\dom\mobilemessage\MobileMessageCallback.cpp
b2g\gecko\dom\mobilemessage\interfaces\nsIMobileMessageCallback.idl

In case of class 2 error, SMS telephony sends "SMS_SEND_FAIL_RETRY_ERROR(12)" to Gecko layer.
(const unsigned short SMS_SEND_FAIL_RETRY_ERROR = 18;
 ->This constant value is defined in nsIMobileMessageCallback.idl
Then Gecko sends the error string of "SmsSendFailRetry" to Gaia app(sms app).
If Gaia app receives the error string of "SmsSendFailRetry" from Gecko, display an option screen of "Message failed would you like to retry?" with a soft key option of "yes or no".

In case of class 3 error, SMS telephony sends a cause code to Gecko layer.(cause code is like belows according to verizon requirement)
==================================================================
const unsigned short NETWORK_PROBLEMS_ERROR = 12;
const unsigned short GENERAL_PROBLEMS_ERROR = 13;
const unsigned short SERVICE_NOT_AVAILABLE_ERROR = 14;
const unsigned short INVALID_DESTINATION_ERROR = 15;
const unsigned short MESSAGE_TOO_LONG_FOR_NETWORK_ERROR = 16;
const unsigned short SERVICE_NOT_SUPPORTED_ERROR = 17;

-> This constant values is defined in nsIMobileMessageCallback.idl
==================================================================
Then Gecko sends the error cause code string to Gaia app(sms app).
If Gaia app receives the cause code string(cause code error stiring like belows) from Gecko layer,
prompt to user with "Delivery Faied: Cause code error string" with soft key option of "OK" at the bottom of screen.
==============================
"Network Problems"
"General Problems"
"Service Not Available"
"Invalid Destination"
"Message Too Long For Network"
"Service Not Supported"
==============================

SMS Telephony team will attach the modified sources to Bugzilla and request Mozilla team to review and apply the modified Gecko sources
<Modified source>
b2g\gecko\dom\mobilemessage\MobileMessageCallback.cpp
b2g\gecko\dom\mobilemessage\interfaces\nsIMobileMessageCallback.idl

Best Regards
Rian.

Comment 6

[Rian Kim]

Attachment: Add cause codes for class 3 error and class2

Comment 7

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Rian,

Mozilla team surely can review your patch.
I just put your on the "Assigned To" field. 
Is there anything else you need from us?

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Rian,

To merge the patch from Bugzilla to gecko, and have your contribution recorded we need a formal
mercurial patch for the review process.
After the formal patch attached, you can request a review? to me to start the review process. :)
Would you please follow the chapters in the link below to request a review?
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code
 1. Getting the code from the Mercurial repository
    Note: Since this bug is to be landed in 2.2r branch, please clone the 2.2r repository 
          instead of m-c.
 2. Creating a patch
 3. Try Servers/Getting commit Access to the source code.
    This is nice to have to request Access level 1 to request your patch to be tested on the try server before reviewing and landing. The patch can only be landed if try server is green.

In addition, here are my comments for the patch in comment 6:
1. Please check the link below as an example to see what files to be modified if a new error cause is added.
   https://hg.mozilla.org/mozilla-central/rev/c3202ada540e
   They are
   - dom/mobilemessage/interfaces/nsIMobileMessageCallback.idl
   - dom/mobilemessage/src/MobileMessageCallback.cpp
   - embedding/android/GeckoSmsManager.java
   - mobile/android/base/GeckoSmsManager.java
2. For consistency of the naming, please have all the literal strings in MobileMessageCallback.cpp 
   defined without space included.
3. Since all the error causes are explained in comment 5, we don't have to add these comments in the
   implementation which could be found by blaming the change in the repository.
4. For INVALID_DESTINATION_ERROR, it's redundant to the INVALID_ADDRESS_ERROR we have defined, so
   we could remove this one.

You can speed up the process with the comments above addressed.

Thanks for your contribution to make thing better. :)

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

NI for comment 8.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Move the discussion of bug 1175428 comment 9 to here:
(In reply to Rian Kim from comment #9)
> The error class 2 means one specific error of "ERROR_SMS_SEND_FAIL_RETRY"
> reported by modem.
> Currently Gecko only support auto-retry for the "ERROR_SMS_SEND_FAIL_RETRY".
> To meet a verizon requirement of "Only allow three manual retries when error
> class is 2".
> Mozilla team need to consider Gecko side implement as well as Gaia side
> 
> I would like to propose some additional changes in gecko side, so that Gaia
> can use it to implement this functionality.
> 
> Proposed changes required in Gecko:
> 1. Error codes (Class 2 error in this case) should be stored along with the
> message information.
> 2. Retry (Manual retry) count should be stored along with the message
> information. 
For #1, #2, the schema change to the MobileMessageDB is required and is not recommended in current stage because this bug is to be fixed on 2.2r firstly. It will cause a migration problem from 2.2r to m-c in the future.

> 3. Interface availability (new/update existing) to Gaia for setting retry
> count.
Not a generic user scenario from Web API perspective.
This will be suggested to be done by oem vendor for the customization behaviour.
> 
> Based on the error codes and retry count Gaia should take a decision of
> providing resend option or not.
> If the manual retry is more than 3. The message will be marked as permanent
> error (can't be resend).
Without a change in DB schema, we'd like to suggest gaia to add some extra info in runtime for the request of sending SMS and its related retrial to support this feature instead.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> 4. For INVALID_DESTINATION_ERROR, it's redundant to the
> INVALID_ADDRESS_ERROR we have defined, so we could remove this one.

One more thing,
5. SMS_SEND_FAIL_RETRY_ERROR, I'd suggest to rename this error as RETRY_REQUIRED_ERROR instead for better understanding.

Comment 12

[Younghwan Ji]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> Hi Rian,
> 
> To merge the patch from Bugzilla to gecko, and have your contribution
> recorded we need a formal
> mercurial patch for the review process.
> After the formal patch attached, you can request a review? to me to start
> the review process. :)
> Would you please follow the chapters in the link below to request a review?
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code
>  1. Getting the code from the Mercurial repository
>     Note: Since this bug is to be landed in 2.2r branch, please clone the
> 2.2r repository 
>           instead of m-c.
>  2. Creating a patch
>  3. Try Servers/Getting commit Access to the source code.
>     This is nice to have to request Access level 1 to request your patch to
> be tested on the try server before reviewing and landing. The patch can only
> be landed if try server is green.
> 
> In addition, here are my comments for the patch in comment 6:
> 1. Please check the link below as an example to see what files to be
> modified if a new error cause is added.
>    https://hg.mozilla.org/mozilla-central/rev/c3202ada540e
>    They are
>    - dom/mobilemessage/interfaces/nsIMobileMessageCallback.idl
>    - dom/mobilemessage/src/MobileMessageCallback.cpp
>    - embedding/android/GeckoSmsManager.java
>    - mobile/android/base/GeckoSmsManager.java
> 2. For consistency of the naming, please have all the literal strings in
> MobileMessageCallback.cpp 
>    defined without space included.
> 3. Since all the error causes are explained in comment 5, we don't have to
> add these comments in the
>    implementation which could be found by blaming the change in the
> repository.
> 4. For INVALID_DESTINATION_ERROR, it's redundant to the
> INVALID_ADDRESS_ERROR we have defined, so
>    we could remove this one.
> 
> You can speed up the process with the comments above addressed.
> 
> Thanks for your contribution to make thing better. :)

Hi, Bevis.

I'm work with Rian.

Could you please tell me 2.2r repository name?
$ hg clone https://hg.mozilla.org/[[THIS]]

I can't find 2.2r repository name in https://hg.mozilla.org.

Comment 13

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

(In reply to Younghwan Ji from comment #12)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> > Hi Rian,
> > 
> > To merge the patch from Bugzilla to gecko, and have your contribution
> > recorded we need a formal
> > mercurial patch for the review process.
> > After the formal patch attached, you can request a review? to me to start
> > the review process. :)
> > Would you please follow the chapters in the link below to request a review?
> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code
> >  1. Getting the code from the Mercurial repository
> >     Note: Since this bug is to be landed in 2.2r branch, please clone the
> > 2.2r repository 
> >           instead of m-c.
> >  2. Creating a patch
> >  3. Try Servers/Getting commit Access to the source code.
> >     This is nice to have to request Access level 1 to request your patch to
> > be tested on the try server before reviewing and landing. The patch can only
> > be landed if try server is green.
> > 
> > In addition, here are my comments for the patch in comment 6:
> > 1. Please check the link below as an example to see what files to be
> > modified if a new error cause is added.
> >    https://hg.mozilla.org/mozilla-central/rev/c3202ada540e
> >    They are
> >    - dom/mobilemessage/interfaces/nsIMobileMessageCallback.idl
> >    - dom/mobilemessage/src/MobileMessageCallback.cpp
> >    - embedding/android/GeckoSmsManager.java
> >    - mobile/android/base/GeckoSmsManager.java
> > 2. For consistency of the naming, please have all the literal strings in
> > MobileMessageCallback.cpp 
> >    defined without space included.
> > 3. Since all the error causes are explained in comment 5, we don't have to
> > add these comments in the
> >    implementation which could be found by blaming the change in the
> > repository.
> > 4. For INVALID_DESTINATION_ERROR, it's redundant to the
> > INVALID_ADDRESS_ERROR we have defined, so
> >    we could remove this one.
> > 
> > You can speed up the process with the comments above addressed.
> > 
> > Thanks for your contribution to make thing better. :)
> 
> Hi, Bevis.
> 
> I'm work with Rian.
> 
> Could you please tell me 2.2r repository name?
> $ hg clone https://hg.mozilla.org/[[THIS]]
> 
> I can't find 2.2r repository name in https://hg.mozilla.org.

Hi Younghwan, here it is: http://hg.mozilla.org/releases/mozilla-b2g37_v2_2r

Thanks!

Comment 14

[Younghwan Ji]

Hi, Bevis.

I have a problem to commit modified source.

When I command "hg push", system response below;

pushing to ssh://hg.mozilla.org/releases/mozilla-b2g37_v2_2r
searching for changes
remote: abort: could not lock repository /repo/hg/mozilla/releases/mozilla-b2g37
abort: unexpected response: empty string

------------------------------------------
Should I get another permission to push commit?


This is Try server information;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=870ee9d0eb1a


Please help me.
Thank you.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

No, unfortunately, we are not allow to push the commit directly.

You should export the patch (hg export xxx > xxx.patch) and attach it to this bug to request a review.
After r+ granted, you can add "checkin-needed" in the |keywords| field of this bug with the positive try server result.
Then, the chief will help you to merge the patch marked as r+ to the corresponding branch.

In addition, for the try server request,
You can use the trychooser which is a hg extension with all unit tests and talos tests enabled on b2g(emulator) platform:
try: -b do -p emulator,emulator-jb,emulator-kk -u all -t all

[1] http://trychooser.pub.build.mozilla.org/

Comment 16

[Younghwan Ji]

Attachment: bug1175430.patch

Comment 17

[Younghwan Ji]

Hi, Bevis.

Thank you for your guide.

Could you please review attached patch file?

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

You can set the |review?| to me to request the review. :)

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dda24d9de95

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Wesley,

Can we set this bug as public for landing the patch?
Otherwise, we have to open a public one instead.

Comment 21

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I don't see confidential info in this bug.

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8644064 [details] [diff] [review]
bug1175430.patch

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

Hi Younghwan,

Please see my comment.
Please help to address these issues and request a review again with the new patch.

In addition.

Please have the comment of your patch in 1 line with reviewer info and 
change the description for better understanding. For example,
>  Bug 1175430 - Expose Network-Specific Error Cause for Various Error Handling in App Layer. r=btseng.

Thanks!

::: dom/mobilemessage/MobileMessageCallback.cpp
@@ +60,5 @@
>      case nsIMobileMessageCallback::SIM_NOT_MATCHED_ERROR:
>        errorStr = NS_LITERAL_STRING("SimNotMatchedError");
>        break;
> +    case nsIMobileMessageCallback::NETWORK_PROBLEMS_ERROR:
> +      errorStr = NS_LITERAL_STRING("NetworkProblems");

Please have the same convention to have "Error" appended at the end of error string.

s/NetworkProblems/NetworkProblemsError

Thanks!

@@ +63,5 @@
> +    case nsIMobileMessageCallback::NETWORK_PROBLEMS_ERROR:
> +      errorStr = NS_LITERAL_STRING("NetworkProblems");
> +      break;
> +    case nsIMobileMessageCallback::GENERAL_PROBLEMS_ERROR:
> +      errorStr = NS_LITERAL_STRING("GeneralProblems");

ditto.

s/GeneralProblems/GeneralProblemsError

@@ +66,5 @@
> +    case nsIMobileMessageCallback::GENERAL_PROBLEMS_ERROR:
> +      errorStr = NS_LITERAL_STRING("GeneralProblems");
> +      break;
> +    case nsIMobileMessageCallback::SERVICE_NOT_AVAILABLE_ERROR:
> +      errorStr = NS_LITERAL_STRING("ServiceNotAvailable");

ditto

s/ServiceNotAvailable/ServiceNotAvailableError

@@ +69,5 @@
> +    case nsIMobileMessageCallback::SERVICE_NOT_AVAILABLE_ERROR:
> +      errorStr = NS_LITERAL_STRING("ServiceNotAvailable");
> +      break;
> +    case nsIMobileMessageCallback::MESSAGE_TOO_LONG_FOR_NETWORK_ERROR:
> +      errorStr = NS_LITERAL_STRING("MessageTooLongForNetwork");

ditto

s/MessageTooLongForNetwork/MessageTooLongForNetworkError

@@ +72,5 @@
> +    case nsIMobileMessageCallback::MESSAGE_TOO_LONG_FOR_NETWORK_ERROR:
> +      errorStr = NS_LITERAL_STRING("MessageTooLongForNetwork");
> +      break;
> +    case nsIMobileMessageCallback::SERVICE_NOT_SUPPORTED_ERROR:
> +      errorStr = NS_LITERAL_STRING("ServiceNotSupported");

ditto

s/ServiceNotSupported/ServiceNotSupportedError

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Younghwan,

Please check comment 22 and request a review again by set the review flag to ? in your new patch.

Thanks! :)

Comment 24

[Younghwan Ji]

Attachment: Part 1: Expose More Network-Specific Error Cause for Various Error Handling in App Layer. r=btseng.

Hi, Bevis.
I attach new patch file and add set review flag to ?.
Please review again.
Thank you very much for your help.

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94c22134e9a2

Comment 26

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8645519 [details] [diff] [review]
Part 1: Expose More Network-Specific Error Cause for Various Error Handling in App Layer. r=btseng.

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

Looks good to me now.
BTW, thanks for removing extra alignment to make this patch more clear.

Let's wait for the test result on the try server before checking in this patch. :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94c22134e9a2

Comment 27

[Bevis Tseng[:bevis][:btseng](Exited)]

update correct assignee.

Comment 28

[Bevis Tseng[:bevis][:btseng](Exited)]

try server result is green.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/93db1a349ab9

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93db1a349ab9

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part2: Update UUID due to newly added const in xpcom interface.

Hi Edgar,

May I have your review for updating the uuid which was missed in patch part1?

Thanks!

Comment 32

[Bevis Tseng[:bevis][:btseng](Exited)]

reopen due to comment 31.

I'll update another patch for 2.2r with part1 and part2 merged together.

Sorry about missing the uuid part.

Comment 33

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: [2.2r] Expose More Network-Specific Error Cause for Various Error Handling in App Layer. r=btseng.

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

Set checkin-needed for Patch Part 2 in m-c.
Try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0879c8852db1

Comment 35

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8647378 [details] [diff] [review]
[2.2r] Expose More Network-Specific Error Cause for Various Error Handling in App Layer. r=btseng.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Carrier required feature.
Testing completed: Y
Risk to taking this patch (and alternatives if risky): N.
String or UUID changes made by this patch:846d74cc-4195-11e5-a968-2f4a44d4ba4c

try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f79bee7aa6a9

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8647374 [details] [diff] [review]
Part2: Update UUID due to newly added const in xpcom interface.

We have a mercurial commit hook that enforces UUID bumps on IDL changes. You can see in the commit that landed on m-c that it was indeed already done. So there's no need for this second patch.

Comment 37

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #36)
> Comment on attachment 8647374 [details] [diff] [review]
> Part2: Update UUID due to newly added const in xpcom interface.
> 
> We have a mercurial commit hook that enforces UUID bumps on IDL changes. You
> can see in the commit that landed on m-c that it was indeed already done. So
> there's no need for this second patch.

Thanks for informing this.
Change the status of this bug accordingly.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8647378 [details] [diff] [review]
[2.2r] Expose More Network-Specific Error Cause for Various Error Handling in App Layer. r=btseng.

feature-b2g:2.2r+ have auto-approval

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/449be47d995f