B2G SMS: Exception in processParcel() at receiving multipart SMS messages (>= 379 chars) sent from emulator console

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: rwood, Assigned: vicamo)

Tracking

unspecified
mozilla20
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

On the b2g device emulator:

Simulate an incoming SMS (via emulator's 'sms send' command) and make the SMS text length to be 379 characters or greater.  The first time, the incoming message is received fine.  Now repeat the same thing, use the emulator command and simulate a second incoming SMS of 379 or greater - notice this time the SMS is not received. The emulator callback works / the emulator returns 'OK' after issuing the 'sms send' command but the 'sms.onreceived' event is never fired.

Having two incoming SMS messages of 378 characters or less works fine, but two incoming messages of 379 characters or above will reproduce the problem on the second message.  Also note that simulating an incoming SMS with a larger number of characters (i.e. 1600) works fine on the first message.

Reproduce by running the attached marionette WebSMS script.  Note that it can also be reproduced manually via telnet to the emulator, so it is not a marionette issue.
Posted file Marionette WebSMS test script (obsolete) —
Blocks: 806811
I found ril_worker throws an exception at receiving the third segment. `TypeError: options is undefined in line 589`.

 579   processParcel: function processParcel() {
 ...
 587       options = this.tokenRequestMap[token];
 588       delete this.tokenRequestMap[token];
 589       request_type = options.rilRequestType;
It's funny that I got my own tests, too.

addr_1: 5551110000
addr_2: 5554
body_1: "a" * 380
body_2: "1" * 10 + "2" * 10 + ... + "0" * 10

Only addr_2 with body_1 triggers this issue.
Summary: [WebAPI] WebSMS: Simulating incoming SMS of >= 379 chars (on emulator) fails on second time → B2G SMS: Exception in processParcel() at receiving multipart SMS messages (>= 379 chars) sent from emulator console
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> Created attachment 685538 [details]
> I/Gecko   (   42): RIL Worker: New incoming parcel of size 656
> I/Gecko   (   42): RIL Worker: Read 360, but parcel size is 656. Going to sleep.
> I/Gecko   (   42): RIL Worker: Received 668 bytes.
> I/Gecko   (   42): RIL Worker: Already read 360
> I/Gecko   (   42): RIL Worker: Parcel (size 656): 0,0,0,0,235,3,0,0,64,1,0,0,48,0, ....

Something wrong in emulator or hardware/ril. Incoming SMS should be a unsolicited request, so we should have had "1,0,0,0,235,3,0,0,..." here.

> I/Gecko   (   42): RIL Worker: We have at least one complete parcel.
> I/Gecko   (   42): RIL Worker: Parcel handling threw TypeError: options is undefined
> I/Gecko   (   42): processParcel@resource://gre/modules/ril_worker.js:589

And parcel handler should not failed here for a unmatched solicited request as well.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> It's funny that I got my own tests, too. Only addr_2 with body_1 triggers this issue.

Actually other combinations also cause wrong parcel data read, but their request_type is some arbitrary number and is therefore recognized as an unsolicited request.
When we try to grow incoming buffer in function writeToIncoming(), the minimum size is currently set to incoming.length. It's incorrect and should be set to (incoming.length + this.readIncoming) so that we can prevent previous read but not consumed yet data being overwritten.
Assignee: nobody → vyang
Blocks: 759529
Yoshi, would you like to review this patch?
Attachment #688241 - Flags: review?(allstars.chh)
Posted patch Part 2: add test case (obsolete) — Splinter Review
Hi Rob,

This patch is based on you attachment #684763 [details] with a small fix for pending emulator response. I still list you as the author.
Attachment #684763 - Attachment is obsolete: true
Attachment #688242 - Flags: review?(rwood)
Comment on attachment 688242 [details] [diff] [review]
Part 2: add test case

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

Looks good, thanks Vicamo.
Attachment #688242 - Flags: review?(rwood) → review+
Attachment #688241 - Flags: review?(allstars.chh) → review+
Add r=yoshi. Already r+.
Attachment #688241 - Attachment is obsolete: true
Posted patch Part 2: add test cases (obsolete) — Splinter Review
Hi Rob, I add xpcshell test case as well because the marionette test depends on SMS functions and this issue should be a generic Buf operation bug. Please help reviewing this patch again. Thank you :)
Attachment #688242 - Attachment is obsolete: true
Attachment #688650 - Flags: review?(rwood)
Comment on attachment 688650 [details] [diff] [review]
Part 2: add test cases

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

Hi Vicamo, I have never written an xpcshell test and am not familiar with them at all, so I am not qualified to review that particular test. I am unsure whom to suggest instead, maybe one of the B2G peers?  https://wiki.mozilla.org/Modules/Boot2Gecko
Attachment #688650 - Flags: review?(rwood) → review-
Comment on attachment 688650 [details] [diff] [review]
Part 2: add test cases

Hi Yoshi, would you like to review the xpcshell test cases?
Attachment #688650 - Flags: review- → review?(allstars.chh)
Comment on attachment 688650 [details] [diff] [review]
Part 2: add test cases

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

LGTM,
Can you explain and add comment for the +4 in parcel size?

Thanks

::: dom/system/gonk/tests/test_ril_worker_buf.js
@@ +111,5 @@
> +
> +  // Prepare two parcels, whose size are both smaller than the incoming buffer
> +  // size but larger when combined, to trigger the bug.
> +  let pA_dataLength = buf.INCOMING_BUFFER_LENGTH / 2;
> +  let pA_parcelSize = pA_dataLength + worker.PARCEL_SIZE_SIZE + 4;

Could you explain why +4 here?

@@ +117,5 @@
> +                             worker.RESPONSE_TYPE_UNSOLICITED,
> +                             request,
> +                             calloc(pA_dataLength, 1));
> +
> +  let pB_dataLength = buf.INCOMING_BUFFER_LENGTH *3 / 4;

nit, space
Attachment #688650 - Flags: review?(allstars.chh) → review+
Address review comment #14. Actually, the expression for parcel size is semantically wrong. They should have been what they look like now.
Attachment #688650 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4130f20282de
https://hg.mozilla.org/mozilla-central/rev/957e9d5fe527
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 688647 [details] [diff] [review]
Part 1: fix ril worker exception due to buffer overwritten: v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 699256
User impact if declined: might not receive multipart SMS message correctly
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?rev=00e26dece6ea
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Attachment #688647 - Flags: approval-mozilla-beta?
Attachment #688647 - Flags: approval-mozilla-aurora?
Attachment #690294 - Flags: approval-mozilla-beta?
Attachment #690294 - Flags: approval-mozilla-aurora?
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> Risk to taking this patch (and alternatives if risky): 

To be able to make a call on this non-b-b+ bug, we need a risk evaluation.
Attachment #688647 - Flags: approval-mozilla-beta? → approval-mozilla-b2g18?
Attachment #690294 - Flags: approval-mozilla-beta? → approval-mozilla-b2g18?
(In reply to Alex Keybl [:akeybl] from comment #19)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> > Risk to taking this patch (and alternatives if risky): 
> 
> To be able to make a call on this non-b-b+ bug, we need a
> risk evaluation.

This is actually a bug that had existed since very early RIL implementation. It's not some new feature, workaround or the like. Without this, transactions of two huge, successional parcels may fail with exceptions shown comment #2. The only known victim is SMS for now.

IMHO, there is no risk approving uplift of this patch.
Comment on attachment 688647 [details] [diff] [review]
Part 1: fix ril worker exception due to buffer overwritten: v2

Low or no risk, and impact automated testing. Approving for branches.
Attachment #688647 - Flags: approval-mozilla-b2g18?
Attachment #688647 - Flags: approval-mozilla-b2g18+
Attachment #688647 - Flags: approval-mozilla-aurora?
Attachment #688647 - Flags: approval-mozilla-aurora+
Attachment #690294 - Flags: approval-mozilla-b2g18?
Attachment #690294 - Flags: approval-mozilla-b2g18+
Attachment #690294 - Flags: approval-mozilla-aurora?
Attachment #690294 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.