Closed
Bug 814761
Opened 8 years ago
Closed 8 years ago
B2G SMS: Exception in processParcel() at receiving multipart SMS messages (>= 379 chars) sent from emulator console
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: rwood, Assigned: vicamo)
References
Details
Attachments
(3 files, 4 obsolete files)
14.98 KB,
text/plain
|
Details | |
2.12 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
6.19 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
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;
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
(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.
Assignee | ||
Comment 6•8 years ago
|
||
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
Assignee | ||
Comment 7•8 years ago
|
||
Yoshi, would you like to review this patch?
Attachment #688241 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 8•8 years ago
|
||
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)
Reporter | ||
Comment 9•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #688241 -
Flags: review?(allstars.chh) → review+
Assignee | ||
Comment 10•8 years ago
|
||
Add r=yoshi. Already r+.
Attachment #688241 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
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)
Reporter | ||
Comment 12•8 years ago
|
||
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-
Assignee | ||
Comment 13•8 years ago
|
||
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+
Assignee | ||
Comment 15•8 years ago
|
||
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
Assignee | ||
Comment 16•8 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/4130f20282de http://hg.mozilla.org/integration/mozilla-inbound/rev/957e9d5fe527
Comment 17•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4130f20282de https://hg.mozilla.org/mozilla-central/rev/957e9d5fe527
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Assignee | ||
Comment 18•8 years ago
|
||
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?
Assignee | ||
Updated•8 years ago
|
Attachment #690294 -
Flags: approval-mozilla-beta?
Attachment #690294 -
Flags: approval-mozilla-aurora?
Comment 19•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #688647 -
Flags: approval-mozilla-beta? → approval-mozilla-b2g18?
Updated•8 years ago
|
Attachment #690294 -
Flags: approval-mozilla-beta? → approval-mozilla-b2g18?
Assignee | ||
Comment 20•8 years ago
|
||
(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 21•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #690294 -
Flags: approval-mozilla-b2g18?
Attachment #690294 -
Flags: approval-mozilla-b2g18+
Attachment #690294 -
Flags: approval-mozilla-aurora?
Attachment #690294 -
Flags: approval-mozilla-aurora+
Comment 22•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/fbaa50688290 https://hg.mozilla.org/releases/mozilla-aurora/rev/f0dd7c6613c8 https://hg.mozilla.org/releases/mozilla-b2g18/rev/3dc7f40480de https://hg.mozilla.org/releases/mozilla-b2g18/rev/e09d309b5145
You need to log in
before you can comment on or make changes to this bug.
Description
•