Closed Bug 907140 Opened 12 years ago Closed 12 years ago

[B2G][MMS] Block sending email from MMS until we supported this feature.

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26
Project Flags:
blocking-b2g leo+
Tracking Flags:
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: leo.bugzilla.gaia, Assigned: ctai)

References

Details

Attachments

(2 files, 9 obsolete files)

leo-bug-907140.patch v2.0
5.11 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
b2g18-leo-bug-907140.patch
5.10 KB, patch
Details | Diff | Splinter Review
MMS message:When sending an MMS to an email address,the message is sent but the message will never be received at the receiving side. From the logs it is shown that the message email address is normalized to phone number. SMS message:When sending an SMS to the same email address,the email id is converted to number which is shown in th UI itself and the message will not be sent. Issue:In case of MMS it is wrong information to the user, that it is showing MMS sent to an email address. Please clarify on this.
Please find the logs which gives the information: I/Gecko ( 142): -@- MmsService: send: aParams: {} I/Gecko ( 142): -@- MmsService: createSavableFromParams: aParams: {} I/Gecko ( 142): -@- MmsService: createSavableFromParams: normalize phone number from sasikala.paruchuri@gmail.com to 7274525272782487446245266 I/Gecko ( 142): -@- MmsService: createSavableFromParams: message: {"headers":{"to":[{"address":"7274525272782487446245266","type":"PLMN"}]},"parts":[{"headers":{"content-type":{"media":"application/smil","params":{"name":"smil.xml","charset":{"charset":"utf-8"}}},"content-location":"smil.xml","content-id":"<smil>"},"content":"<smil><head><layout><root-layout width=\"320px\" height=\"480px\"/><region id=\"Image\" left=\"0px\" top=\"0px\" width=\"320px\" height=\"320px\" fit=\"meet\"/><region id=\"Text\" left=\"0px\" top=\"320px\" width=\"320px\" height=\"160px\" fit=\"meet\"/></layout></head><body><par dur=\"5000ms\"><img src=\"ee.jpg\" region=\"Image\"/></par></body></smil>"},{"headers":{"content-type":{"media":"image/jpeg","params":{"name":"ee.jpg"}},"content-location":"ee.jpg","content-id":"<ee.jpg>"},"content":{}}],"type":"mms","deliveryStatusRequested":true,"timestamp":1376998550143,"receivers":["sasikala.paruchuri@gmail.com"]} I/Gecko ( 142): -@- MmsService: Saving sending message is done. Start to send. I/Gecko ( 142): -@- MmsService: updateProxyInfo: {"host":"10.10.1.100","port":9401,"type":"http","flags":1,"resolveFlags":0,"failoverTimeout":1800,"failoverProxy":null,"TRANSPARENT_PROXY_RESOLVES_HOST":1} I/Gecko ( 142): -@- MmsService: msg: {"headers":{"to":[{"address":"7274525272782487446245266","type":"PLMN"}],"x-mms-message-type":128,"x-mms-transaction-id":"{cebf991c-1df4-4bf3-a3b9-691af9f1dfca}","x-mms-mms-version":19,"from":null,"date":"2013-08-20T11:35:50.369Z","x-mms-message-class":"personal","x-mms-expiry":604800,"x-mms-priority":129,"x-mms-read-report":true,"x-mms-delivery-report":true,"content-type":{"params":{"type":"application/smil","start":"<smil>"},"media":"application/vnd.wap.multipart.related"}},"parts":[{"headers":{"content-type":{"media":"application/smil","params":{"name":"smil.xml","charset":{"charset":"utf-8"}}},"content-location":"smil.xml","content-id":"<smil>"},"content":"<smil><head><layout><root-layout width=\"320px\" height=\"480px\"/><region id=\"Image\" left=\"0px\" top=\"0px\" width=\"320px\" height=\"320px\" fit=\"meet\"/><region id=\"Text\" left=\"0px\" top=\"320px\" width=\"320px\" height=\"160px\" fit=\"meet\"/></layout></head><body><par dur=\"5000ms\"><img src=\"ee.jpg\" region=\"Image\"/ I/Gecko ( 142): -@- MmsService: All parts loaded: [{"headers":{"content-type":{"media":"application/smil","params":{"name":"smil.xml","charset":{"charset":"utf-8"}}},"content-location":"smil.xml","content-id":"<smil>"},"content":"<smil><head><layout><root-layout width=\"320px\" height=\"480px\"/><region id=\"Image\" left=\"0px\" top=\"0px\" width=\"320px\" height=\"320px\" fit=\"meet\"/><region id=\"Text\" left=\"0px\" top=\"320px\" width=\"320px\" height=\"160px\" fit=\"meet\"/></layout></head><body><par dur=\"5000ms\"><img src=\"ee.jpg\" region=\"Image\"/></par></body></smil>"},{"headers":{"content-type":{"media":"image/jpeg","params":{"name":"ee.jpg"}},"content-location":"ee.jpg","content-id":"<ee.jpg>"},"content":{"0":255,"1":216,"2":255,"3":224,"4":0,"5":16,"6":74,"7":70,"8":73,"9":70,"10":0,"11":1,"12":1,"13":1,"14":0,"15":72,"16":0,"17":72,"18":0,"19":0,"20":255,"21":219,"22":0,"23":67,"24":0,"25":3,"26":2,"27":2,"28":3,"29":2,"30":2,"31":3,"32":3,"33":3,"34":3,"35":4,"36":3,"37":3,"38":4,"39":5,"40":8,"41":5,"42":5, I/Gecko ( 142): -@- MmsService: acquire: buffer the MMS request and setup the MMS data call.
blocking-b2g: --- → leo+
Flags: needinfo?(schung)
:ctai - Do you know anything about this? Why is it showing as success?
Flags: needinfo?(ctai)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Corey, we do not support MMS to email on v1.1, would it make sense to block out inputting symbols (especially "@" symbol) in the recipient? We do however support alphanumeric input i believe, so we cannot simply block all non-numeric input.
Flags: needinfo?(gnarf37)
Blocking "@" symbol input is doable, but having these symbol is valid for contact name and user should be able to type/search contact name in recipient field. It seems not an ideal way to just block certain symbol input especially we don't have any warning for that. The first thing need to be verified is why the message sent to an email address will return success here. Although we might not able to add new string for warning dialog about "email address is not supported yet...", it still better than wrong sending status.
Flags: needinfo?(schung)
Looks like |PhoneNumberUtils.normalize| can convert from email to phone number. So it will show success.
Flags: needinfo?(ctai)
We'll solve bug 907164 instead, marking messages with email addresses as invalid.
Assignee: nobody → ctai
Status: RESOLVED → REOPENED
Component: Gaia::SMS → DOM: Device Interfaces
Flags: needinfo?(gnarf37)
Product: Boot2Gecko → Core
Resolution: DUPLICATE → ---
Summary: [MMS] Sending MMS to an email address is shown as success even though the email id converted as a numbet → [B2G][MMS] Block sending email from MMS until we supported this feature.
We will block sending to email via MMS in gecko part until we support it.
Attached patch bug-907140.patch v1.0 (obsolete) — — Splinter Review
Attached patch bug-907140.patch v1.1 (obsolete) — — Splinter Review
Attachment #793893 - Attachment is obsolete: true
Attached patch bug-907140.patch v1.2 (obsolete) — — Splinter Review
Attachment #793896 - Attachment is obsolete: true
Attachment #793900 - Attachment description: bug-907140.patch → bug-907140.patch v1.2
Attachment #793900 - Flags: feedback?(vyang)
Comment on attachment 793900 [details] [diff] [review] bug-907140.patch v1.2 Review of attachment 793900 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MmsPduHelper.jsm @@ +174,5 @@ > + * Address string which want to find the type. > + * > + * @return Address type. > + */ > + findType: function findType(address) { nit: resolveType @@ +178,5 @@ > + findType: function findType(address) { > + let type = null; > + if (address.match(this.REGEXP_TEST_EMAIL)) { > + type = "email"; > + return type; just |return "email";| @@ +216,5 @@ > defineLazyRegExp(Address, "REGEXP_ALPHANUM", "^\\w+$"); > +defineLazyRegExp(Address, "REGEXP_TEST_PLMN", "^\\+?[\\d.-]+$"); > +defineLazyRegExp(Address, "REGEXP_TEST_IPV4", "^\\d{1,3}(?:\\.\\d{1,3}){3}$"); > +defineLazyRegExp(Address, "REGEXP_TEST_IPV6", "^[\\da-fA-F]{4}(?::[\\da-fA-F]{4}){7}$"); > +defineLazyRegExp(Address, "REGEXP_TEST_EMAIL", "@"); Since we have already REGEXP_NUM and REGEXP_ALPHANUM, please also follow the naming and name them as REGEXP_PLMN, ... ::: dom/mobilemessage/src/gonk/MmsService.js @@ +1267,5 @@ > if (intermediate.headers[type]) { > if (intermediate.headers[type] instanceof Array) { > for (let index in intermediate.headers[type]) { > + savable.receivers.push({"address": intermediate.headers[type][index].address, > + "type": intermediate.headers[type][index].type}); let addressFields = intermediate.headers[type]; if (addressFields instanceof Array) { for (let field of addressFields) { ... } } @@ +1272,4 @@ > } > } else { > + savable.receivers.push({"address": intermediate.headers[type].address, > + "type": intermediate.headers[type].type}); ditto. @@ +1646,5 @@ > + let addressType = MMS.Address.findType(receivers[i]); > + let normalizedAddress = receivers[i]; > + if (addressType == "PLMN") { > + normalizedAddress = PhoneNumberUtils.normalize(receivers[i], false); > + } let receiver = receivers[i]; let type = MMS.Address.resolveType(receiver); let addres; if (type == "PLMN") { address = PhoneNumberUtils.normalize(receiver); isAddrValid = !PhoneNumberUtils.isPlainPhoneNumber(address); if (DEBUG) { ... } } else { address = receiver; isAddrValid = false; if (DEBUG) { ... } } headersTo.push({...}); receivers[i] = {...} ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +810,5 @@ > + */ > + upgradeSchema12: function upgradeSchema12(transaction, next) { > + let messageStore = transaction.objectStore(MESSAGE_STORE_NAME); > + > + // Populate new "envelopeIdIndex" attributes. This comment should be updated as well. @@ +819,5 @@ > + return; > + } > + > + let messageRecord = cursor.value; > + if (messageRecord.type == "mms") { bail-out early. @@ +822,5 @@ > + let messageRecord = cursor.value; > + if (messageRecord.type == "mms") { > + for (let ix = 0; ix < messageRecord.receivers.length; ++ix) { > + messageRecord.receivers[ix] = {"address": messageRecord.receivers[ix], > + "type": "PLMN"}; Call |MMS.Address.resolveType()| instead. Please also updates |messageRecord.headers.{to,cc}| for all outgoing MMS messages as well. @@ +894,5 @@ > + // Now we have type in receivers, so we need to pass the array of address > + // without type. > + let receivers = []; > + for (let ix = 0; ix < aMessageRecord.receivers.length; ++ix) { > + receivers.push(aMessageRecord.receivers[ix].address); for (let receiver of aMessageRecord.receivers) { receivers.push(receiver.address); } @@ +1317,5 @@ > + let storedReceiver = messageRecord.receivers[i].address; > + let normStoreReceiver = storedReceiver; > + if (messageRecord.receivers[i].type == "PLMN") { > + normStoreReceiver = PhoneNumberUtils.normalize(storedReceiver, false); > + } For non-PLMN typed addresses, we should probably go full match. Therefore we should not reach here, just have a return: if (aAddress.type != "PLMN") { // Mind |aCreate| here. return; } 928 // 2) Traverse throught all participants and check all alias addresses. 929 aParticipantStore.openCursor().onsuccess = (function (event) {
Attachment #793900 - Flags: feedback?(vyang)
Attached patch bug-907140.patch v1.3 (obsolete) — — Splinter Review
Attachment #793900 - Attachment is obsolete: true
Attached patch bug-907140.patch v1.4 (obsolete) — — Splinter Review
Attachment #794437 - Attachment is obsolete: true
Attachment #794454 - Flags: review?(vyang)
Attachment #794454 - Flags: review?(vyang)
Attached patch bug-907140.patch v1.5 (obsolete) — — Splinter Review
Attachment #794454 - Attachment is obsolete: true
Attachment #794515 - Flags: review?(vyang)
Comment on attachment 794515 [details] [diff] [review] bug-907140.patch v1.5 Review of attachment 794515 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +940,5 @@ > // and local(0987654321) types. The "nationalNumber" parsed from > // phonenumberutils will be "987654321" in this case. > > // Normalize address before searching for participant record. > + let address = aAddress.address ? aAddress.address : aAddress; I think I still can't agree that we should allow two different forms of |aAddress|. It has to be of '{type: <type>, address: <address>}', and we can have: function createParticipantRecord(aParticipantStore, aAddresses, aCallback) { let participantRecord = { addresses: aAddresses }; let addRequest = aParticipantStore.add(participantRecord); addRequest.onsuccess = function (event) { participantRecord.id = event.target.result; aCallback(participantRecord); }; }, function findParticipantRecordByAddress(aParticipantStore, aAddress, aCreate, aCallback) { if (aAddress.type != "PLMN") { // Go full match. let needle = aAddress.address; let request = aParticipantStore.index("addresses").get(needle); request.onsuccess = (function onsuccess(event) { let participantRecord = event.target.result; if (participantRecord) { aCallback(participantRecord); return; } if (aCreate) { this.createParticipantRecord(aParticipantStore, [needle], aCallback); return; } aCallback(null); }).bind(this); return; } // Original phone number matching code ... } The only two things we have to take special care are: 1) |findParticipantRecordByAddress| is also called in |upgradeSchema7|, 2) |findParticipantIdsByAddresses| is also called in |FilterSearcherHelper.transact| Both are easy. Maybe you even want to separate that |findParticipantRecordByAddress| call into two functions -- findParticipantRecordByPlmnAddress| and |findParticipantRecordByOtherAddress|. @@ +1462,5 @@ > let isSuccess = false; > let slicedReceivers = receivers.slice(); > if (aMessage.msisdn) { > + let found = slicedReceivers.indexOf({"address": aMessage.msisdn, > + "type": "PLMN"}); |found| will always be -1 here.
Attachment #794515 - Flags: review?(vyang)
Attached patch bug-907140.patch v1.6 (obsolete) — — Splinter Review
Attachment #794515 - Attachment is obsolete: true
Attachment #795869 - Flags: review?(vyang)
Attachment #795869 - Flags: review?(vyang)
Attached patch bug-907140.patch v1.7 (obsolete) — — Splinter Review
Attachment #795869 - Attachment is obsolete: true
Attachment #796475 - Flags: review?(vyang)
Chia, Please explain if the patch has any risks? Please do risk-assessment of patch.
Flags: needinfo?(ctai)
Attached patch leo-bug-907140.patch v1.8 (obsolete) — — Splinter Review
Per discussed with Vicamo, upload a riskless patch for this bug.
Attachment #796475 - Attachment is obsolete: true
Attachment #796475 - Flags: review?(vyang)
Flags: needinfo?(ctai)
Attachment #799278 - Flags: review?(vyang)
Attachment #799278 - Attachment is obsolete: true
Attachment #799278 - Flags: review?(vyang)
Attachment #799313 - Flags: review?(vyang)
Hi, Preeti, The new patch leo-bug-907140.patch v2.0 should be no risk. It is a small one. Thanks. (In reply to Preeti Raghunath(:Preeti) from comment #22) > Chia, > > Please explain if the patch has any risks? Please do risk-assessment of > patch.
Comment on attachment 799313 [details] [diff] [review] leo-bug-907140.patch v2.0 Review of attachment 799313 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #799313 - Flags: review?(vyang) → review+
https://hg.mozilla.org/mozilla-central/rev/c9a5a622b27b
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: