Closed Bug 1154993 Opened 5 years ago Closed 5 years ago

[flame][3.0]Can't view SMS that has no phone number associated

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kairo, Assigned: julienw)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file)

SMS I get from some services, including from my bank for transaction codes, or from my operator for missed calls, only have a name but no phone number assigned.

Right now on master/nightly ("3.0") with my Flame, I neither get a notification for them, nor can I view them in the SMS app.

When I tap the unread message in the SMS app to try and view it, I get the following messages in logcat:

I/Gecko   (  207): ContactDB: normalized filterValue is empty, can't perform match search.
E/Messages( 1353): Content JS ERROR: Got an error while looking up contacts. UnknownError [object DOMError] 
E/Messages( 1353):     at contacts_findBy/< (app://sms.gaiamobile.org/gaia_build_defer_index.js:419:31)
E/Messages( 1353): Content JS ERROR: Navigation.toPanel: got an exception: ([object DOMError]) undefined 
E/Messages( 1353):     at catchError (app://sms.gaiamobile.org/gaia_build_defer_index.js:378:193)
E/Messages( 1353):     at rejected (app://sms.gaiamobile.org/gaia_build_defer_index.js:396:170)
May I know what kind of services could send the message without number but still have name displayed? Maybe some more image for the receiving the notification and message threads view would be helpful, thanks!
[Blocking Requested - why for this release]:

Likely a regression from bug 1142540.

I do have such cases as well. I think the "sender" property contains an arbitrary string that's not a number. As seen above, we get an error from mozContacts in that case (if there are only non-digits).
Blocks: 1142540
blocking-b2g: --- → 3.0?
So we have a rejected promise at the end of the chain, because we assumed mozContacts error should not be recoverable. But we see here that at least one case should be recoverable.

But we can't distinguish easily between a real error and this error.

So here are 2 possible fixes:
* do not call mozContacts if no character is a digit.
* mozContacts should not return an error if the input is non-empty but is normalized as an empty value. It should just return an empty result.

NI Gregor on the second possible fix, which looks like the most correct to me.
Flags: needinfo?(anygregor)
Reuben, can you take a look here?
Flags: needinfo?(anygregor) → needinfo?(reuben.bmo)
I can also do the change if you think the change should be in the Contacts API.
Just my 2p: this is horrible for dogfooding, if it cannot be fixed very quickly backing out bug 1142540 (which fixes the issue, tested locally) and re-landing it once the underlying problem is solved. To give you the feeling of how bad it is I can't access messages from 2FA services that send you codes including GitHub, my bank and the Dutch DigiD system for accessing government sites.
Let me take this, I'll prepare a patch for mozContacts today.
Assignee: nobody → felash
Comment on attachment 8594706 [details] [review]
[gaia] julienw:1154993-recover-contacts-errors > mozilla-b2g:master

After further thinking, I now think it's always bad to choke on some API errors; basic functionality should never break.

So let's recover from mozContacts error which is what we used to do before bug 1142540. I checked that this fixes the issue for me.

I still think that mozContacts should not send an error in this case, but this could be handled in a separate bug.
Attachment #8594706 - Flags: review?(azasypkin)
Whiteboard: [p=1]
Comment on attachment 8594706 [details] [review]
[gaia] julienw:1154993-recover-contacts-errors > mozilla-b2g:master

PR looks good to me, but how do you feel about small integration test for this bug to avoid such breakage in the future? 

Please, re-request review if you decide to do it in this PR, otherwise I'm fine with test-only follow-up.
Attachment #8594706 - Flags: review?(azasypkin) → review+
Depends on: 1156288
Filed bug 1156288 as a follow-up for integration tests.
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1156651
Already landed in 3.0.
blocking-b2g: 3.0? → ---
There's no contract that says API errors are all unrecoverable, and in fact this isn't the only recoverable error we throw in the API. I don't think changing the behavior is worth the trouble in this case.
Flags: needinfo?(reuben.bmo)
You need to log in before you can comment on or make changes to this bug.