Last Comment Bug 883770 - [Dialer][SMS][Contacts] Call log does no longer show contact names after version upgrade
: [Dialer][SMS][Contacts] Call log does no longer show contact names after vers...
: dataloss, regression
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla25
Assigned To: Michael Henretty [:mhenretty]
: Andrew Overholt [:overholt]
: 885587 (view as bug list)
Depends on:
Blocks: 883750 883756 883821 885587 890909
  Show dependency treegraph
Reported: 2013-06-17 02:03 PDT by Henrik Skupin (:whimboo)
Modified: 2013-07-09 02:09 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (987 bytes, patch)
2013-06-25 00:30 PDT, Gregor Wagner [:gwagner]
bent.mozilla: review+
Details | Diff | Splinter Review
Patch with upgrade path fix (1.70 KB, patch)
2013-06-25 20:19 PDT, Michael Henretty [:mhenretty]
anygregor: review+
Details | Diff | Splinter Review
Followup (1.24 KB, patch)
2013-07-01 20:18 PDT, Gregor Wagner [:gwagner]
reuben.bmo: review+
Details | Diff | Splinter Review

Description Henrik Skupin (:whimboo) 2013-06-17 02:03:28 PDT
Gaia   0b0cd9ee3eb9fb09f8505214615492bcc5eff7da
BuildID 20130616230206
Version 18.0

Since I have updated my phone from the build below, the call does no longer show contact names. Only the phone numbers called are visible. This is a regression between builds 20130613230207 and 20130616230206.

Gaia   0c836be7f24693b74ad7fd21c8153824f1c7f548
BuildID 20130613230207
Version 18.0
Comment 1 Henrik Skupin (:whimboo) 2013-06-17 02:05:54 PDT
Might be this is caused by a Gecko change? In the call log I only see normalized phone numbers without brackets and spaces while in my contacts every one has those special characters.
Comment 2 Julien Wajsberg [:julienw] 2013-06-17 03:28:22 PDT
Bug 871930 removed some magic we had in Dialer, not sure this is related (because it landed before your 20130613 build). Maybe Etienne will have more information.
Comment 3 Henrik Skupin (:whimboo) 2013-06-19 02:09:18 PDT
This even doesn't work for local numbers without the country prefix and any brackets nor spaces. Just a number like 0351123456 will fail to show the contact name in the call log.
Comment 4 Gregor Wagner [:gwagner] 2013-06-19 11:11:02 PDT
Can you try to delete the contact and add it again? This might be an issue of a missing DB migration code.
Comment 5 Marcia Knous [:marcia - use ni] 2013-06-19 15:26:13 PDT
Henrik: Can you add a screenshot of what you see? Parul has been testing this today to see if she can reproduce.
Comment 6 Etienne Segonzac (:etienne) 2013-06-20 07:39:14 PDT
(In reply to Gregor Wagner [:gwagner] from comment #4)
> Can you try to delete the contact and add it again? This might be an issue
> of a missing DB migration code.

Yep. The call log is working okay.
Looks like a migration issue :/
Comment 7 Julien Wajsberg [:julienw] 2013-06-20 08:00:58 PDT
same thing for SMS by the way.

* the contact would not match on a number (no difference in internationalization, space, etc, it was a plain internationalized number both in sms and in contact)
* I updated the contact, changed a number and put it back
=> it was matching again.
Comment 8 Julien Wajsberg [:julienw] 2013-06-20 09:54:21 PDT
I'd like to add that my contacts were freshly imported contacts from GMail, so I don't think it was a migration problem for me, but I may miss something.
Comment 9 Henrik Skupin (:whimboo) 2013-06-20 10:45:55 PDT
(In reply to Julien Wajsberg [:julienw] from comment #7)
> both in sms and in contact)
> * I updated the contact, changed a number and put it back
> => it was matching again.

Yeah, I can confirm that! I have only updated the name of the contact and checked the call log and sms list. Now the name is displayed correctly again. So what kind of migration issue could this be? Have we updated the schema of the contacts database lately, and with the update of the contact we correctly write out the information?

If that is the case it seems to be very important to get fixed before any of our users will get updated from 1.0 to 1.1.

I don't think the request for a screenshot is still valid? Given that others do also see that.
Comment 10 Julien Wajsberg [:julienw] 2013-06-21 03:55:27 PDT
We need leo+ on this, lots of contact matching is broken right now because of this.
Comment 11 Gregor Wagner [:gwagner] 2013-06-21 11:11:25 PDT
We know what's going on here. It's the missing migration code for the match field.
Reuben can you take this?
Comment 12 Reuben Morais [:reuben] 2013-06-21 23:21:31 PDT
What missing migration?
Comment 13 Julien Wajsberg [:julienw] 2013-06-22 07:19:15 PDT
*** Bug 885587 has been marked as a duplicate of this bug. ***
Comment 14 Gregor Wagner [:gwagner] 2013-06-25 00:30:19 PDT
Created attachment 767071 [details] [diff] [review]
Comment 15 Gregor Wagner [:gwagner] 2013-06-25 00:43:02 PDT
Ah how much do I love this upgrade scenario.
The bad thing is that this doesn't solve the problem for people that already see this bug. This will only solve it for users that haven't upgraded yet.
Comment 16 Henrik Skupin (:whimboo) 2013-06-25 00:45:16 PDT
Would another update of the schema help here?
Comment 17 Gregor Wagner [:gwagner] 2013-06-25 02:15:23 PDT
(In reply to Henrik Skupin (:whimboo) [away 06/28 - 07/07] from comment #16)
> Would another update of the schema help here?

Yeah I was thinking more about it. We should make the buggy upgrade code a noop and bump the revision number again with a correct upgrade code.
Michael is taking care of it :)
Comment 18 Naoki Hirata :nhirata (please use needinfo instead of cc) 2013-06-25 14:15:58 PDT
Based on comment 11 and having a patch, removing regressionwindow-wanted.
Comment 19 Michael Henretty [:mhenretty] 2013-06-25 20:19:29 PDT
Created attachment 767570 [details] [diff] [review]
Patch with upgrade path fix
Comment 20 Gregor Wagner [:gwagner] 2013-07-01 04:15:24 PDT
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-07-01 06:06:09 PDT
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-07-01 10:56:45 PDT
Comment 24 Gregor Wagner [:gwagner] 2013-07-01 20:18:53 PDT
Created attachment 769995 [details] [diff] [review]

My mistake :(
Comment 25 Reuben Morais [:reuben] 2013-07-01 20:22:16 PDT
Comment on attachment 769995 [details] [diff] [review]

Review of attachment 769995 [details] [diff] [review]:

Good catch.
Comment 27 Ryan VanderMeulen [:RyanVM] 2013-07-02 12:38:08 PDT
Comment 29 Julien Wajsberg [:julienw] 2013-07-07 08:51:45 PDT
I wonder if there should be another follow up, because the 'match' search does not match anything now.

(however, 'contains' search still works ok)
Comment 30 Henrik Skupin (:whimboo) 2013-07-07 10:13:35 PDT
Whatever has been done here, this is not fixed on my Unagi for the nightly channel:

Gaia   d336288e6cda1a8f974ceea41b9d7860c2367d1f
BuildID 20130706230209
Version 18.0
Comment 31 Julien Wajsberg [:julienw] 2013-07-08 04:28:46 PDT
I verified this works for new contact but still not for the old ones :(

Michael, Gregor, can I somehow investigate what's inside the indexeddb ?
Comment 32 Henrik Skupin (:whimboo) 2013-07-08 05:09:01 PDT
Julien, I assume that we should reopen the  bug completely, or? I don't think that it will be fixed on trunk either.
Comment 33 Michael Henretty [:mhenretty] 2013-07-08 05:36:46 PDT
Julien, you can see what's in the contacts DB by using bent's IndexedDB FF extension:

To use the extension for this purpose, you will need to first adb pull the contacts sqlite file off the phone and place it in your FF profile folder for indexeddb (also make sure to delete any .DS_Store files if you're using MacOS finder to move stuff around). Restart your browser, then open the IndexedDB browser (Tools->Web Developer->IDB Browser), and drill into the DB named "contacts." Inside "contacts" there will be another "contacts" entry, click on this and click the Load Data button. Here you should see all your contacts. If you examine the the value of one, you should see a "parsedTel" entry in the JSON string. If it's missing, the upgrade probably failed. Let me know if you encounter any hiccups, cause I had to do some wrangling to get it to work for me.

Reopening this bug for further investigation.
Comment 34 Julien Wajsberg [:julienw] 2013-07-08 07:00:13 PDT
It seems that the problem comes with using |this.substringMatching| in most places except the migration path.

Therefore, for a number +33123456789, we try to match 23456789 whereas only +33123456789, 123456789 and 0123456789 are in the index. However, adding/updating or importing generates the index correctly.
Comment 35 Jason Smith [:jsmith] 2013-07-08 07:14:23 PDT
Guys - Can you file a followup bug for this?
Comment 36 Julien Wajsberg [:julienw] 2013-07-08 07:18:28 PDT

Could use the just-filed Bug 890909 (that I was going to dupe ;) ).
Comment 37 Gregor Wagner [:gwagner] 2013-07-08 14:25:40 PDT
(In reply to Julien Wajsberg [:julienw] from comment #34)
> It seems that the problem comes with using |this.substringMatching| in most
> places except the migration path.
> Therefore, for a number +33123456789, we try to match 23456789 whereas only
> +33123456789, 123456789 and 0123456789 are in the index. However,
> adding/updating or importing generates the index correctly.

Hm I don't see how this can reduce the search space. The substringMatching only adds the last x digits to the search space but we should still find the number with the national formats. 
What format is 23456789 in your example? Isn't 0123456789 or 123456789 the local format?
Comment 38 Julien Wajsberg [:julienw] 2013-07-08 14:35:03 PDT
It reduces the search space because the value that is looked for is only the sliced value... which is not in the index.


When investigating with Mike, we found that being in France I should not be using the sliced value, but because "init" is called too soon, we always end up with the default brazil configuration. 

But that's another issue, and we should file another bug for this. I also don't understand why we don't always slice for all countries... because we don't want to match 2 similar numbers from 2 different countries?
Comment 39 Henrik Skupin (:whimboo) 2013-07-09 00:57:10 PDT
Julien, why did you move the status to affected again? I thought we have bug 890909 for the followup work now?
Comment 40 Julien Wajsberg [:julienw] 2013-07-09 02:09:10 PDT
completely right, I mixed up my bugs, sorry.

Note You need to log in before you can comment on or make changes to this bug.