Closed Bug 945089 Opened 11 years ago Closed 10 years ago

There is some case that concatenated SMS data is lost

Categories

(Firefox OS Graveyard :: RIL, defect)

Product:
Firefox OS Graveyard
Component:
RIL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed, b2g-v2.0 unaffected)

Status:
RESOLVED FIXED
Milestone:
1.4 S3 (14mar)
Project Flags:
blocking-b2g 1.4+
Tracking Flags:
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- unaffected

People

(Reporter: mu-ota, Assigned: bevis)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(4 files, 12 obsolete files)

Patch Part 2: Revise deprecated test cases. r=vyang
2.36 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
Patch Part 3 v3: Add new test cases to test the concatenation of text/binary sms. r=vyang
7.09 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
Patch Part 1.1 v6: Create new ObjectStore for saving uncomplete SMS segments. r=vyang
12.68 KB, patch
bevis
: review+
Details | Diff | Splinter Review
Patch Part 1.2 v6: Move SMS concatenation logic from ril_worker.js to RadioInterfaceLayer.js. r=vyang
29.53 KB, patch
vicamo
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.57 Safari/537.36

Steps to reproduce:

1.First Concatenated SMS is received. (waiting next concatenated SMSs:in case receivedSegments is smaller than segmentMaxSeq) 
2.B2g process is restart(ex B2g process is killed)


Actual results:

The received concatenated SMS data(in hashmap) is lost.
(Concatenated SMS is not stored in DB until all concatenated SMSs have received)


Expected results:

Concatenated SMS is stored in DB.(even if All SMSs has not received)
I think this case maybe is little.
But Data Packet lost is serious issue, because User payed money packet data.
Component: Gaia::SMS → RIL
This may need to pull nearly everything related to SMS out from ril_worker to some higher layer, store TPDUs in yet another database.
Hi Vicamo-san,

When the device save concatenated SMS data for non-volatilization, I think that it is necessary to consider it about the cleaning timing of concatenated SMS data.

I am glad when you consider it in conjunction with the contents which Ohta-san listed.
Hi at-kitamura-san,

I would prefer to address this issue after bug 873351, which is going to move all SMS processing code out of RadioInterfaceLayer.  Then we can continue to extract more SMS code, especially concatenated SMS segments caching, into SmsService.  Not a must, but will surely have a cleaner source code layout.
Depends on bug 873351 based on comment #4 for now.
Depends on: 873351
Vicamo, after discussing with KDDI, it is a 1.4+ bug. Let's see if it's necessary to only fix it after fixing bug 873351? And can you take this bug?
blocking-b2g: --- → 1.4+
Flags: needinfo?(vyang)
To change internal ril interface.
Blocks: b2g-ril-interface
Flags: needinfo?(vyang)
QA Contact: vyang
I think Vicamo want to take this bug.
Assignee: nobody → vyang
QA Contact: vyang
Status: UNCONFIRMED → NEW
Ever confirmed: true
This doesn't fall under the QC blocking feature list & DSDS feature list for 1.4. Renoming.
blocking-b2g: 1.4+ → 1.4?
Ken

Please review if this is needed for any of the features in 1.4? The new ask list from QC.
Flags: needinfo?(kchang)
It is requested by Carrier.
Flags: needinfo?(kchang)
Vicamo is busy on preparing IPv6 patch for tokyo's workshop. Bevis, please take this bug. Thanks very much.
Assignee: vyang → btseng
Blocking on 1.4+ to meet Carrier request.
blocking-b2g: 1.4? → 1.4+
Whiteboard: [FT:RIL]
Target Milestone: --- → 1.4 S3 (14mar)
PM Triage: remains 1.4+
Blocks: 972182
Comment on attachment 8387393 [details] [diff] [review]
Patch Part 1 v1: Save SMS PDU into DB to prevent data lost of the concatenated SMS when device reboot. r=vyang

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +1356,5 @@
> +    /**
> +     * This smsSegmentStore is used to store uncomplete SMS segments.
> +     * Each entry looks like this:
> +     *
> +     * { hash: <String> (primary key),

nit: please leave key composition job to database itself.  Composing keys outside db itself means the way to compose it becomes a convention to all db clients.  This loses some flexibility when we're to refactor/improve the performance of that db because it's quite impossible to ask all implementations to follow at the same time.  So, define as few attributes as possible and leave db task, schema maintenance to db itself.

This also follows we have to store |iccId| as well.

@@ +1357,5 @@
> +     * This smsSegmentStore is used to store uncomplete SMS segments.
> +     * Each entry looks like this:
> +     *
> +     * { hash: <String> (primary key),
> +     *   messageType: <Number>,

Please group the attributes.  Some of them are only referenced once.

  * [ Only referenced at the first time. ]
  *
  * messageType: <Number>,
  * teleservice: <Number>,
  * ...

@@ +1361,5 @@
> +     *   messageType: <Number>,
> +     *   teleservice: <Number>,
> +     *   SMSC: <String>,
> +     *   sentTimestamp: <Number>,
> +     *   sender: <String>,

* [ Referenced in every call to |saveSmsSegment()| ]
*
* sender: <String>,

@@ +1365,5 @@
> +     *   sender: <String>,
> +     *   encoding: <Number>,
> +     *   messageClass: <String>,
> +     *   header: {
> +     *     segmentRef: <Number>,

Since we're to reassign corresponding fields to a new savable object, we may omit this header structure.  Just use |obj.segmentRef| ...

@@ +1372,5 @@
> +     *     originatorPort: <Number>,
> +     *     destinationPort: <Number>,
> +     *   },
> +     *   mwi: {
> +     *     discard: <Boolean>,

ditto

@@ +1382,5 @@
> +     *   serviceCategory: <Number>,
> +     *   language: <String>,
> +     *
> +     *   // Handy fields for concatenation.
> +     *   segmentMaxSeq: <Number>,

nit: we can have it from |header.segmentMaxSeq|.

@@ +1385,5 @@
> +     *   // Handy fields for concatenation.
> +     *   segmentMaxSeq: <Number>,
> +     *   receivedSegments: <Number>,
> +     *   segments: [],
> +     *   timestamp: <Number> }

Please also group fields created by mmdb itself.

@@ +1388,5 @@
> +     *   segments: [],
> +     *   timestamp: <Number> }
> +     *
> +     */
> +    let smsSegmentStore = db.createObjectStore(SMS_SEGMENT_STORE_NAME,

nit: unreferenced `smsSegmentStore`.

@@ +1389,5 @@
> +     *   timestamp: <Number> }
> +     *
> +     */
> +    let smsSegmentStore = db.createObjectStore(SMS_SEGMENT_STORE_NAME,
> +                                               { keyPath: "hash" });

IndexedDB records are sorted by primary key in ascending order.  Using string primary key here means there will be many record re-orders whenever a new record is inserted.  Please use a numeric 'id' and set 'autoIncrement' to true.

You'll also need an index for that hash string.

@@ +2498,5 @@
> +          aSmsSegment.segments = [];
> +          aSmsSegment.timestamp = Date.now();
> +          if (aSmsSegment.encoding == RIL.PDU_DCS_MSG_CODING_8BITS_ALPHABET) {
> +            aSmsSegment.segments[seq] = aSmsSegment.data;
> +            delete aSmsSegment.data;

Why should we delete |aSmsSegment.data| here?

@@ +2501,5 @@
> +            aSmsSegment.segments[seq] = aSmsSegment.data;
> +            delete aSmsSegment.data;
> +          } else {
> +            aSmsSegment.segments[seq] = aSmsSegment.body;
> +            delete aSmsSegment.body;

ditto

@@ +2506,5 @@
> +          }
> +
> +          let addRequest = segmentStore.add(aSmsSegment);
> +          addRequest.onsuccess = function(event) {
> +            aCallback.notify(Cr.NS_OK, null);

Please use |txn.oncomplete| to execute callbacks as possible, or it will block further transactions and reduce IndexedDB throughput.

@@ +2508,5 @@
> +          let addRequest = segmentStore.add(aSmsSegment);
> +          addRequest.onsuccess = function(event) {
> +            aCallback.notify(Cr.NS_OK, null);
> +          };
> +          addRequest.onerror = function(event) {

ditto.

@@ +2516,5 @@
> +              }
> +            }
> +            aCallback.notify(Cr.NS_ERROR_FAILURE, null);
> +          };
> +        } else {

nit: bail-out early.

@@ +2539,5 @@
> +          // If the segments of a WAP Push are not received in sequence
> +          // (e.g., SMS with seq == 1 is not the 1st segment received by the device),
> +          // we have to retrieve the port information from 1st segment and
> +          // save it into the cached msgRecord.header.
> +          if (aSmsSegment.teleservice === RIL.PDU_CDMA_MSG_TELESERIVCIE_ID_WAP && seq === 1) {

nit: line wrap at 80 chars boundary.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2966,5 @@
> +   * Hash map for received multipart sms fragments. Messages are hashed with
> +   * its sender address and concatenation reference number. Three additional
> +   * attributes `segmentMaxSeq`, `receivedSegments`, `segments` are inserted.
> +   */
> +  _receivedSmsSegmentsMap: null,

We have already pushed the segments into database, why do we need another runtime cache map?
Attachment #8387393 - Flags: review?(vyang)
Attachment #8387394 - Flags: review?(vyang) → review+
Comment on attachment 8387395 [details] [diff] [review]
Patch Part 3: Add new test cases to test the concatenation of text/binary sms. r=vyang

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

::: dom/mobilemessage/tests/marionette/test_mt_sms_concatenation.js
@@ +108,5 @@
> +}
> +
> +function sendRawSmsAndWait(aPdus) {
> +  for (let pdu of aPdus) {
> +    sendRawSmsToEmulator(pdu);

Please help document the `aPdu` parameter of |sendRawSmsToEmulator|.  "A hex string representing the whole SMS T-PDU." would be nice.  Thank you :)

@@ +109,5 @@
> +
> +function sendRawSmsAndWait(aPdus) {
> +  for (let pdu of aPdus) {
> +    sendRawSmsToEmulator(pdu);
> +  }

let promises = [];

promises.push(waitForManagerEvent("received"));
for (let pdu of aPdus) {
  promises.push(sendRawSmsToEmulator(pdu));
}

return Promise.all(promises);

@@ +123,5 @@
> +
> +function verifyBinaryMessage(aMessage) {
> +  is(aMessage.type, "mms", "MmsMessage type");
> +  is(aMessage.delivery, "not-downloaded", "MmsMessage delivery");
> +  // remove duplicated M-Notification.ind for next test.

nit: an empty line before this comment.

@@ +125,5 @@
> +  is(aMessage.type, "mms", "MmsMessage type");
> +  is(aMessage.delivery, "not-downloaded", "MmsMessage delivery");
> +  // remove duplicated M-Notification.ind for next test.
> +  return Promise.resolve()
> +         .then(deleteMessagesById([aMessage.id]));

You can simply do:

  return deleteMessagesById([aMessage.id]);

@@ +134,5 @@
> +  return sendRawSmsAndWait(buildTextPdus(PDU_DCS_NORMAL_UCS2))
> +    .then(function(aReceivedMessage) {
> +      return Promise.resolve()
> +        .then(verifyTextMessage.bind(null, aReceivedMessage, "normal"));
> +    });

You may have a simpler form instead:

  return sendRawSmsAndWait(buildTextPdus(PDU_DCS_NORMAL_UCS2))
    .then((aReceivedMessage) => verifyTextMessage(aReceivedMessage, "normal"));

@@ +138,5 @@
> +    });
> +}
> +
> +function testClass0Text() {
> +  log("testClass0Text()");

Looks like you can unify testNormalText and testClass0Text as testText(aDcs, aMessageClass)

@@ +156,5 @@
> +    });
> +}
> +
> +function testClass0Binary() {
> +  log("testClass0Binary()");

ditto. testBinary(aDcs)

@@ +165,5 @@
> +    });
> +}
> +
> +startTestCommon(function testCaseMain() {
> +  manager.onreceived = function(event) {

Please create an utility function 'waitForManagerEvent' instead.  See http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/tests/marionette/head.js#154

@@ +169,5 @@
> +  manager.onreceived = function(event) {
> +    receivedMessage = event.message;
> +  };
> +
> +  SpecialPowers.setCharPref("dom.mms.retrieval_mode", "manual");

Use |SpecialPowers.pushPrefEnv(<inPrefs>, <callback>)| instead.

@@ +175,5 @@
> +  return Promise.resolve()
> +    .then(testNormalText)
> +    .then(testClass0Text)
> +    .then(testNormalBinary)
> +    .then(testClass0Binary)

.then(() => testText(PDU_DCS_NORMAL_UCS2, "normal"))
.then(() => testText(PDU_DCS_CLASS0_UCS2, "class-0"))
.then(() => testBinary(PDU_DCS_NORMAL_8BIT))
.then(() => testBinary(PDU_DCS_CLASS0_8BIT))

@@ +180,5 @@
> +    .then(function() {
> +      // Clear |manager.onreceived| handler.
> +      manager.onreceived = null;
> +      // Restore retrieval_mode.
> +      SpecialPowers.clearUserPref("dom.mms.retrieval_mode");

Don't need this if you use pushPrefEnv.
Attachment #8387395 - Flags: review?(vyang)
Comment on attachment 8387393 [details] [diff] [review]
Patch Part 1 v1: Save SMS PDU into DB to prevent data lost of the concatenated SMS when device reboot. r=vyang

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +1356,5 @@
> +    /**
> +     * This smsSegmentStore is used to store uncomplete SMS segments.
> +     * Each entry looks like this:
> +     *
> +     * { hash: <String> (primary key),

Will do

@@ +1357,5 @@
> +     * This smsSegmentStore is used to store uncomplete SMS segments.
> +     * Each entry looks like this:
> +     *
> +     * { hash: <String> (primary key),
> +     *   messageType: <Number>,

Will do

@@ +1361,5 @@
> +     *   messageType: <Number>,
> +     *   teleservice: <Number>,
> +     *   SMSC: <String>,
> +     *   sentTimestamp: <Number>,
> +     *   sender: <String>,

will do.

@@ +1365,5 @@
> +     *   sender: <String>,
> +     *   encoding: <Number>,
> +     *   messageClass: <String>,
> +     *   header: {
> +     *     segmentRef: <Number>,

Because the message.header and message.mwi will still be used by RadioInterface.handleSmsReceived(), this is to ensure that both Class 0 SMS concatenation handled in RadioInterface._processReceivedSmsSegment() and MobileMessageDB.jsm will provide same structure for further use in RadioInterface.handleSmsReceived().

@@ +1372,5 @@
> +     *     originatorPort: <Number>,
> +     *     destinationPort: <Number>,
> +     *   },
> +     *   mwi: {
> +     *     discard: <Boolean>,

ditto

@@ +1382,5 @@
> +     *   serviceCategory: <Number>,
> +     *   language: <String>,
> +     *
> +     *   // Handy fields for concatenation.
> +     *   segmentMaxSeq: <Number>,

Will do.

@@ +1385,5 @@
> +     *   // Handy fields for concatenation.
> +     *   segmentMaxSeq: <Number>,
> +     *   receivedSegments: <Number>,
> +     *   segments: [],
> +     *   timestamp: <Number> }

will do.

@@ +1388,5 @@
> +     *   segments: [],
> +     *   timestamp: <Number> }
> +     *
> +     */
> +    let smsSegmentStore = db.createObjectStore(SMS_SEGMENT_STORE_NAME,

will do

@@ +1389,5 @@
> +     *   timestamp: <Number> }
> +     *
> +     */
> +    let smsSegmentStore = db.createObjectStore(SMS_SEGMENT_STORE_NAME,
> +                                               { keyPath: "hash" });

will do.

@@ +2498,5 @@
> +          aSmsSegment.segments = [];
> +          aSmsSegment.timestamp = Date.now();
> +          if (aSmsSegment.encoding == RIL.PDU_DCS_MSG_CODING_8BITS_ALPHABET) {
> +            aSmsSegment.segments[seq] = aSmsSegment.data;
> +            delete aSmsSegment.data;

Just wonder if this will duplicate the storage in DB because we have already appended the data/body into the segments[] for further process.

@@ +2501,5 @@
> +            aSmsSegment.segments[seq] = aSmsSegment.data;
> +            delete aSmsSegment.data;
> +          } else {
> +            aSmsSegment.segments[seq] = aSmsSegment.body;
> +            delete aSmsSegment.body;

ditto

@@ +2506,5 @@
> +          }
> +
> +          let addRequest = segmentStore.add(aSmsSegment);
> +          addRequest.onsuccess = function(event) {
> +            aCallback.notify(Cr.NS_OK, null);

will do

@@ +2508,5 @@
> +          let addRequest = segmentStore.add(aSmsSegment);
> +          addRequest.onsuccess = function(event) {
> +            aCallback.notify(Cr.NS_OK, null);
> +          };
> +          addRequest.onerror = function(event) {

will do

@@ +2516,5 @@
> +              }
> +            }
> +            aCallback.notify(Cr.NS_ERROR_FAILURE, null);
> +          };
> +        } else {

Will do.

@@ +2539,5 @@
> +          // If the segments of a WAP Push are not received in sequence
> +          // (e.g., SMS with seq == 1 is not the 1st segment received by the device),
> +          // we have to retrieve the port information from 1st segment and
> +          // save it into the cached msgRecord.header.
> +          if (aSmsSegment.teleservice === RIL.PDU_CDMA_MSG_TELESERIVCIE_ID_WAP && seq === 1) {

will do.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2966,5 @@
> +   * Hash map for received multipart sms fragments. Messages are hashed with
> +   * its sender address and concatenation reference number. Three additional
> +   * attributes `segmentMaxSeq`, `receivedSegments`, `segments` are inserted.
> +   */
> +  _receivedSmsSegmentsMap: null,

The reason is that we still need what we did in ril_worker for the concatenation of Class 0 SMS which has to be handled irrespectively to the storage according 3GPP TS 23.040:
`When a mobile terminated message is class 0 and the MS has the capability of displaying short messages, the MS shall
display the message immediately and send an acknowledgement to the SC when the message has successfully reached
the MS irrespective of whether there is memory available in the (U)SIM or ME. The message shall not be automatically
stored in the (U)SIM or ME.`
Thanks for suggestion! I'll revise the test case again. :)

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
Separate new ObjectStore implementation to new patch.
Attachment #8387393 - Attachment is obsolete: true
Attachment #8389880 - Flags: review?(vyang)
Separate the logic of using sms-segment store for sms concatenation to new patch.
Attachment #8389882 - Flags: review?(vyang)
refine test case.
Attachment #8387395 - Attachment is obsolete: true
Attachment #8389885 - Flags: review?(vyang)
Comment on attachment 8389885 [details] [diff] [review]
Patch Part 3 v2: Add new test cases to test the concatenation of text/binary sms. r=vyang

cancel review for minor change.
Attachment #8389885 - Flags: review?(vyang)
Comment on attachment 8389882 [details] [diff] [review]
Patch Part 1.2 v2: Move SMS concatenation logic from ril_worker.js to RadioInterfaceLayer.js. r=vyang

cancel review for minor change.
Attachment #8389882 - Flags: review?(vyang)
Fix TestRILCodeQuality issue found in try server.
Attachment #8389882 - Attachment is obsolete: true
Attachment #8390218 - Flags: review?(vyang)
apply Promise.all() & waitForManagerEvent() for testing.
Attachment #8390220 - Attachment is obsolete: true
Attachment #8390220 - Flags: review?(vyang)
Attachment #8390224 - Flags: review?(vyang)
Comment on attachment 8389880 [details] [diff] [review]
Patch Part 1.1 v2: Create new ObjectStore for saving uncomplete SMS segments. r=vyang

cancel review because the the segment in db was not deleted after concatenation is complete.
Attachment #8389880 - Flags: review?(vyang)
Attachment #8390224 - Flags: review?(vyang) → review+
update patch to ensure that the segments of a complete sms is deleted correctly.
Attachment #8389880 - Attachment is obsolete: true
Attachment #8390261 - Flags: review?(vyang)
Comment on attachment 8390261 [details] [diff] [review]
Patch Part 1.1 v3: Create new ObjectStore for saving uncomplete SMS segments. r=vyang

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +2504,5 @@
> +        if (DEBUG) debug("Transaction " + txn + " completed.");
> +        aCallback.notify(Cr.NS_OK, completeMessage);
> +      };
> +
> +      txn.onerror = function onerror(event) {

use onabort instead.

@@ +2519,5 @@
> +        debug("Saving SMS Segment: " + aSmsSegment.hash + ", seq: " + seq);
> +      }
> +      let getRequest = segmentStore.index("hash").get(aSmsSegment.hash);
> +      getRequest.onsuccess = function(event) {
> +        let msgRecord = event.target.result;

nit: please rename to segmentRecord.  We use messageRecord, threadRecord, participantRecord to clarify what are we manipulating.

@@ +2532,5 @@
> +          } else {
> +            aSmsSegment.segments[seq] = aSmsSegment.body;
> +          }
> +
> +          let addRequest = segmentStore.add(aSmsSegment);

nit: |addRequest| is assigned but never referenced.

@@ +2533,5 @@
> +            aSmsSegment.segments[seq] = aSmsSegment.body;
> +          }
> +
> +          let addRequest = segmentStore.add(aSmsSegment);
> +        } else {

nit: bail-out early.

@@ +2559,5 @@
> +          // we have to retrieve the port information from 1st segment and
> +          // save it into the msgRecord.
> +          if (aSmsSegment.teleservice === RIL.PDU_CDMA_MSG_TELESERIVCIE_ID_WAP
> +              && seq === 1) {
> +            if (!msgRecord.originatorPort && aSmsSegment.originatorPort) {

nit: Since port information is only available in the 1st segment and we don't have duplicated 1st segment here, the |!msgRecord.originatorPort| check is certainly redundant.

@@ +2563,5 @@
> +            if (!msgRecord.originatorPort && aSmsSegment.originatorPort) {
> +              msgRecord.originatorPort = aSmsSegment.originatorPort;
> +            }
> +
> +            if (!msgRecord.destinationPort && aSmsSegment.destinationPort) {

ditto.

@@ +2570,5 @@
> +          }
> +
> +          if (msgRecord.receivedSegments < msgRecord.segmentMaxSeq) {
> +            if (DEBUG) debug("Message is incomplete.");
> +            let updateRequest = segmentStore.put(msgRecord);

nit: |updateRequest| is assigned but never referenced.

@@ +2574,5 @@
> +            let updateRequest = segmentStore.put(msgRecord);
> +            return;
> +          }
> +
> +          // Rebuild full body

Please move these lines into |txn.oncomplete|.  Don't block DB transaction as possible.

@@ +2597,5 @@
> +
> +          completeMessage = msgRecord;
> +
> +          // Delete Record in DB
> +          let deleteRequest = segmentStore.delete(msgRecord.id);

nit: |deleteRequest| is assigned but never referenced.
Attachment #8390261 - Flags: review?(vyang)
Attachment #8390306 - Flags: review?(vyang) → review+
update patch to refine sendAckSMS()
Attachment #8390218 - Attachment is obsolete: true
Attachment #8390218 - Flags: review?(vyang)
Attachment #8390455 - Flags: review?(vyang)
remove handy fields useful only for concatenation from completed message.
Attachment #8390306 - Attachment is obsolete: true
Attachment #8390629 - Flags: review?(vyang)
Update patch to:
1. refine handleSmsMultipart().
2. precisely purge unnecessary fields from completed message.
Attachment #8390455 - Attachment is obsolete: true
Attachment #8390455 - Flags: review?(vyang)
Attachment #8390631 - Flags: review?(vyang)
Comment on attachment 8390629 [details] [diff] [review]
Patch Part 1.1 v5: Create new ObjectStore for saving uncomplete SMS segments. r=vyang

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +2534,5 @@
> +      };
> +
> +      txn.onabort = function onerror(event) {
> +        if (DEBUG) debug("Caught error on transaction", event.target.errorCode);
> +        aCallback.notify(Cr.NS_ERROR_FAILURE, null, null);

The callback takes only two parameters.
Attachment #8390629 - Flags: review?(vyang) → review+
Comment on attachment 8390631 [details] [diff] [review]
Patch Part 1.2 v5: Move SMS concatenation logic from ril_worker.js to RadioInterfaceLayer.js. r=vyang

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

Thank you!  Looks we're almost there.  A few nits to be addressed.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3031,5 @@
> +
> +    let isMultipart = (segment.segmentMaxSeq && (segment.segmentMaxSeq > 1));
> +    let messageClass = segment.messageClass;
> +
> +    let handleReceivedAndAck = function (aRvOfIncompleteMsg, aCompleteMessage) {

nit: remove that SP between keyword 'function' and left parenthesis.

@@ +3034,5 @@
> +
> +    let handleReceivedAndAck = function (aRvOfIncompleteMsg, aCompleteMessage) {
> +      if (aCompleteMessage) {
> +        if (this.handleSmsReceived(
> +                   this._purgeCompleteSmsMessage(aCompleteMessage))) {

I'd rather have:

  this._purgeCompleteSmsMessage(aCompleteMessage);
  if (this.handleSmsReceived(aCompleteMessage)) {
    ...
  }

@@ +3045,5 @@
> +    }.bind(this);
> +
> +    // No need to access SmsSegmentStore for Class 0 SMS and Single SMS.
> +    if (!isMultipart ||
> +         (messageClass == RIL.GECKO_SMS_MESSAGE_CLASSES[RIL.PDU_DCS_MSG_CLASS_0])) {

nit: Alignment.

  if (!isMultipart ||
      (messageClass == ...
      ^
      | HERE

@@ +3197,5 @@
>  
>    /**
> +   * Handle ACK response of received SMS.
> +   */
> +  sendAckSms: function(aRv, aMessageClass) {

Please just pass |aMessage| to this function.

@@ +3198,5 @@
>    /**
> +   * Handle ACK response of received SMS.
> +   */
> +  sendAckSms: function(aRv, aMessageClass) {
> +      if (aMessageClass !== RIL.GECKO_SMS_MESSAGE_CLASSES[RIL.PDU_DCS_MSG_CLASS_2]) {

nit: bail-out early.

  if (aMessageClass === ...) {
    return;
  }

::: dom/system/gonk/ril_worker.js
@@ +8784,5 @@
>        replace:          false,
>        header:           message.header,
>        body:             message.body,
>        data:             message.data,
> +      sentTimestamp:    message[PDU_CDMA_MSG_USERDATA_TIMESTAMP],

nit: rename the same 'timestamp' attribute in 'GsmPDUHelper.readMessage' as well.
Attachment #8390631 - Flags: review?(vyang)
Comment on attachment 8392037 [details] [diff] [review]
Patch Part 1.2 v6: Move SMS concatenation logic from ril_worker.js to RadioInterfaceLayer.js. r=vyang

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

Thank you :)
Attachment #8392037 - Flags: review?(vyang) → review+
Blocks: b2g-sms
No longer depends on: 873351
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: