886764 - [Contacts API] we can have empty strings in the "telMatch" index resulting in matching these contacts for invalid phone numbers in findByIndex

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Description

[ayman maat :maat]

sending an SMS or MMS to a single invalid phone number assigns name of first contact in contact list to the header of the message thread

**PATH**

1) ensure that a contact exists in the contact list with a valid mobile phone number.
2) open message app
3) select new message 
4) add a completely invalid number to the 'to field'
5) type something in the message field
6) select send
 
**ACTUAL**

6.1) Message is created, but sent and fails …however name of first contact in contact list is now present in header for some reason

7) Select header

**ACTUAL** 

overlay opens with invalid number input in step 4) in header and 3 CTAs: 'Call', 'Send Message', Cancel'

8) Select 'Call' returns user to thread view
9) Select 'Send Message' opens the New message dialogue with the name of the contact identified in step 6.1) repopulated in the 'to field'. If the user adds text to the message field and selects send the new message is appended to the thread created in 6.1)

10) return to the message app inbox and select new message
11) navigate to the contact list and select the contact who's name was output in the header in step 6.1)
12) write something in the message field
13) select send
14) navigate back to message app inbox

**ACTUAL**
15) Two threads are now present. the one containing the message sent in 6) and added to in 9), and a new thread continuing the  message sent in 13)

Comment 1

[Rick Waldron [:rwaldron]]

Attachment: carrier tag

When you say header, are you referring to white bar that displays carrier information?

If yes, then please close this bug as a duplicate of bug 885264

Thanks!

Comment 2

[Dietrich Ayala (:dietrich)]

Rick, does bug 885264 also fix the invalid number scenario?

Comment 3

[ayman maat :maat]

(In reply to Rick Waldron from comment #1)
> Created attachment 767264 [details]
> carrier tag
> 
> When you say header, are you referring to white bar that displays carrier
> information?
> 
> If yes, then please close this bug as a duplicate of bug 885264
> 
> Thanks!

No i am referring to the orange bar that contains the name of the contact (if the contact is in the phone book) or the phone number (if the contact is not in the phone book)

I will provide you with screenshots in a moment to clear up any confusion

Comment 4

[ayman maat :maat]

Comment on attachment 767264 [details]
carrier tag

Thats not the header. The header is the large horizontal orange block at the top of the page containing back button, 123456 and edit button. 

what you have highlighted is the 'type / phonenumber' banner.

Comment 5

[ayman maat :maat]

Attachment: HTML5_SMS-MMSAbbreviatedFlow_20130627_Bug886764__V0.1

Reference Comment 0, 1, 3, 4

Comment 6

[bhavana bajaj [:bajaj]]

ayman, Is this a regression or a new request ?

Comment 7

[Rick Waldron [:rwaldron]]

First, thank you for these bug flow diagrams, super helpful!

(In reply to ayman maat :maat from comment #5)
> Created attachment 767681 [details]
> HTML5_SMS-MMSAbbreviatedFlow_20130627_Bug886764__V0.1
> 
> Reference Comment 0, 1, 3, 4


Following this bug flow, I documented the current behaviour: 

  https://www.dropbox.com/sh/4orx66740rckgac/LNZdRQ5PpB 

as it appears on: 

  gaia/master@d34e713d09ac1fadf1e471cca441fcdb93b119b9


The only obvious issue (that I can see) is the carrier tag (which is fixed in bug 885264)

Comment 8

[ayman maat :maat]

Rick, i got a fresh V1 train build today, flashed my device and did a 'git pull' and 'make reset-gaia' ...and can still reproduce the behavior i stated in comment 0 with the additional behavior now of:

6.1) If the invalid number is made up of alphabetic characters such as 'ayman' the messages app attempts to send the message to '29626' which results in an error message being sent from my carrier (though the name on the incoming message in my inbox and in the header of the thread is the 1st name in my contact list) telling me that 'the speed dial 29626 is not assigned'

6.2) If the invalid number is made up of alphabetic characters such as 'YYY' the messages app attempts to send the message to '999' which results in an error message being sent from my carrier (though the name on the incoming message in my inbox and in the header of the thread is the 1st name in my contact list)telling me that 'the speed dial 999 is not assigned'

6.3) If the invalid number is made up '@@@' the messages app fails to send the message.

The result of 6.1 and 6.2 should certainly not happen, so will raise a separate bug about this.

build information:

V1-train
Gecko - 914243b
Gaia - 477e572
build id - 20130627123702 

NeedInfo to Rick and Julien to look into this further as i do not understand why we are getting different results.

Comment 9

[Rick Waldron [:rwaldron]]

First of all, thanks for the thorough descriptions here. I've updated b2g18 (from last midnight) and gaia/master (the idea being that if v1-train has issues, we may need to simply uplift a patch from master)

(In reply to ayman maat :maat from comment #8)
> Rick, i got a fresh V1 train build today, flashed my device and did a 'git
> pull' and 'make reset-gaia' ...and can still reproduce the behavior i stated
> in comment 0 with the additional behavior now of:
> 
> 6.1) If the invalid number is made up of alphabetic characters such as
> 'ayman' the messages app attempts to send the message to '29626' which
> results in an error message being sent from my carrier (though the name on
> the incoming message in my inbox and in the header of the thread is the 1st
> name in my contact list) telling me that 'the speed dial 29626 is not
> assigned'

This translation of alphabetic characters happens after gaia/Messages app calls navigator.mozMobileMessage.send/sendMMS

> 
> 6.2) If the invalid number is made up of alphabetic characters such as 'YYY'
> the messages app attempts to send the message to '999' which results in an
> error message being sent from my carrier (though the name on the incoming
> message in my inbox and in the header of the thread is the 1st name in my
> contact list)telling me that 'the speed dial 999 is not assigned'

I've never seen this message before

> 
> 6.3) If the invalid number is made up '@@@' the messages app fails to send
> the message.

Sounds right to me :)

> 
> The result of 6.1 and 6.2 should certainly not happen, so will raise a
> separate bug about this.
> 
> build information:
> 
> V1-train
> Gecko - 914243b
> Gaia - 477e572
> build id - 20130627123702 
> 
> NeedInfo to Rick and Julien to look into this further as i do not understand
> why we are getting different results.


(this is just notes for now, I'm not going to clear the needsinfo until I have more time to test)

Comment 10

[ayman maat :maat]

(In reply to Rick Waldron from comment #9)

> > 6.2) If the invalid number is made up of alphabetic characters such as 'YYY'
> > the messages app attempts to send the message to '999' which results in an
> > error message being sent from my carrier (though the name on the incoming
> > message in my inbox and in the header of the thread is the 1st name in my
> > contact list)telling me that 'the speed dial 999 is not assigned'
> 
> I've never seen this message before

I am not sure where this message comes from either as i have never seen it before. My hypothesis is that it might have occurred due to Delivery Report being on even though it the switch is turned off in the settings (another bug i raised)... but i cannot be certain of that so like i say it is just a hypothesis.

Comment 11

[Rick Waldron [:rwaldron]]

(In reply to ayman maat :maat from comment #10)
> (In reply to Rick Waldron from comment #9)
> 
> > > 6.2) If the invalid number is made up of alphabetic characters such as 'YYY'
> > > the messages app attempts to send the message to '999' which results in an
> > > error message being sent from my carrier (though the name on the incoming
> > > message in my inbox and in the header of the thread is the 1st name in my
> > > contact list)telling me that 'the speed dial 999 is not assigned'
> > 
> > I've never seen this message before
> 
> I am not sure where this message comes from either as i have never seen it
> before. My hypothesis is that it might have occurred due to Delivery Report
> being on even though it the switch is turned off in the settings (another
> bug i raised)... but i cannot be certain of that so like i say it is just a
> hypothesis.

Cool, thanks for the additional insight!

Comment 12

[Alex Keybl [:akeybl]]

Ayman, would you mind answering comment 6 or better clarifying the reason why this should be considered as a blocker for v1.2?

Comment 13

[Wayne Chang [:wchang]]

Triage - leo+ as according to the bug description the device is telling the user that a message was sent to the first contact in the address book when actually the user is attempting to send to an invalid number (which the user may not know to be invalid).

Comment 14

[Julien Wajsberg [:julienw]]

Note: I've seen this with the reference workloads. There are 3 special threads whose number is "BIG-THREAD-SMS", "BIG-THREAD-MMS" and "BIG-THREAD-MIXED". Of course the numbers are invalid, but that makes it easy to find them when we need it.

However since recently, they get associated with a random contact. Strangely, it's not the first contact, but it consistentely is the same contact each time it starts again. However, I tried resetting my phone + importing the same contacts again, and then another contact was matched.

Taking it to investigate if this is more a Gaia or Device interfaces problem.

Comment 16

[Julien Wajsberg [:julienw]]

see bug 890754 comment 0 for some more information.

Comment 17

[Julien Wajsberg [:julienw]]

I think this comes from Bug 874501 and bug 880644.

Reuben, I think the "normalize-with-only-numbers" step ends up with an empty number when there is no digit, and therefore the match search returns anything.

Reuben, do you think this should be fixed in the Gecko API, or in Gaia (or in both) ?

Comment 18

[Robert Kaiser]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> see bug 890754 comment 0 for some more information.

Note that the case there (SMS received from services) is probably way more common than someone sending messages to non-numbers.

Comment 19

[Julien Wajsberg [:julienw]]

needinfo Michael as well.

In my investigation, we actually get an empty number to search the index for.

Now, does that mean that the telMatch index somehow has empty values ? This looks wrong.
Should we prevent getting empty values ? Should we see why we have empty values in the index ? Should we do both ?

Comment 20

[Julien Wajsberg [:julienw]]

(In reply to Robert Kaiser (:[e-mail]) from comment #18)
> (In reply to Julien Wajsberg [:julienw] from comment #16)
> > see bug 890754 comment 0 for some more information.
> 
> Note that the case there (SMS received from services) is probably way more
> common than someone sending messages to non-numbers.

Yep, but it's more difficult to reproduce ;)

Although the reference workloads ("APP=sms make reference-workload-light" in gaia) simulate this.

Comment 21

[Robert Kaiser]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> In my investigation, we actually get an empty number to search the index for.

I somehow think it might makes sense to stop searching there (I also think it might not be right to search for "2" if the SMS sender is "car2go Wien" as I mentioned in one comment, but we come out well in that case as hopefully no "2" entry exists).

> Now, does that mean that the telMatch index somehow has empty values ?

As I mentioned in bug 890754 comment 0 mine has, and I'm willing to share the DB with a dev if needed (of course, it has my private contacts, so I won't put it up publicly). This might be a different bug, though, but I wondered about that as well. I actually have 3 "" entries in that index.

Comment 22

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> needinfo Michael as well.
> 
> In my investigation, we actually get an empty number to search the index for.
> 
> Now, does that mean that the telMatch index somehow has empty values ? This
> looks wrong.
> Should we prevent getting empty values ? Should we see why we have empty
> values in the index ? Should we do both ?

I just saw that I actually have some empty values in my telMatch index. Both belong to a small 4-digit number.

I also checked that removing these entries fixes the bug for me. So this is it :)

Comment 23

[Julien Wajsberg [:julienw]]

I think we should never have empty values in the index, so we should:

* remove all empty values that exist in the index in a migration path
* never add them in the first place

We should probably not look them up either, but from the current code, it seems that the only way to exit the find function is to abort the transaction, which triggers an error, and I'm not sure that's what we want to do. Therefore ensuring we never have empty values as lookup indexes would be good enough for me.

Mickael, I can try to come up with a patch if your hands are full (but probably not before a few days) ?

Comment 24

[Julien Wajsberg [:julienw]]

Mike will fix this in Gecko :)

Comment 26

[Reuben Morais [:reuben]]

Attachment: Patch

Comment 27

[Reuben Morais [:reuben]]

Comment on attachment 772875 [details] [diff] [review]
Patch

After popping the patch to test the upgrade code I noticed I couldn't reproduce the bug with the steps in comment 0, and inspecting the Contacts DB in the device shows that there are no empty strings in the search indexes.

When I try to send an SMS to "blabla", the header shows "blabla", not any other contact. If I tap the header and save that as a contact, and then try to use the contact to send a new SMS, the app doesn't let me.

Comment 28

[Reuben Morais [:reuben]]

maat, are you running 1.1 or 1.0.1? I wonder if this is fixed on central.

Comment 29

[Julien Wajsberg [:julienw]]

Reuben, I see this bug with the b2g18 branch, and I'll try to reproduce today with mozcentral.

To get an empty string in the index, you need a short number in your address book (short is 4 digits for me).

To search for an empty string, you need a "number-without-digit". BTW I wonder what happens with short numbers as filterValue, I think it works correctly for them.

Comment 30

[ayman maat :maat]

(In reply to Reuben Morais [:reuben] from comment #28)
> maat, are you running 1.1 or 1.0.1? I wonder if this is fixed on central.

i can reproduce this after flashing with one of todays builds and am running:

build-20130710062410
Gecko-7e9bf57
Gaia-e251ee6

however its not now assigning the name of the first contact in contact list to the header of the message thread but one third from the end of the contact list when read A to Z. Its not clear to me why the app is selecting this particular contact. 

If i delete this contact the app is assigning the name of the contact third from the beginning of the contact list. If i delete this contact the app then assignes the name of the contact fourth from the end of the contact list.

...without reading the code I dont understand why it is behaving like this. Non of the tests I am doing are giving me an explanation.

needInfo me if you require further input from me

Comment 31

[Julien Wajsberg [:julienw]]

Ayman, our understanding is that your contacts have an empty string for their 'telMatch' index.

This could be because of a current bug (and then it will be reproducible from a fresh state) or an older bug.

How do you import your contacts ? Do you import them from eg GMail ?

I'm currently trying on a gecko central as well, will report soon.

Comment 32

[Gene Lian (I already quit Mozilla)]

I didn't really follow all the details in this bug but I'm sure there is something to do at Bug 891756 to deal with the invalid number from the Gecko's point of view.

Comment 33

[Julien Wajsberg [:julienw]]

I just tried on central (rev 145340:04d8c309fe72), same problem.

I simply added a contact whose number is simply 0123456789 (valid number in france).

I get the following telMatch values :
""
"0123456789"
"123456789"
"23456789"
"null"

"null" and "" are quite strange to me.

Note that I was using a phone without a sim card here.

Comment 34

[Julien Wajsberg [:julienw]]

Gene> this specific bug is dealing with a matching problem (see duplicates to see that this is more general that simply sending).

Comment 35

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #31)
> Ayman, our understanding is that your contacts have an empty string for
> their 'telMatch' index.
> 
> This could be because of a current bug (and then it will be reproducible
> from a fresh state) or an older bug.
> 
> How do you import your contacts ? Do you import them from eg GMail ?

at the moment i am getting this situation with contacts imported from SIM as that is what i am working on at the moment...will let you know later about other sources, but right now cannot test them

Comment 36

[ayman maat :maat]

...further to comment 35 ...however the original bug in comment 0 was raised with manually input contacts

Comment 37

[Julien Wajsberg [:julienw]]

Ayman> no need to further test, I reproduce it with manually added contacts as well.

Reuben don't hesitate to ask on IRC, I have my indexed db browser ready :)

Comment 38

[Reuben Morais [:reuben]]

Julien, can you try applying my patch and seeing if it fixes the problem? It seems I can't reproduce it with a US SIM card… Adding a contact with number "0123456789" gives me correct entries in the telMatch index.

Comment 39

[Robert Kaiser]

What about "911"? I found out that on my phone it's all short numbers like the the "Euro Emergency Call" 122 and others up to max. 6 digits.

Comment 40

[Gene Lian (I already quit Mozilla)]

(In reply to Julien Wajsberg [:julienw] (PTO 15th -> 28th July) from comment #34)
> Gene> this specific bug is dealing with a matching problem (see duplicates
> to see that this is more general that simply sending).

Got it. Temporally remove the dependency on bug 891756. Please add it back and renominate it to be leo+ if you think it can help solve this bug.

Comment 41

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #38)
> Julien, can you try applying my patch and seeing if it fixes the problem? It
> seems I can't reproduce it with a US SIM card… Adding a contact with number
> "0123456789" gives me correct entries in the telMatch index.

Reuben, at least it didn't fixed the existing entries for me. I haven't had the time to check what happens for a new entry yet... will try to have a look during the week-end.

Comment 42

[Julien Wajsberg [:julienw]]

Comment on attachment 772875 [details] [diff] [review]
Patch

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +364,5 @@
> +      } else if (currVersion == 12) {
> +        if (DEBUG) debug("Cleaning up empty substring entries in telMatch index");
> +        if (!objectStore) {
> +          objectStore = aTransaction.objectStore(STORE_NAME);
> +          objectStore.openCursor().onsuccess = function(event) {

I think you forgot to call `cursor.continue` :)

Comment 43

[Julien Wajsberg [:julienw]]

Attachment: fixed b2g18 patch

This patch works for me on the b2g18 branch (I had to resolve a minor conflict).

I added a cursor.continue at the appropriate place and the bug is resolved after the migration. (because I already did a migration, I had to delete the contacts db, push a gecko without the patch, import my contact from gmail, and push my gecko with the patch).

Comment 44

[Julien Wajsberg [:julienw]]

In _findWithIndex, is it possible to not search when the normalized value is the empty string ? I don't know how this could work with the IndexedBDHelper functions though.

Comment 45

[Julien Wajsberg [:julienw]]

When I say "not search" I mean "returns an empty array without even searching".

Comment 46

[Reuben Morais [:reuben]]

Awesome, thanks for looking!

(In reply to Julien Wajsberg [:julienw] (PTO 15th -> 28th July) from comment #44)
> In _findWithIndex, is it possible to not search when the normalized value is
> the empty string ? I don't know how this could work with the IndexedBDHelper
> functions though.

Everything is possible, it'd just require some rearranging :)
Can we just abort the transaction? It's an invalid number, it makes sense to error out.

Comment 47

[Reuben Morais [:reuben]]

Attachment: Patch with missing continue()

Comment 48

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #46)
> Awesome, thanks for looking!
> 
> (In reply to Julien Wajsberg [:julienw] (PTO 15th -> 28th July) from comment
> #44)
> > In _findWithIndex, is it possible to not search when the normalized value is
> > the empty string ? I don't know how this could work with the IndexedBDHelper
> > functions though.
> 
> Everything is possible, it'd just require some rearranging :)
> Can we just abort the transaction? It's an invalid number, it makes sense to
> error out.

We can say it's invalid for searching with the "match" operation. (please document it ;) ).

Also, might make sense to wait for bug 892497... so that we're sure that the upgrade code to create the parsedTel index and the new upgrade code are correctly called sequentially.

Lastly, I see that in makeImport you check the "tel" index in addition to the "parsedTel" index; do you think it's worth it to check it in the migration code as well ?

Comment 49

[Reuben Morais [:reuben]]

Attachment: Remove empty strings from index, stop them from being saved

This patch removes empty strings from search.tel as well.

Comment 50

[Gregor Wagner [:gwagner]]

Comment on attachment 776708 [details] [diff] [review]
Remove empty strings from index, stop them from being saved

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +453,5 @@
> +          function removeEmptyStrings(value) {
> +            if (value) {
> +              const oldLength = value.length;
> +              for (let i = 0; i < value.length; ++i) {
> +                if (!value[i].length) {

This seems right but we have to make sure we don't have actual 'null' values in there otherwise we would throw an error.

@@ +464,5 @@
> +
> +          let cursor = event.target.result;
> +          if (cursor) {
> +            if (removeEmptyStrings(cursor.value.search.parsedTel) ||
> +                removeEmptyStrings(cursor.value.search.tel)) {

That doesn't work. You don't execute the 2nd removeEmptyStrings if the 1st one returns true;

Comment 51

[Gregor Wagner [:gwagner]]

Comment on attachment 776708 [details] [diff] [review]
Remove empty strings from index, stop them from being saved

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +453,5 @@
> +          function removeEmptyStrings(value) {
> +            if (value) {
> +              const oldLength = value.length;
> +              for (let i = 0; i < value.length; ++i) {
> +                if (!value[i].length) {

Nah we have to change it. It can contain null values.
For example:
https://hg.mozilla.org/mozilla-central/file/582ffcd0459a/dom/contacts/fallback/ContactDB.jsm#l431

Comment 52

[Reuben Morais [:reuben]]

Attachment: Remove empty strings from index, stop them from being saved, v2

interdiff:
diff -u b/dom/contacts/fallback/ContactDB.jsm b/dom/contacts/fallback/ContactDB.jsm
--- b/dom/contacts/fallback/ContactDB.jsm
+++ b/dom/contacts/fallback/ContactDB.jsm
@@ -467,7 +467,7 @@
             if (value) {
               const oldLength = value.length;
               for (let i = 0; i < value.length; ++i) {
-                if (!value[i].length) {
+                if (!value[i]) {
                   value.splice(i, 1);
                 }
               }
@@ -477,8 +477,9 @@
 
           let cursor = event.target.result;
           if (cursor) {
-            if (removeEmptyStrings(cursor.value.search.parsedTel) ||
-                removeEmptyStrings(cursor.value.search.tel)) {
+            let modified = removeEmptyStrings(cursor.value.search.parsedTel);
+            let modified2 = removeEmptyStrings(cursor.value.search.tel);
+            if (modified || modified2) {
               cursor.update(cursor.value);
             }
           } else {

Comment 53

[Gregor Wagner [:gwagner]]

Comment on attachment 777523 [details] [diff] [review]
Remove empty strings from index, stop them from being saved, v2

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +981,5 @@
>          }
> +
> +        if (!normalized.length) {
> +          dump("ContactDB: normalized filterValue is empty, can't perform match search.\n");
> +          return txn.abort();

Should we really abort? Lets just dump an error. Better to match too many phone numbers than complete failure.

Comment 54

[Julien Wajsberg [:julienw]]

(In reply to Gregor Wagner [:gwagner] from comment #51)
> Comment on attachment 776708 [details] [diff] [review]
> Remove empty strings from index, stop them from being saved
> 
> Review of attachment 776708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +453,5 @@
> > +          function removeEmptyStrings(value) {
> > +            if (value) {
> > +              const oldLength = value.length;
> > +              for (let i = 0; i < value.length; ++i) {
> > +                if (!value[i].length) {
> 
> Nah we have to change it. It can contain null values.
> For example:
> https://hg.mozilla.org/mozilla-central/file/582ffcd0459a/dom/contacts/
> fallback/ContactDB.jsm#l431

is it right ?
I mean, should we have null values in the index ? My guts tell me that we could run into the same problem if we don't prevent null values as well :/

Comment 55

[Gregor Wagner [:gwagner]]

Comment on attachment 777523 [details] [diff] [review]
Remove empty strings from index, stop them from being saved, v2

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

Huh this doesn't work
js> let y = {}                                                  
js> y["1234"] = 1                                               
1
js> y[null] = 1                                                 
1
js> y["56789"] = 1
1
js> for (let num in y) { if (num.length) { print(num.length)} }
4
4
5
js>

Comment 56

[Julien Wajsberg [:julienw]]

I don't exactly understand this test. y[null] seems to be equivalent to y["null"], but in the contact DB, it's actually stored in a property and not a key...

Comment 57

[Gregor Wagner [:gwagner]]

(In reply to Julien Wajsberg [:julienw] (PTO 15th -> 28th July) from comment #56)
> I don't exactly understand this test. y[null] seems to be equivalent to
> y["null"], but in the contact DB, it's actually stored in a property and not
> a key...

Sorry for not explaining more. This was just for Reuben.
In order to avoid duplicates we store all numbers as object keys first. The way we wanted to avoid 'null' values in the DB doesn't work and this is what my test here is showing.

Comment 58

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/projects/birch/rev/815fe12820d2

Comment 59

[Gregor Wagner [:gwagner]]

Attachment: patch

Fixed the "null" issue.

Comment 60

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/815fe12820d2

Comment 61

[Julien Wajsberg [:julienw]]

Comment on attachment 782384 [details] [diff] [review]
patch

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +639,5 @@
>                    }
>                  }
>                }
>                for (let num in containsSearch) {
> +                if (num != "null") {

gregor, shouldn't we check for the empty string case as well ?

Comment 62

[Julien Wajsberg [:julienw]]

Comment on attachment 782384 [details] [diff] [review]
patch

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +639,5 @@
>                    }
>                  }
>                }
>                for (let num in containsSearch) {
> +                if (num != "null") {

and a comment would have been good here...

Comment 63

[Ryan VanderMeulen [:RyanVM]]

The b2g18 patch doesn't apply.

Comment 64

[Gregor Wagner [:gwagner]]

(In reply to Julien Wajsberg [:julienw] (PTO 15th -> 28th July) from comment #61)
> Comment on attachment 782384 [details] [diff] [review]
> patch
> 
> Review of attachment 782384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +639,5 @@
> >                    }
> >                  }
> >                }
> >                for (let num in containsSearch) {
> > +                if (num != "null") {
> 
> gregor, shouldn't we check for the empty string case as well ?

This is the path for new numbers and I don't think we can have an empty string here. This can only happen when 'normalized' is empty and we check for this at the beginning.

Comment 65

[Julien Wajsberg [:julienw]]

(In reply to Gregor Wagner [:gwagner] from comment #64)

> This is the path for new numbers and I don't think we can have an empty
> string here. This can only happen when 'normalized' is empty and we check
> for this at the beginning.

right, thanks, I missed that !

Comment 66

[Gregor Wagner [:gwagner]]

Attachment: b2g18 version

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/658f625177ca

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/658f625177ca