B2G MMS: MMSCONF-GEN-C-003: Support for maximum values for MMS parameters

RESOLVED FIXED in Firefox 22

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: vicamo, Assigned: ctai)

Tracking

unspecified
mozilla22
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

6 years ago
See OMA-TS-MMS-CONF-V1_3-20110913-A section 10.2.5, and Appendix C.1.1
See OMA-ETS-MMS_CON-V1_3-20101015-C section 5.1.1.1.10, 5.2.2.2.1, 5.2.2.2.3, 5.2.2.2.4, 5.2.2.3.1, 5.2.2.3.2, 5.4.3.9
(Reporter)

Updated

6 years ago
Summary: B2G MMS: [MMSCONF-GEN-C-003] Support for maximum values for MMS parameters → B2G MMS: MMSCONF-GEN-C-003: Support for maximum values for MMS parameters
(Reporter)

Comment 1

6 years ago
Maximum values of MMS parameters: (MMS-CONF 10.2.5)

1) Message ID - 40 characters
2) X-Mms-Transaction-Id - 40
3) X-Mms-Content-Location - 100
4) MMSC URL Length - 50
5) Subject - 40 (in M-Notification.ind)
6) X-Mms-Response-Text - 30
7) To, Cc and Bcc - 312
8) Name parameter for Content-Type in WSP multipart headers - 40
9) X-Mms-Retrieve-Text - 30
10) X-Mms-Store-Status-Text - 30
(Reporter)

Updated

6 years ago
Blocks: 809832
(Reporter)

Updated

6 years ago
Blocks: 804679
Assignee: nobody → ctai
Status: NEW → ASSIGNED
We should check the fields(X-Mms-Transaction-Id, Subject,  To, Cc and Bcc, Name parameter for Content-Type in WSP multipart headers) before sending MMS.
And We don't need to check the fields while receiving MMS. Other fields are generated by operators.
Because Message ID is UUID type, a string of 32 hexadecimal digits, we don't need to check the Message ID. It should not be able exceed 40 characters.
example:
'{12345678-1234-5678-1234-567812345678}',
'12345678123456781234567812345678'
Message ID is generated by UUID generator in FFOS now.
Created attachment 689137 [details] [diff] [review]
CheckMMSParameters-v1
Created attachment 689147 [details] [diff] [review]
CheckMMSParameters-v1.1
Attachment #689137 - Attachment is obsolete: true
Attachment #689147 - Flags: feedback?(vyang)
(Reporter)

Comment 7

6 years ago
Comment on attachment 689147 [details] [diff] [review]
CheckMMSParameters-v1.1

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

Can we postpone works here? I think we can have this implemented inside some sendMMS() function. We'll have to implement some kind of retry scheme in the future, and this patch will check every time we're going to send a PDU out and will certainly check the same PDU for several times in retry process.

::: dom/mms/src/ril/MmsService.js
@@ +595,5 @@
>    },
>  
> +  /**
> +   * Check length of recipients(to, cc, bcc).
> +   * 

trailing white space

@@ +608,5 @@
> +      if (recipients[ix].address.length > MMS.MMS_MAX_LENGTH_RECIPIENT) {
> +        return false;
> +      }
> +      if (recipients[ix].type === "email") {
> +        let found = recipients[ix].address.indexOf("<");

What if "not found"?

@@ +620,5 @@
> +  },
> +
> +  /**
> +   * Check maximum values of MMS parameters.
> +   * 

trailing white space
Attachment #689147 - Flags: feedback?(vyang)
(Reporter)

Updated

6 years ago
No longer blocks: 804679
leo+, this bug is needed to fulfill MMS user stories for v1.1
blocking-b2g: --- → leo+
no dependency with gaia
target to finish between 3/11~3/15
Whiteboard: [
Whiteboard: [ → [3/11~3/15]
Created attachment 723046 [details] [diff] [review]
CheckMMSParameters-v2
Attachment #689147 - Attachment is obsolete: true
Created attachment 723047 [details] [diff] [review]
CheckMMSParameters-v2
Attachment #723046 - Attachment is obsolete: true
Created attachment 723335 [details] [diff] [review]
CheckMMSParameters-v2
Attachment #723047 - Attachment is obsolete: true
Attachment #723335 - Flags: feedback?(vyang)
(Reporter)

Comment 13

5 years ago
Comment on attachment 723335 [details] [diff] [review]
CheckMMSParameters-v2

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

::: dom/mms/src/ril/MmsService.js
@@ +429,5 @@
> +     * @return true if the lengths are less than the maximum values of
> +     *         recipient.
> +     * @see OMA-TS-MMS_CONF-V1_3-20110511-C section 10.2.5
> +     */
> +    checkRecipientsLength: function checkRecipientsLength(recipients) {

How about throwing an exception on error, returning 0 or positive numbers for number of recipients?

@@ +463,5 @@
> +
> +      let totalRecipients = 0;
> +      let to = msg.headers["to"];
> +      if (Array.isArray(to)) {
> +        totalRecipients += to.length;

Then we can have: 

  let totalRecipients = 0;
  try {
    totalRecipients += this.checkRecipientsLength(msg.headers["to"]);
    totalRecipients += this.checkRecipientsLength(msg.headers["cc"]);
    totalRecipients += this.checkRecipientsLength(msg.headers["bcc"]);
  } catch (ex) {
    return false;
  }

@@ +501,5 @@
> +      }
> +
> +      if (Array.isArray(msg.parts)) {
> +        for (let i = 0; i < msg.parts.length; i++) {
> +          if(msg.parts[i].headers["content-type"] && 

sp between |if| and left parenthesis, and trailing ws.

@@ +659,5 @@
>  
> +  if (!gMmsTransactionHelper.checkMaxValuesParameters(msg)) {
> +    //We should notify end user that the header format is wrong.
> +    debug("Check max values parameters fail.");
> +    this.parametersCheckFail = true;

Just throw an error and catch in appropriate place.

::: dom/mms/tests/test_mms_service.js
@@ +3,5 @@
> +
> +let MMS_Service = {};
> +subscriptLoader.loadSubScript("resource://gre/components/MmsService.js", MMS_Service);
> +MMS_Service.debug = do_print;
> +let CallFunc = MMS_Service.gMmsTransactionHelper;

Please have a newMmsTransactionHelper() instead. We have similar thing for RIL Worker.

  function newMmsTransactionHelper() {
    let ns = {};
    subscriptLoader.loadSubScript("...", ns);
    ns.debug = do_print;
    return ns.gMmsTransactionHelper;
  }

@@ +115,5 @@
> +  let msg = {};
> +  msg.headers = {};
> +  msg.headers["to"] = [
> +                       { address: "Joe User <joe@user.org>", type: "email" },
> +                     ];

Indentation.
Attachment #723335 - Flags: feedback?(vyang)
Created attachment 723836 [details] [diff] [review]
CheckMMSParameters-v2.1
Attachment #723335 - Attachment is obsolete: true
Created attachment 723861 [details] [diff] [review]
CheckMMSParameters-v2.2
Attachment #723836 - Attachment is obsolete: true
Attachment #723861 - Flags: review?(vyang)
(Reporter)

Comment 17

5 years ago
Comment on attachment 723861 [details] [diff] [review]
CheckMMSParameters-v2.2

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

::: dom/mms/src/ril/MmsService.js
@@ +433,5 @@
> +      if (recipients && recipients.address) {
> +        return 1;
> +      }
> +      let totalRecipients = 0;
> +      if (Array.isArray(recipients)) {

Bail out early.

@@ +477,5 @@
> +        return false;
> +      }
> +
> +      if (totalRecipients < 1 || totalRecipients >
> +        MMS.MMS_MAX_TOTAL_RECIPIENTS) {

if (totalRecipients < 1 ||
    totalRecipients > MMS.MMS_MAX_TOTAL_RECIPIENTS) {

@@ +481,5 @@
> +        MMS.MMS_MAX_TOTAL_RECIPIENTS) {
> +        return false;
> +      }
> +
> +      if (Array.isArray(msg.parts)) {

ditto.
Attachment #723861 - Flags: review?(vyang) → review+
Created attachment 723866 [details] [diff] [review]
CheckMMSParameters-v2.3
Attachment #723861 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1907546aa8e7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2a8c49f56bb6

(pushed with on top of bustage-free)
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox22: --- → fixed
Whiteboard: [3/11~3/15] → [NO_UPLIFT]
Added [NO_UPLIFT] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out.

------------------------------
If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible:

Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643)
Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages
Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS
Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments.
Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741).
Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS
Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix)
Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix)
Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event
Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead().
Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete().
Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage(). 
Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
Bug 845643 - B2G MMS: Save retrieved MMS into database.
Bug 792321 - Check max values of MMS parameters in sendRequest.
Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS
Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS
Bug 839436 - B2G MMS: make DB be able to save MMS messages.
Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.
Per off-line discussion with Michael, we decided not to back out the MMS bugs that have already been in mozilla-b2g18. Removing [NO_UPLIFT] to make the check-in status sync'ed.
Whiteboard: [NO_UPLIFT]
(Reporter)

Updated

5 years ago
Blocks: 840063

Updated

5 years ago
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.