Closed Bug 886764 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: maat, Assigned: reuben)

References

Details

(Whiteboard: MMS_TEF, [u=commsapps-user c=messaging p=0])

Attachments

(4 files, 5 obsolete files)

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)
blocking-b2g: --- → leo?
Whiteboard: MMS_TEF
Depends on: 823392
Attached image 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!
Attachment #767264 - Flags: feedback?(aymanmaat)
Flags: needinfo?(aymanmaat)
Assignee: nobody → waldron.rick
Rick, does bug 885264 also fix the invalid number scenario?
(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
Flags: needinfo?(aymanmaat)
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.
Reference Comment 0, 1, 3, 4
ayman, Is this a regression or a new request ?
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)
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.
Flags: needinfo?(waldron.rick)
Flags: needinfo?(felash)
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)
(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.
(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!
Flags: needinfo?(waldron.rick)
Depends on: 888306
Ayman, would you mind answering comment 6 or better clarifying the reason why this should be considered as a blocker for v1.2?
Flags: needinfo?(aymanmaat)
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).
blocking-b2g: leo? → leo+
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.
Assignee: waldron.rick → felash
Flags: needinfo?(felash)
Whiteboard: MMS_TEF → MMS_TEF, [u=commsapps-user c=messaging p=0]
Duplicate of this bug: 890754
see bug 890754 comment 0 for some more information.
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) ?
Flags: needinfo?(reuben.bmo)
Flags: needinfo?(aymanmaat)
(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.
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 ?
Flags: needinfo?(mhenretty)
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
(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.
(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 :)
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) ?
Mike will fix this in Gecko :)
Assignee: felash → mhenretty
Flags: needinfo?(reuben.bmo)
Flags: needinfo?(mhenretty)
Attachment #767264 - Flags: feedback?(aymanmaat)
Duplicate of this bug: 885313
Assignee: mhenretty → reuben.bmo
Component: Gaia::SMS → DOM: Device Interfaces
OS: Mac OS X → All
Product: Boot2Gecko → Core
Hardware: x86 → All
Version: unspecified → Trunk
Attached patch Patch (obsolete) — Splinter Review
Attachment #772875 - Flags: review?(anygregor)
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.
Attachment #772875 - Flags: review?(anygregor)
maat, are you running 1.1 or 1.0.1? I wonder if this is fixed on central.
Flags: needinfo?(aymanmaat)
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.
Flags: needinfo?(felash)
(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
Flags: needinfo?(aymanmaat)
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.
Flags: needinfo?(aymanmaat)
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.
Depends on: 891756
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.
Gene> this specific bug is dealing with a matching problem (see duplicates to see that this is more general that simply sending).
Flags: needinfo?(felash)
Summary: [SMS/MMS] 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 → [SMS/MMS] we can have empty strings in the "telMatch" index resulting in matching these contacts for invalid phone numbers in findByIndex
(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
Flags: needinfo?(aymanmaat)
...further to comment 35 ...however the original bug in comment 0 was raised with manually input contacts
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 :)
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.
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.
No longer depends on: 891756
Summary: [SMS/MMS] we can have empty strings in the "telMatch" index resulting in matching these contacts for invalid phone numbers in findByIndex → [Contacts API] we can have empty strings in the "telMatch" index resulting in matching these contacts for invalid phone numbers in findByIndex
Flags: needinfo?(felash)
(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.
Priority: -- → P2
Target Milestone: --- → 1.1 QE5
(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.
Flags: needinfo?(felash)
Target Milestone: 1.1 QE5 → 1.1 QE4 (15jul)
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` :)
Attached patch fixed b2g18 patch (obsolete) — Splinter Review
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).
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.
When I say "not search" I mean "returns an empty array without even searching".
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.
Attached patch Patch with missing continue() (obsolete) — Splinter Review
Attachment #772875 - Attachment is obsolete: true
Attachment #775243 - Flags: review?(anygregor)
(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 ?
Attachment #775243 - Flags: review?(anygregor)
This patch removes empty strings from search.tel as well.
Attachment #775243 - Attachment is obsolete: true
Attachment #776708 - Flags: review?(anygregor)
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 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
Attachment #776708 - Flags: review?(anygregor)
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 {
Attachment #776708 - Attachment is obsolete: true
Attachment #777523 - Flags: review?(anygregor)
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.
Attachment #777523 - Flags: review?(anygregor) → review+
(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 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>
Attachment #777523 - Flags: review+
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...
(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.
Attached patch patchSplinter Review
Fixed the "null" issue.
Attachment #777523 - Attachment is obsolete: true
Attachment #782384 - Flags: review+
Attachment #782384 - Attachment description: reubenxx.diff → patch
https://hg.mozilla.org/mozilla-central/rev/815fe12820d2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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 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...
The b2g18 patch doesn't apply.
(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.
(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 !
Attached patch b2g18 versionSplinter Review
Attachment #775219 - Attachment is obsolete: true
Flags: needinfo?(reuben.bmo)
You need to log in before you can comment on or make changes to this bug.