1085765 - [FFOS2.0][Woodduck][SMS][Voicemail]MS can't receive the "Voice Mail Notification"when we don't input text content.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P2)

Description

[sync-1]

DEFECT DESCRIPTION:
 MS can not receive this "Voice Mail Notification".
 
 REPRODUCING PROCEDURES:
 1.Use NOW SMS to send Voicemail/fax/email/video/other type or clear type voicemail/fax/email/video/other  "Voice Mail Notification" without text.
 
 EXPECTED BEHAVIOUR:
 MS should receive these type of "Voice Mail Notification".
 >Remark:when we send these "Voice Mail Notification" with text,they can be receive successfully.
 
 Code analyse:
 Now I modify the following code and seem fix this bug, pls review it: 
 /gecko/dom/system/gonk/ril_worker.js
   /**
    * User data can be 7 bit (default alphabet) data, 8 bit data, or 16 bit
    * (UCS2) data.
    *
    *  msg
    *        message object for output.
    *  length
    *        length of user data to read in octets.
    */
   readUserData: function(msg, length) {
     ......
     switch (msg.encoding) {
       case PDU_DCS_MSG_CODING_7BITS_ALPHABET:
         // 7 bit encoding allows 140 octets, which means 160 characters
         // ((140x8) / 7 = 160 chars)
         ......
         // Add condition by tao.li.hz.com, if no user data, don't read it.
         if (length > 0) {
           msg.body = this.readSeptetsToString(length, paddingBits, langIndex,
                                               langShiftIndex);
                 }
         break;
       ......
     }
   },

Comment 1

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

Hi,

Can we have the radio log of this symptom for further analysis?
adb logcat -v threadtime -b radio > radio.txt
keyword: UNSOL_RESPONSE_NEW_SMS

Then, we can reproduce it locally with the SMS PDU to see what happened and write it into the test case if needed.

Thanks!

Comment 2

[sync-1]

Created an attachment (id=982241)
 RadioAndMainLog.rar
 
 Radio and main log as attachment.

Comment 3

[sync-1]

Created an attachment (id=982241)
 RadioAndMainLog.rar
 
 Radio and main log as attachment.

Comment 4

[sync-1]

Attachment: RadioAndMainLog.rar

Created an attachment (id=982241)
 RadioAndMainLog.rar
 
 Radio and main log as attachment.

Comment 5

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

Thanks for the log.

I can reproduce it locally.
It seems that there is no correct boundary check in GsmPDUHelperObject.readSeptetsToString() when length is 0 and then, an exception was thrown at 
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#7048

Comment 6

[sync-1]

Received a normal SMS, thf following log will appear:
 RIL Worker: [0] Solicited response for request type 37, token 152, error 0
 
 Received a voicemail SMS, the following log will appear:
 RIL Worker: [0] Solicited response for request type 37, token 124, error 2
 Here request type 37 means SMS_ACKNOWLEDGE.
 
 Please help to analyse why voicemail SMS have "error 2 log", thanks.

Comment 7

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

(In reply to sync-1 from comment #6)
> Received a normal SMS, thf following log will appear:
>  RIL Worker: [0] Solicited response for request type 37, token 152, error 0
>  
>  Received a voicemail SMS, the following log will appear:
>  RIL Worker: [0] Solicited response for request type 37, token 124, error 2
>  Here request type 37 means SMS_ACKNOWLEDGE.
>  
>  Please help to analyse why voicemail SMS have "error 2 log", thanks.
There is no difference to send the positive SMS_ACK to rild.
The error was reported by rild/modem and "2" means ERROR_GENERIC_FAILURE.
We cannot help too much for this from upper layer.
Please have vendor to check why this error was returned instead.

Thanks!

Comment 8

[sync-1]

(In reply to comment #5)
 > Comment from Mozilla:(In reply to sync-1 from comment #6)
 > > Received a normal SMS, thf following log will appear:
 > >  RIL Worker: [0] Solicited response for request type 37, token 152, error 0
 > >  
 > >  Received a voicemail SMS, the following log will appear:
 > >  RIL Worker: [0] Solicited response for request type 37, token 124, error 2
 > >  Here request type 37 means SMS_ACKNOWLEDGE.
 > >  
 > >  Please help to analyse why voicemail SMS have "error 2 log", thanks.
 > There is no difference to send the positive SMS_ACK to rild.
 > The error was reported by rild/modem and "2" means ERROR_GENERIC_FAILURE.
 > We cannot help too much for this from upper layer.
 > Please have vendor to check why this error was returned instead.
 > Thanks!
 
 OK, I will check it with vendor.

Comment 9

[sync-1]

(In reply to comment #4)
 > Comment from Mozilla:Thanks for the log.
 > I can reproduce it locally.
 > It seems that there is no correct boundary check in
 > GsmPDUHelperObject.readSeptetsToString() when length is 0 and then, an
 > exception was thrown at 
 > http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#7048
 
 If there is no user data, msg.body should be null.

Comment 10

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

Attachment: Part 1 v1: Bail out earlier from GsmPDUHelper.readUserData() when there is no further data to read.

Hi Hsinyi,

May I have your review for this fix?

Thanks!

Comment 11

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

Attachment: Part 1 v1: Bail out earlier from GsmPDUHelper.readUserData() when there is no further data to read.

Hi Hsinyi,

May I have your review for this fix?

Thanks!

Comment 12

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

Attachment: Part 2 v1: Add test case to verify MWI without text body.

Hi Hsinyi,

May I have your review for the change in voicemail test cases?

Thanks!

Comment 13

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

Attachment: Part 2 v1: Add test case to verify MWI without text body.

Hi Hsinyi,

May I have your review for the change in voicemail test cases?

Thanks!

Comment 14

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

Comment on attachment 8508565 [details] [diff] [review]
Part 1 v1: Bail out earlier from GsmPDUHelper.readUserData() when there is no further data to read.

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

Nice catch!

::: dom/system/gonk/ril_worker.js
@@ +7856,5 @@
>  
>      msg.body = null;
>      msg.data = null;
> +
> +    if (length <= 0) {

nit: isn't a check |!length| sufficient? do we really need to worry the case of length being smaller than 0?

Comment 15

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

Comment on attachment 8508570 [details] [diff] [review]
Part 2 v1: Add test case to verify MWI without text body.

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

Thank you :)

Comment 16

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> Comment on attachment 8508565 [details] [diff] [review]
> Part 1 v1: Bail out earlier from GsmPDUHelper.readUserData() when there is
> no further data to read.
> 
> Review of attachment 8508565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice catch!
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +7856,5 @@
> >  
> >      msg.body = null;
> >      msg.data = null;
> > +
> > +    if (length <= 0) {
> 
> nit: isn't a check |!length| sufficient? do we really need to worry the case
> of length being smaller than 0?

Per offline discussion, this is just in case if the PDU is malformed with incorrect user data length. :)

Comment 17

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

Attachment: Part 1: Bail out earlier from GsmPDUHelper.readUserData() when there is no further data to read. r=htsai

update final patch with positive try server result.

Comment 18

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

Attachment: Part 2: Add test case to verify MWI without text body. r=htsai

update final patch with positive try server result.

Comment 19

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

Attachment: [2.0] Part 1: Bail out earlier from GsmPDUHelper.readUserData() when there is no further data to read. r=htsai

update patch for v2.0

Comment 20

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

Attachment: [2.0] Part 2: Add test case to verify MWI without text body. r=htsai

update patch for v2.0

Comment 21

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

Attachment: [2.0] Part 2: Add test case to verify MWI without text body. r=htsai

update v2.0 patch.

Comment 22

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

update try server result in m-c:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69bd2445373d

Comment 23

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

update try server result in 2.0v:
https://tbpl.mozilla.org/?tree=Try&rev=74357d710019
update try server result in 2.1v:
https://tbpl.mozilla.org/?tree=Try&rev=4baea36ca0fe

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/de549f759bbb
https://hg.mozilla.org/integration/b2g-inbound/rev/1ee4c707d6b3

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de549f759bbb
https://hg.mozilla.org/mozilla-central/rev/1ee4c707d6b3

Comment 26

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

Comment on attachment 8509231 [details] [diff] [review]
[2.0] Part 1: Bail out earlier from GsmPDUHelper.readUserData() when there is no further data to read. r=htsai

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: 
Without this patch, it is not possible for the device user to be notified his/her voicemail status if the Voicemail Notification from network delivered w/o text body. The fix was located at ril_worker.js. There is no impact for QCT's CAF RIL like 2.0v but will impact for 2.0m branch.

Testing completed: Test case included to enhance the test coverage.

Risk to taking this patch (and alternatives if risky): No

String or UUID changes made by this patch: No

Comment 27

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

Comment on attachment 8509235 [details] [diff] [review]
[2.0] Part 2: Add test case to verify MWI without text body. r=htsai

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: 
Without this patch, it is not possible for the device user to be notified his/her voicemail status if the Voicemail Notification from network delivered w/o text body. The fix was located at ril_worker.js. There is no impact for QCT's CAF RIL like 2.0v but will impact for 2.0m branch.

Testing completed: Test case included to enhance the test coverage.

Risk to taking this patch (and alternatives if risky): No

String or UUID changes made by this patch: No

Comment 28

[Josh Cheng [:josh]]

Triage:
Hi Kai-Zhen,
2.0M+. Thanks!

Comment 29

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/dd43498b7463
http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/76838404c71b

Comment 30

[Kai-Zhen Li [:seinlin][:kli]]

Per comment 24.

Comment 31

[bhavana bajaj [:bajaj]]

Bevis, is this a 2.0 regression ? Is this reproducible on Flame ?

Comment 32

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

(In reply to bhavana bajaj [:bajaj] from comment #31)
> Bevis, is this a 2.0 regression ? Is this reproducible on Flame ?

No, this is not a regression and is reproducible on Flame with MOZ RIL.

Comment 33

[bhavana bajaj [:bajaj]]

Comment on attachment 8509231 [details] [diff] [review]
[2.0] Part 1: Bail out earlier from GsmPDUHelper.readUserData() when there is no further data to read. r=htsai

Discussed offline with Bevis/Ken on not taking this for 2.0 at this point.
For 2.1, once we have a necessary branches created we can land this at that point.

Comment 34

[sync-1]

请关闭该问题,谢谢.

Comment 35

[bhavana bajaj [:bajaj]]

All the RIL landings will have to wait till we have a separate branch created, so clearing them up the approval request for now. Once the branch is ready we can land with with a=bajaj.