820220 - B2G SMS: extra octets sent in UCS2 encoded multipart messages

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

This bug was observed in re-producing bug 809553 but seems to be a different one. At sending multipart messages encoded in UCS2, GsmPDUHelper.writeUserDataHeader() adds extra octets as GSM 7-Bit national language identifiers. These octets are neither counted in the UDHL, nor in the UDL. SMSC may or may not accept these wrongly encoded TPDUs, and may be the root cause of bug 809533.

Blocks bug 809533 because no matter it duplicates bug 809533 or not, we still have to fix this first.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think bug 809533 is not the bug you're looking for :-)

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Sorry, it's bug 809553. BB? according to bug 809553 comment #3.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1/2: fix extra octets sent in multipart UCS2 encoded messages

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2/2: xpcshell test case

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [DEBUG] Gecko

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 690691 [details] [diff] [review]
Part 1/2: fix extra octets sent in multipart UCS2 encoded messages

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

::: dom/system/gonk/ril_worker.js
@@ +2528,5 @@
>     */
>    sendSMS: function sendSMS(options) {
> +    if (options.verified == null) {
> +      options.verified = true;
> +

seems we don't need this.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 690692 [details] [diff] [review]
Part 2/2: xpcshell test case

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

::: dom/system/gonk/tests/test_ril_worker_sms.js
@@ +713,5 @@
>    }
>  }
>  test_receiving_ucs2_alphabets(ucs2str);
>  
> +// Bug 809553: B2G SMS: wrong order and truncated content in multi-part messages

wrong bug id?

@@ +725,5 @@
> +    // + 1(first octet) + 1(message reference)
> +    // + 2(DA len, TOA) + 4(addr)
> +    // + 1(pid) + 1(dcs)
> +    // + 1(UDL) + 6(UDHL, type, len, ref, max, seq)
> +    // + 12(2 * strlen("Hello "))

should be strlen("Hello World!"), right?

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #6)
> Part 1/2: fix extra octets sent in multipart UCS2 encoded messages
> Review of attachment 690691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2528,5 @@
> >     */
> >    sendSMS: function sendSMS(options) {
> > +    if (options.verified == null) {
> > +      options.verified = true;
> > +
> 
> seems we don't need this.

We may call sendSMS with the same options object, so we should not verify it twice.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #7)
> Part 2/2: xpcshell test case
> Review of attachment 690692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/tests/test_ril_worker_sms.js
> @@ +713,5 @@
> >    }
> >  }
> >  test_receiving_ucs2_alphabets(ucs2str);
> >  
> > +// Bug 809553: B2G SMS: wrong order and truncated content in multi-part messages
> 
> wrong bug id?

Oops!

> @@ +725,5 @@
> > +    // + 1(first octet) + 1(message reference)
> > +    // + 2(DA len, TOA) + 4(addr)
> > +    // + 1(pid) + 1(dcs)
> > +    // + 1(UDL) + 6(UDHL, type, len, ref, max, seq)
> > +    // + 12(2 * strlen("Hello "))
> 
> should be strlen("Hello World!"), right?

Each sendParcel() call represents one outgoing segment of a multipart SMS message. Here, we have the first segment send, so it's "Hello " only. I'll update comments to make it more clear.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1/2: fix extra octets sent in multipart UCS2 encoded messages : v2

per offline discuss, address comment #8

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2/2: xpcshell test case : v2

address review comment #9

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1562979f08f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e217e280f14a

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1562979f08f8
https://hg.mozilla.org/mozilla-central/rev/e217e280f14a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dda8dc606878
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8123546a8b4

https://hg.mozilla.org/releases/mozilla-b2g18/rev/88606e02ee17
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e9df06a23fbc