As suggested in the summary, we should check whether there are already a message retrieved/retrieving with the same transaction id before starting the retrieval. See bug 810067 as well.

Comment on attachment 728892 [details] [diff] [review] Patch v1.3 Review of attachment 728892 [details] [diff] [review]: ----------------------------------------------------------------- Need to review again when the following fix is included. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1267,5 @@ > if (aMessage.type == "sms") { > addresses = [aMessage.receiver]; > } else if (aMessage.type == "mms") { > addresses = aMessage.receivers; > + aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"]; This is not quite correct. We didn't prepare ["x-mms-transaction-id"] yet when calling .saveSendingMessage(). We should do something like below in the MmsService.js: let savableMessage = this.createSavableFromParams(aParams); let sendTransaction; try { sendTransaction = new SendTransaction(savableMessage); } catch (e) { aRequest.notifySendMmsMessageFailed(...); return; } gMobileMessageDatabaseService .saveSendingMessage(sendTransaction.msg, ...);

Comment on attachment 729363 [details] [diff] [review] Patch v1.6 Review of attachment 729363 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1040,5 @@ > * > * @param notification > * The parsed MMS message object. > */ > handleNotificationIndication: function handleNotificationIndication(notification) { Sorry. Bug 854422 lands first. You have to rebase again. ;P @@ +1049,5 @@ > + (function (aRv, aMessageRecord) { > + if (Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR === aRv > + && aMessageRecord) { > + debug("We already get the NotificationIndication = " > + + transactionId + " before."); debug("We already got the NotificationIndication with transactionId = " @@ +1301,5 @@ > Services.obs.notifyObservers(aDomMessage, kMmsSentObserverTopic, null); > }); > }; > > let savableMessage = this.createSavableFromParams(aParams); s/createSavableFromParams/createSendableFromParams s/savableMessage/sendableMessage and move the following lines message["type"] = "mms"; message["deliveryStatusRequested"] = true; message["timestamp"] = Date.now(); message["receivers"] = receivers; to another new function .ceateSavableFromSendable(aSendable). @@ +1304,5 @@ > > let savableMessage = this.createSavableFromParams(aParams); > + let sendTransaction; > + try { > + sendTransaction = new SendTransaction(savableMessage); s/savableMessage/sendableMessage @@ +1307,5 @@ > + try { > + sendTransaction = new SendTransaction(savableMessage); > + } catch (e) { > + debug("Exception: fail to create a SendTransaction instance."); > + sendTransactionCb(aDomMessage.id, false); We cannot call this because .sendTransactionCb() calls .setMessageDelivery() internally, which cannot find a saved record. I think we should call directly call: aRequest.notifySendMmsMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR); @@ +1309,5 @@ > + } catch (e) { > + debug("Exception: fail to create a SendTransaction instance."); > + sendTransactionCb(aDomMessage.id, false); > + return; > + } Call: let savableMessage = this.ceateSavableFromSendable(sendTransaction.msg); or let savableMessage = this.ceateSavableFromSendable(sendableMessage); @@ +1310,5 @@ > + debug("Exception: fail to create a SendTransaction instance."); > + sendTransactionCb(aDomMessage.id, false); > + return; > + } > + gMobileMessageDatabaseService.saveSendingMessage(sendTransaction.msg, s/sendTransaction.msg/savableMessage ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl @@ +83,5 @@ > void getMessageRecordById(in long aMessageId, > in nsIRilMobileMessageDatabaseRecordCallback aCallback); > + > + /** > + * |aTransactionId| DOMString: the transaction id of mms pdu. s/mms/MMS ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1176,5 @@ > // TODO Bug 853384 - for some SIMs we cannot retrieve the vaild > // phone number, thus setting the SMS' receiver to be null. > aMessage.receiver = self; > } else if (aMessage.type == "mms") { > + aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"]; I'd prefer adding the following assignment in the .convertIntermediateToSavable(...): intermediate.transactionId = intermediate.headers["x-mms-transaction-id"]; And add a sanity check to ensure the existence of |aMessage.transactionId| at the head of .saveReceivedMessage(). Also, update the comment in the .idl. Also, I'd prefer moving this assignment to further below: aMessage.deliveryIndex = [DELIVERY_RECEIVED, timestamp]; aMessage.readIndex = [FILTER_READ_UNREAD, timestamp]; if (aMessage.type == "mms") { aMessage.transactionIdIndex = aMessage.transactionId; } where deal with all the index assignment stuff. @@ +1267,5 @@ > if (aMessage.type == "sms") { > addresses = [aMessage.receiver]; > } else if (aMessage.type == "mms") { > addresses = aMessage.receivers; > + aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"]; I'd prefer adding the following assignment in the .ceateSavableFromSendable(aSavable): message["transactionId"] = aSavable.headers["x-mms-transaction-id"]; Add a sanity check for |aMessage.transactionId| at the head of .saveSendingmessage(). Also, update the comment in the .idl. Also move this assignment to: // Adding needed indexes and extra attributes for internal use. // threadIdIndex & participantIdsIndex are filled in saveRecord(). ... if (aMessage.type == "mms") { aMessage.transactionIdIndex = aMessage.transactionId; } @@ +1374,5 @@ > messageStore.put(messageRecord); > }; > }); > }, > + getMessageRecordByTransactionId: function getMessageRecordByTransactionId(aTransactionId, aCallback) { New line above this function.

Revised accroding to Gene's comment.

Comment on attachment 730034 [details] [diff] [review] Patch v1.8 Review of attachment 730034 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1180,5 @@ > }, > > /** > * A utility function to convert the MmsParameters dictionary object > + * to a sendable message. sendable intermediate object. @@ +1267,5 @@ > + }, > + > + /** > + * A utility function to convert the sendable intermediate object > + * to a sendable intermediate object. s/sendable/savable @@ +1277,2 @@ > // The following attributes are needed for saving message into DB. > + let saveable = aSendable; s/saveable/savable and below. ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl @@ +39,5 @@ > * - If |type| == "mms", we also need: > * - |delivery| DOMString: the delivery state of received message > * - |deliveryStatus| DOMString Array: the delivery status of received message > * - |receivers| DOMString Array: the phone numbers of receivers > + * - |transactionId| DOMString: the transaction id from pdu header. the transaction ID from MMS PDU header. @@ +58,5 @@ > * - |receiver| DOMString: the phone number of receiver > * > * - If |type| == "mms", we also need: > * - |receivers| DOMString Array: the phone numbers of receivers > + * - |transactionId| DOMString: the transaction id from pdu header. Ditto. @@ +85,5 @@ > void getMessageRecordById(in long aMessageId, > in nsIRilMobileMessageDatabaseRecordCallback aCallback); > + > + /** > + * |aTransactionId| DOMString: the transaction id of MMS pdu. the transaction ID of MMS PDU. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +562,5 @@ > }); > }; > }, > > + // Add index of transaction id for MMS. NIT: please use the following format for a function comment. /** * Add index of transaction id for MMS. */ @@ +1162,5 @@ > (aMessage.type == "sms" && aMessage.messageClass == undefined) || > (aMessage.type == "mms" && (aMessage.delivery == undefined || > !Array.isArray(aMessage.deliveryStatus) || > !Array.isArray(aMessage.receivers))) || > + (aMessage.type == "mms" && aMessage.transactionId == undefined) || Please combine this condition with the above one. @@ +1220,5 @@ > saveSendingMessage: function saveSendingMessage(aMessage, aCallback) { > if ((aMessage.type != "sms" && aMessage.type != "mms") || > (aMessage.type == "sms" && !aMessage.receiver) || > (aMessage.type == "mms" && !Array.isArray(aMessage.receivers)) || > + (aMessage.type == "mms" && aMessage.transactionId == undefined) || Ditto. @@ +1270,5 @@ > if (aMessage.type == "sms") { > addresses = [aMessage.receiver]; > } else if (aMessage.type == "mms") { > addresses = aMessage.receivers; > + aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"]; aMessage.transactionIdIndex = aMessage.transactionId; @@ +1403,5 @@ > + request.onerror = function onerror(event) { > + if (DEBUG) { > + if (event.target) > + debug("Caught error on transaction", event.target.errorCode); > + aCallback.notify(Ci.nsIMobileMessageCallback.INTERNAL_ERROR, null); Why is the line in the |if (DEBUG) {}| block?

Btw, we still need Vicamo's final review since I'm not a formal RIL reviewer.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #14) > Created attachment 730574 [details] [diff] [review] > Patch v1.9 Modified according to Gene's comment.

(In reply to Gene Lian [:gene] from comment #12) > Comment on attachment 730034 [details] [diff] [review] > Patch v1.8 > > Review of attachment 730034 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/mms/src/ril/MmsService.js > @@ +1180,5 @@ > > }, > > > > /** > > * A utility function to convert the MmsParameters dictionary object > > + * to a sendable message. > > sendable intermediate object. > > @@ +1267,5 @@ > > + }, > > + > > + /** > > + * A utility function to convert the sendable intermediate object > > + * to a sendable intermediate object. > > s/sendable/savable > > @@ +1277,2 @@ > > // The following attributes are needed for saving message into DB. > > + let saveable = aSendable; > > s/saveable/savable and below. > > ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl > @@ +39,5 @@ > > * - If |type| == "mms", we also need: > > * - |delivery| DOMString: the delivery state of received message > > * - |deliveryStatus| DOMString Array: the delivery status of received message > > * - |receivers| DOMString Array: the phone numbers of receivers > > + * - |transactionId| DOMString: the transaction id from pdu header. > > the transaction ID from MMS PDU header. > > @@ +58,5 @@ > > * - |receiver| DOMString: the phone number of receiver > > * > > * - If |type| == "mms", we also need: > > * - |receivers| DOMString Array: the phone numbers of receivers > > + * - |transactionId| DOMString: the transaction id from pdu header. > > Ditto. > > @@ +85,5 @@ > > void getMessageRecordById(in long aMessageId, > > in nsIRilMobileMessageDatabaseRecordCallback aCallback); > > + > > + /** > > + * |aTransactionId| DOMString: the transaction id of MMS pdu. > > the transaction ID of MMS PDU. > > ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js > @@ +562,5 @@ > > }); > > }; > > }, > > > > + // Add index of transaction id for MMS. > > NIT: please use the following format for a function comment. > > /** > * Add index of transaction id for MMS. > */ > > @@ +1162,5 @@ > > (aMessage.type == "sms" && aMessage.messageClass == undefined) || > > (aMessage.type == "mms" && (aMessage.delivery == undefined || > > !Array.isArray(aMessage.deliveryStatus) || > > !Array.isArray(aMessage.receivers))) || > > + (aMessage.type == "mms" && aMessage.transactionId == undefined) || > > Please combine this condition with the above one. > > @@ +1220,5 @@ > > saveSendingMessage: function saveSendingMessage(aMessage, aCallback) { > > if ((aMessage.type != "sms" && aMessage.type != "mms") || > > (aMessage.type == "sms" && !aMessage.receiver) || > > (aMessage.type == "mms" && !Array.isArray(aMessage.receivers)) || > > + (aMessage.type == "mms" && aMessage.transactionId == undefined) || > > Ditto. > > @@ +1270,5 @@ > > if (aMessage.type == "sms") { > > addresses = [aMessage.receiver]; > > } else if (aMessage.type == "mms") { > > addresses = aMessage.receivers; > > + aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"]; > > aMessage.transactionIdIndex = aMessage.transactionId; > > @@ +1403,5 @@ > > + request.onerror = function onerror(event) { > > + if (DEBUG) { > > + if (event.target) > > + debug("Caught error on transaction", event.target.errorCode); > > + aCallback.notify(Ci.nsIMobileMessageCallback.INTERNAL_ERROR, null); > > Why is the line in the |if (DEBUG) {}| block? Thanks. :D

This is mms conformance feature. Nominate for leo+.

Comment on attachment 730574 [details] [diff] [review] Patch v1.9 Review of attachment 730574 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1047,4 @@ > > let transactionId = notification.headers["x-mms-transaction-id"]; > + gMobileMessageDatabaseService.getMessageRecordByTransactionId(transactionId, > + (function (aRv, aMessageRecord) { Please tear down this long long function into small pieces. It's getting difficult to read, let alone review it :( @@ +1272,5 @@ > + * > + * @param aSendable > + * The sendable intermediate object. > + */ > + ceateSavableFromSendable : function ceateSavableFromSendable(aSendable) { nit: c'r'eateSavableFromSendable @@ +1283,5 @@ > + let receivers = savable["receivers"] = []; > + for (let idx in headersTo) { > + receivers.push(headersTo[idx].address); > + } > + savable["transactionId"] = aSendable.headers["x-mms-transaction-id"]; Transaction id is mandatory for M-Send.req, but not really meaningful in this bug. ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl @@ +58,5 @@ > * - |receiver| DOMString: the phone number of receiver > * > * - If |type| == "mms", we also need: > * - |receivers| DOMString Array: the phone numbers of receivers > + * - |transactionId| DOMString: the transaction ID from MMS pdu header. Ditto ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +200,5 @@ > if (DEBUG) debug("Upgrade to version 8. Add participant/thread stores."); > self.upgradeSchema7(db, event.target.transaction); > break; > + case 8: > + if (DEBUG) debug("Upgrade to version 9. Add index of transaction id for MMS."); for incoming MMS. @@ +589,5 @@ > + cursor.continue(); > + } > + messageRecord.transactionIdIndex = messageRecord.transactionId; > + cursor.update(messageRecord); > + cursor.continue(); What about: if (is incoming mms) { update transaction index. } continue(); Because you missed a return statement after line 589 and transaction id is only meaningful when it's an incoming MMS. @@ +1222,5 @@ > saveSendingMessage: function saveSendingMessage(aMessage, aCallback) { > if ((aMessage.type != "sms" && aMessage.type != "mms") || > (aMessage.type == "sms" && !aMessage.receiver) || > + (aMessage.type == "mms" && (!Array.isArray(aMessage.receivers) || > + aMessage.transactionId == undefined)) || Ditto. @@ +1272,5 @@ > if (aMessage.type == "sms") { > addresses = [aMessage.receiver]; > } else if (aMessage.type == "mms") { > addresses = aMessage.receivers; > + aMessage.transactionIdIndex = aMessage.transactionId; Ditto. @@ +1390,5 @@ > + return; > + } > + let request = messageStore.index("transactionId").get(aTransactionId); > + > + request.onsuccess = function onsuccess(event) { txn.oncomplete to shorten database transaction period. @@ +1401,5 @@ > + } > + aCallback.notify(Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR, messageRecord); > + }; > + > + request.onerror = function onerror(event) { txn.onerror

Mandatory MMS conformance bug, blocking.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #22) > Created attachment 735658 [details] [diff] [review] > Patch v1.12 Modifiied according to comment 18.

Comment on attachment 735658 [details] [diff] [review] Patch v1.12 Review of attachment 735658 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1093,5 @@ > + reportAllowed); > + transaction.run(); > + > + if (!success) { > + // At this point we could send a message to content to nit: align to 80 char edge. @@ +1106,5 @@ > + // Broadcasting an 'sms-received' system message to open apps. > + this.broadcastMmsSystemMessage("sms-received", domMessage); > + > + // Notifying observers an MMS message is received. > + Services.obs.notifyObservers(domMessage, kMmsReceivedObserverTopic, null); Please extract these four lines into another utility function. @@ +1167,5 @@ > + > + // For RETRIEVAL_MODE_AUTOMATIC or RETRIEVAL_MODE_AUTOMATIC_HOME but not > + // roaming, proceed to retrieve MMS. > + this.retrieveMessage(url, this.retrieveMessageCallback.bind(this, wish, savableMessage)); > + }, Empty line, please. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +588,5 @@ > + let messageRecord = cursor.value; > + if ("mms" == messageRecord.type && > + (DELIVERY_NOT_DOWNLOADED == messageRecord.delivery || > + DELIVERY_RECEIVED == messageRecord.delivery)) { > + messageRecord.transactionIdIndex = messageRecord.transactionId; At the time we land this patch, every mms record has no |transactionId| field, doesn't it? You have to fetch it from |messageRecord.headers["x-mms-transaction-id"]|. @@ +1047,5 @@ > aMessage.deliveryIndex = [DELIVERY_SENDING, timestamp]; > aMessage.readIndex = [FILTER_READ_READ, timestamp]; > aMessage.delivery = DELIVERY_SENDING; > aMessage.messageClass = MESSAGE_CLASS_NORMAL; > aMessage.read = FILTER_READ_READ; Please keep this empty line.

Revised the patch for comment 24.

Changed this to self in |send|.

Comment on attachment 736100 [details] [diff] [review] Patch v1.14 Review of attachment 736100 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1057,5 @@ > + * The MMS observer topic. > + * @params aDomMessage > + * The nsIDOMMozMmsMessage object. > + */ > + broadcastSysMagAndNotifyObservers: function broadcastSysMagAndNotifyObservers(aName, aObserverTopic, aDomMessage) { Please have two functions broadcast{Sent,Received}MessageEvent() that takes one parameter |aDomMessage| only. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +201,5 @@ > if (DEBUG) debug("Upgrade to version 8. Add participant/thread stores."); > self.upgradeSchema7(db, event.target.transaction); > break; > + case 8: > + if (DEBUG) debug("Upgrade to version 9. Add index of transaction id for incoming MMS."); nit: Add transactionId index for ... @@ +564,5 @@ > }; > }, > > + /** > + * Add index of transaction id for MMS. ditto. @@ +588,5 @@ > + let messageRecord = cursor.value; > + if ("mms" == messageRecord.type && > + (DELIVERY_NOT_DOWNLOADED == messageRecord.delivery || > + DELIVERY_RECEIVED == messageRecord.delivery)) { > + messageRecord.transactionIdIndex = messageRecord.headers["x-mms-transaction-id"]; Line wrap

Rebased and revised.

Clean space and some mistakes.

Uplift WIP: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/838467/b2g18 , verifying.

Comment on attachment 736707 [details] [diff] [review] Patch v1.16 Review of attachment 736707 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1351,5 @@ > Services.obs.notifyObservers(aDomMessage, kSmsFailedObserverTopic, null); > return; > } > > + self.broadcastSentMessageEvent(domMessage); s/domMessage/aDomMessage I'm really surprised. This is impossible to work... How could we send an MMS with this typo? No one has been pointing this out since it was checked in on 4/17... Will fire a bug for this later.