Closed Bug 820220 Opened 7 years ago Closed 7 years ago

B2G SMS: extra octets sent in UCS2 encoded multipart messages

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
I think bug 809533 is not the bug you're looking for :-)
Sorry, it's bug 809553. BB? according to bug 809553 comment #3.
Assignee: nobody → vyang
Blocks: 759529
blocking-basecamp: --- → ?
Blocks: 809553
No longer blocks: 809533
Attached patch Part 2/2: xpcshell test case (obsolete) — Splinter Review
Attachment #690692 - Flags: review?(allstars.chh)
Attached patch [DEBUG] GeckoSplinter Review
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.
Attachment #690691 - Flags: review?(allstars.chh)
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?
Attachment #690692 - Flags: review?(allstars.chh)
(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.
(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.
per offline discuss, address comment #8
Attachment #690691 - Attachment is obsolete: true
Attachment #690786 - Flags: review?(allstars.chh)
address review comment #9
Attachment #690692 - Attachment is obsolete: true
Attachment #690787 - Flags: review?(allstars.chh)
Attachment #690786 - Flags: review?(allstars.chh) → review+
Attachment #690787 - Flags: review?(allstars.chh) → review+
https://hg.mozilla.org/mozilla-central/rev/1562979f08f8
https://hg.mozilla.org/mozilla-central/rev/e217e280f14a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
blocking-basecamp: ? → +
Duplicate of this bug: 820723
You need to log in before you can comment on or make changes to this bug.