There are 2 related problems here. STR: * send a sms to a short number (eg 1234). This should fail. => the displayed message says that there is a network error, and that the message will be resent automatically, which is wrong. This is the generic "default" error message, and it should be modified to be more general. Note that this message is here for december 2012 so it's in 1.0.1, however I think we were not displaying it in that case. Therefore I ask qawanted to check this.

Bug description is not blocking for partners as short numbers could be valid numbers, for example special services/competitions etc. Please renom and elaborate if other considerations need to be taken.

Sorry if that was not clear. The example with short numbers was just an example of network failure, I don't intend to make them invalid. The bug here is that, when the sending fails, the displayed message says there is a network service problem and the message will be automatically resent. That message is displayed for all sending errors. This is wrong because afaik the message is not resent, and there is not a network service problem, rather the number was rejected by the network (but in Gaia we don't really know about this). So this bug is only to rephrase the error message to make it more generic, and not saying anything about retrying automatically or that there is a service network error.

Gecko should return an error code like "InvalidAddress" for this case instead of "InternalError".

The SMS and MMS behaviours are not consistent when sending message with invalid number(s). We need to correct them based on the following rules: 1. We should save the sending message into DB in any way and keep the phone number as it is even if it is invalid. 2. Normalize the phone number and check if it is valid before sending. If it's invalid, return an error code "InvalidAddressError". I'll provide a test cast for this later. Vicamo, could you return some feedback at the same time? Thanks!

Gene, what is an invalid number ? For example, what happens if the user tries to send a small number and it's rejected by the carrier ?

We use the following utility to check that (please see dom/phonenumberutils/PhoneNumber.jsm): function IsPlainPhoneNumber(number) { if (typeof number !== 'string') { return false; } var length = number.length; var isTooLong = (length > MAX_PHONE_NUMBER_LENGTH); var isEmpty = (length === 0); return !(isTooLong || isEmpty || NON_DIALABLE_CHARS_ONCE.test(number)); } We've already had this check for sending SMS and dialing calls. At least, we have to support the same check for sending MMS to make sure all the behaviours are consistent. However, this still doesn't solve the issue here because "1234" can pass this check. The RIL modem will return ERROR_GENERIC_FAILURE after sending SMS and the platform will expose "UnknownError" to the content in the end. The Gaia shouldn't pop up a prompt like "Service currently unavailable" to handle "UnknownError". It doesn't sound precise because the service is actually on. In my opinion, Gaia should only pop up such a prompt when handling "NoSignalError". Therefore, the Gaia end also has some work to do to refine the UI.

(In reply to Gene Lian [:gene] from comment #6) > The Gaia shouldn't pop up a prompt like "Service currently unavailable" to > handle "UnknownError". It doesn't sound precise because the service is > actually on. In my opinion, Gaia should only pop up such a prompt when > handling "NoSignalError". Therefore, the Gaia end also has some work to do > to refine the UI. I fully agree, thanks for opening bug 891855. removing late-l10n from here because it belongs in bug 891855 then.

Great! Thanks for your understanding! Btw, I feel a bit sorry to rename this bug and open another one. I should do it in an opposite way. ;)

I still would like qawanted to check how it works in 1.0.1 (see comment 0) to support having leo+ on this and bug 891855.

note that this bug and bug 891855 must land together, because we sadly have no "default" clause in the switch...

have tested two scenarios: 1. send SMS to 0000 (the issue did not repro using 1234) - v.1.1. - network error notification (as in comment 0) - v.1.0.1- the message is pending, no notification is presented to the user 2. send SMS to the wrong number (656)933-6995 - SMS "Error Invalid Number, re-send using a valid number or short code" is received on both v1.1 and v1.0.1 *Unagi *latest 7/10 builds for v1.1. and 1.0.1

(In reply to Julien Wajsberg [:julienw] from comment #11) > note that this bug and bug 891855 must land together, because we sadly have > no "default" clause in the switch... Got it. We won't land this until bug 891855 is ready.

leo- this given where we are in the cycle as though its not the best error message we are returning, we could continue improving it in 1.2

Comment on attachment 773753 [details] [diff] [review] Part 1, provide "InvalidAddressError" Review of attachment 773753 [details] [diff] [review]: ----------------------------------------------------------------- Thank you, but it's time to define a more strict policy for returning errors. Let's see if we can do better here :) ::: dom/mobilemessage/src/ril/MmsService.js @@ +1557,5 @@ > let receivers = aParams.receivers; > if (receivers.length != 0) { > let headersTo = headers["to"] = []; > for (let i = 0; i < receivers.length; i++) { > + headersTo.push({"address": receivers[i], "type": "PLMN"}); |headers["to"]| is not exposed to content, |receivers| is. So we can surely store something different from users' input here so that we don't have to re-normalize these addresses many times. Please keep what it originally looks like. @@ +1711,5 @@ > + let isAddrValid = true; > + let headers = savableMessage.headers; > + if (!headers) { > + isAddrValid = false; > + } |headers| is always valid because |createSavableFromParams()| always assigns one for each savable MMS message. @@ +1715,5 @@ > + } > + let receivers = headers.to; > + if (isAddrValid && (!receivers || receivers.length == 0)) { > + isAddrValid = false; > + } For invalid MmsParameters format, we should throw an exception to keep synced with what SmsIPCService does. That is, 1) |receivers| must be an array of strings, 2) |subject| and |smil| must be strings if defined, 3) |attachments| must be an array of MmsAttachment. Note we do not check |receivers.length| in SmsIPCService for now, and we should fix that as well. Please move all these format checks to the very beginning of |MmsService::send()| and throws Cr.NS_ERROR_INVALID_ARG. @@ +1720,5 @@ > + for (let i = 0; isAddrValid && i < receivers.length; i++) { > + let address = receivers[i].address; > + let normalizedAddr = receivers[i].address = > + PhoneNumberUtils.normalize(address, false); > + if (DEBUG) debug("Normalizing " + address + " to " + normalizedAddr); Move into |createSavableFromParams()|.

Comment on attachment 773748 [details] [diff] [review] Part 2, test case Review of attachment 773748 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/tests/marionette/test_invalid_address.js @@ +88,5 @@ > +}); > + > +tasks.push(function () { > + log("mozMobileMessage.send(...) should get 'InvalidAddressError' error " + > + "when attempting to send SMS with an invalid phone number."); Could you help abstract this function into a helper function that accepts a phone number? I think we will need some more test cases like "", null, etc. @@ +103,5 @@ > +}); > + > +tasks.push(function () { > + log("mozMobileMessage.sendMMS(...) should get 'InvalidAddressError' error " + > + "when attempting to send MMS with invalid phone numbers."); ditto.

Comment on attachment 775611 [details] [diff] [review] Part 1, provide "InvalidAddressError", V2 Review of attachment 775611 [details] [diff] [review]: ----------------------------------------------------------------- Please check with http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/ipc/SmsIPCService.cpp#173 , there are still several differences. ::: dom/mobilemessage/src/gonk/MmsService.js @@ +1674,5 @@ > + // Check if the |receivers| is valid. > + let receivers = aParams.receivers; > + if (!Array.isArray(receivers) || receivers.length == 0) { > + if (DEBUG) debug("Error! 'receivers' should be a non-empty array."); > + throw Cr.NS_ERROR_INVALID_ARG; Please also make http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/ipc/SmsIPCService.cpp#224 return NS_ERROR_INVALID_ARG. @@ +1676,5 @@ > + if (!Array.isArray(receivers) || receivers.length == 0) { > + if (DEBUG) debug("Error! 'receivers' should be a non-empty array."); > + throw Cr.NS_ERROR_INVALID_ARG; > + return; > + } else { Don't need |else| after a return statement. @@ +1703,5 @@ > + > + // Check if the |attachments| is valid. > + let attachments = aParams.attachments; > + if (attachments != null && > + (!Array.isArray(attachments) || attachments.length == 0)) { http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/ipc/SmsIPCService.cpp#196 always asks for an array, but I do think we should allow undefined/null or empty array here. @@ +1754,2 @@ > gMobileMessageDatabaseService > + .saveSendingMessage(aSavableMessage, This won't work because aSavableMessage will remain undefined after the call any way. Besides, please don't rename |savableMessage| to |aSavableMessage| here because it's not born an argument.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21) > http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/ipc/ > SmsIPCService.cpp#196 always asks for an array, but I do think we should > allow undefined/null or empty array here. I'd prefer firing another bug for this. Let's just have the minimum changes for solving this bug. We're in the v1.1 cycle now and should avoid doing too many clean-up tasks in the leo+ bugs. > > @@ +1754,2 @@ > > gMobileMessageDatabaseService > > + .saveSendingMessage(aSavableMessage, > > This won't work because aSavableMessage will remain undefined after the call > any way. Besides, please don't rename |savableMessage| to |aSavableMessage| > here because it's not born an argument. Stupid me!

Comment on attachment 776326 [details] [diff] [review] Part 1, provide "InvalidAddressError", V3.1 Review of attachment 776326 [details] [diff] [review]: ----------------------------------------------------------------- Thank you :)

Given that this bug requires a l10n-impact change on the gaia side via bug 891855 (late-l10n is over there), is there a rationale why this is actually blocking at this stage in the release cycle?

(In reply to Axel Hecht [:Pike] from comment #26) > Given that this bug requires a l10n-impact change on the gaia side via bug > 891855 (late-l10n is over there), is there a rationale why this is actually > blocking at this stage in the release cycle? I don't have strong opinion about whether this one is blocking or not. This bug has been marked as leo+ by Leo. The purpose of this bug is to make Gecko return a new error code "InvalidAddressError" to Gaia when the Message App attempts to send an message to invalid addresses like "&%&". The current prompt is showing "Service currently unavailable" for that, which is obviously wrong. We also need the Gaia's support at bug 891855 to have a proper prompt to reflect the new error code. That's why this bug blocks that one.

The only change for part-1 patch is remove the check for empty string in saveSendingMessage. My point is: if we can save "&%&", I think it's reasonable to save "" as well. They are equivalent in my point of view: they are all invalid addresses.

Try server for test case: https://tbpl.mozilla.org/?tree=Try&rev=f2a1a0c61a0a

Comment on attachment 777683 [details] [diff] [review] Part 2, test case, V2 Review of attachment 777683 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/tests/marionette/test_invalid_address.js @@ +86,5 @@ > + > + tasks.next(); > +}); > + > +function testInvalidAddressForSMS(aInvalidAddr) { Please place all utility functions before tasks, then we have a much clear, continuous view of the flow. :) @@ +104,5 @@ > + > +tasks.push(testInvalidAddressForSMS.bind(this, "&%&")); > +tasks.push(testInvalidAddressForSMS.bind(this, "")); > + > +function testInvalidAddressForMMS(aInvalidAddrs) { ditto.

Fix the last comment.

(In reply to Axel Hecht [:Pike] from comment #26) > Given that this bug requires a l10n-impact change on the gaia side via bug > 891855 (late-l10n is over there), is there a rationale why this is actually > blocking at this stage in the release cycle? I'd suggest let's directly land this to b2g18 as well since both Gecko (this bug) and Gaia (bug 891855) are ready to go.

https://hg.mozilla.org/projects/birch/rev/b88a9379e980 https://hg.mozilla.org/projects/birch/rev/7b8f3dee3508

(In reply to Gene Lian [:gene] from comment #37) > https://hg.mozilla.org/projects/birch/rev/b88a9379e980 > https://hg.mozilla.org/projects/birch/rev/7b8f3dee3508 Next time, please consolidate multiple csets into one push so we're not spinning two sets of builds and tests and wasting resources.

https://hg.mozilla.org/mozilla-central/rev/b88a9379e980 https://hg.mozilla.org/mozilla-central/rev/7b8f3dee3508

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38) > (In reply to Gene Lian [:gene] from comment #37) > > https://hg.mozilla.org/projects/birch/rev/b88a9379e980 > > https://hg.mozilla.org/projects/birch/rev/7b8f3dee3508 > > Next time, please consolidate multiple csets into one push so we're not > spinning two sets of builds and tests and wasting resources. Yes, I know this policy and I always follow that except for this mistake. Sorry about that.

Marking checkin-needed to land these two b2g18 patches after the b2g18 repository is open.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5f9bcb5dc193 https://hg.mozilla.org/releases/mozilla-b2g18/rev/6c523ffe2a33

This was backed out and relanded while chasing down test failures introduced by the push that this was a part of. https://hg.mozilla.org/releases/mozilla-b2g18/rev/6ef8f8777c69 https://hg.mozilla.org/releases/mozilla-b2g18/rev/c7e0efd51559 https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/6ef8f8777c69 https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/c7e0efd51559

Since both the issues are Resolved Fixed,clearing the flag. Thanks,