Last Comment Bug 890909 - [Contacts API] migration code problem when matching using the substringMatching pref (eg in Brazil)
: [Contacts API] migration code problem when matching using the substringMatchi...
Status: RESOLVED FIXED
[u=commsapps-user c=messaging p=0]
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla25
Assigned To: Michael Henretty [:mhenretty]
:
:
Mentors:
: 894739 (view as bug list)
Depends on: 883770
Blocks: koi-system-fe 894739 896351
  Show dependency treegraph
 
Reported: 2013-07-08 06:35 PDT by Sebastian Hengst [:aryx][:archaeopteryx]
Modified: 2013-07-31 05:41 PDT (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
leo+
wontfix
wontfix
fixed
fixed
wontfix
wontfix
fixed


Attachments
WiP (19 bytes, patch)
2013-07-09 18:41 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
patch (731 bytes, patch)
2013-07-09 18:42 PDT, Gregor Wagner [:gwagner]
no flags Details | Diff | Splinter Review
Add lastKnownSimMcc (16.54 KB, patch)
2013-07-16 09:15 PDT, Michael Henretty [:mhenretty]
no flags Details | Diff | Splinter Review
Add lastKnownSimMcc (16.21 KB, patch)
2013-07-16 09:28 PDT, Michael Henretty [:mhenretty]
no flags Details | Diff | Splinter Review
Add lastKnownSimMcc (16.25 KB, patch)
2013-07-16 14:48 PDT, Michael Henretty [:mhenretty]
no flags Details | Diff | Splinter Review
Patch lastKnownSimMcc (17.23 KB, patch)
2013-07-16 17:38 PDT, Michael Henretty [:mhenretty]
no flags Details | Diff | Splinter Review
Patch lastKnownSimMcc (17.04 KB, patch)
2013-07-16 18:07 PDT, Michael Henretty [:mhenretty]
no flags Details | Diff | Splinter Review
Patch lastKnownSimMcc (11.92 KB, patch)
2013-07-17 17:04 PDT, Michael Henretty [:mhenretty]
anygregor: review+
Details | Diff | Splinter Review
B2G-18 Patch (15.95 KB, patch)
2013-07-22 10:04 PDT, Michael Henretty [:mhenretty]
anygregor: review+
Details | Diff | Splinter Review
Updated Patch for B2G-18 (16.28 KB, patch)
2013-07-26 11:18 PDT, Michael Henretty [:mhenretty]
anygregor: review+
Details | Diff | Splinter Review
lastknownsimmcc.patch (16.26 KB, patch)
2013-07-28 17:06 PDT, Michael Henretty [:mhenretty]
no flags Details | Diff | Splinter Review

Description Sebastian Hengst [:aryx][:archaeopteryx] 2013-07-08 06:35:59 PDT
+++ This bug was initially created as a clone of Bug #883663 +++

Unagi with Gaia 1.1.0.0-prerelease 20130707230209

A SMS conversation thread (the only one) shows only the number of the other participant (starting with +491), but showed the name two weeks before. The participant is saved as contact with the same number (+491).
Comment 1 Julien Wajsberg [:julienw] 2013-07-08 07:19:48 PDT
See bug 883770 for background, this is a follow-up. Especially see bug 883770 comment 34.
Comment 2 Michael Henretty [:mhenretty] 2013-07-08 16:41:38 PDT
After talking with Julien, the problem appears to be that the Contacts Service and Phone Utils (which contains the normalization logic) is initialized before we have obtained the country code from the RIL. The result is that Julien's phone is doing substringMatching as if his country was the default Brazil instead of France. So to fix, we are going to make sure the country code is updated in the Phone Utils and the Contacts Service whenever RIL state changes. As an added cautionary change, we are going to update the lastKnownMcc field of the ril_context as soon as we get info from the sim.
Comment 3 Henrik Skupin (:whimboo) 2013-07-09 02:26:29 PDT
Would you mind to update the bug summary to better reflect the underlying issue? With a couple of those bugs open it's getting hard to separate them. Thanks.
Comment 4 Julien Wajsberg [:julienw] 2013-07-09 03:24:15 PDT
I think there are several problems that each belong to different bugs.

1 - the substringMatching is always enabled
-> this happens because we're trying to get the country before the SIM information was even read (in [1]), therefore the default country is used, that is Brazil.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/ContactService.jsm#46

2 - migration issue when the substringMatching is enabled
-> the migration code does not use the substringMatching pref to generate the various values pushed to the "parsedTel" property (see [1])
However, the lookup code uses it (see [2]), and therefore, does not match anything.
But of course, you need to be sure that the SIM information have been read before doing the migration...

[1] https://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/ContactDB.jsm#349
[2] https://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/ContactDB.jsm#893

Or....

we could just remove the substringMatching pref being different depending on the country. How about this :

* in the parsedTel property, we always push the substringed value
* in findWithIndex's "match" code, we first look up the normalized value, and only if we find nothing, we look up the sliced value.

I understand this is more difficult because of the way the file is programmed (with the newTxn method, etc). But I think this is what will make the API robust.

Keeping the substringMatching pref dependant on the country will expose us to strange bugs, for example changing the SIM in a different country will lead to different results. Not to mention the init problem that is IMHO quite difficult to solve properly.
Comment 5 Gregor Wagner [:gwagner] 2013-07-09 05:53:18 PDT
(In reply to Julien Wajsberg [:julienw] from comment #4)
> I think there are several problems that each belong to different bugs.
> 
> 1 - the substringMatching is always enabled
> -> this happens because we're trying to get the country before the SIM
> information was even read (in [1]), therefore the default country is used,
> that is Brazil.

This only happens during the first startup after a fresh flash. Every consecutive startup will use the lastKnownMcc pref and this should be FR in your case.

> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/
> ContactService.jsm#46
> 
> 2 - migration issue when the substringMatching is enabled
> -> the migration code does not use the substringMatching pref to generate
> the various values pushed to the "parsedTel" property (see [1])
> However, the lookup code uses it (see [2]), and therefore, does not match
> anything.
> But of course, you need to be sure that the SIM information have been read
> before doing the migration...



> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/
> ContactDB.jsm#349
> [2]
> https://mxr.mozilla.org/mozilla-central/source/dom/contacts/fallback/
> ContactDB.jsm#893
> 
> Or....
> 
> we could just remove the substringMatching pref being different depending on
> the country. How about this :
> 
> * in the parsedTel property, we always push the substringed value
> * in findWithIndex's "match" code, we first look up the normalized value,
> and only if we find nothing, we look up the sliced value.
> 
> I understand this is more difficult because of the way the file is
> programmed (with the newTxn method, etc). But I think this is what will make
> the API robust.
> 
> Keeping the substringMatching pref dependant on the country will expose us
> to strange bugs, for example changing the SIM in a different country will
> lead to different results. Not to mention the init problem that is IMHO
> quite difficult to solve properly.

The substring matching is very country dependent. The number of matching digits varies greatly and we would need a separate pref for every single country. We shouldn't enable it by default.

So the only bug I see right now is that for the very first startup after a fresh flash we use BR as the default country name because we don't have the information from the lastKnownMCC. Or do you see BR as your country during ContactService initialization all the time?
Comment 6 Julien Wajsberg [:julienw] 2013-07-09 07:10:29 PDT
(In reply to Gregor Wagner [:gwagner] from comment #5)
> (In reply to Julien Wajsberg [:julienw] from comment #4)
> > I think there are several problems that each belong to different bugs.
> > 
> > 1 - the substringMatching is always enabled
> > -> this happens because we're trying to get the country before the SIM
> > information was even read (in [1]), therefore the default country is used,
> > that is Brazil.
> 
> This only happens during the first startup after a fresh flash. Every
> consecutive startup will use the lastKnownMcc pref and this should be FR in
> your case.

Well, this is not.

Plus, even if that would be the case, the migration code usually runs after a fresh flash, and will therefore be affected by the fresh-flash problem.

> The substring matching is very country dependent. The number of matching
> digits varies greatly and we would need a separate pref for every single
> country. We shouldn't enable it by default.

could you define "greatly" ? I thought 7 digits was enough for all situations. But I understand that we might not want to do this even if 7 digits is less "enough for everybody", because this might find false positives.

> 
> So the only bug I see right now is that for the very first startup after a
> fresh flash we use BR as the default country name because we don't have the
> information from the lastKnownMCC. Or do you see BR as your country during
> ContactService initialization all the time?

Yes I see this after a reboot each time I reboot.

And you still missed the other bug (my bug number 2) that _will_ happen for brazilian users that really use the substring matching: the substringed value is not in the index after a migration.
Comment 7 Michael Henretty [:mhenretty] 2013-07-09 08:21:08 PDT
(In reply to Julien Wajsberg [:julienw] from comment #6)
> > So the only bug I see right now is that for the very first startup after a
> > fresh flash we use BR as the default country name because we don't have the
> > information from the lastKnownMCC. Or do you see BR as your country during
> > ContactService initialization all the time?
> 
> Yes I see this after a reboot each time I reboot.

This is my fault because I told Gregor we were only seeing it on first flash. I thought, Julien, that when we were testing we only saw the Brazil mcc get set in the Phone Utils (the logs you showed me) after the initial flash, but that the original install we were playing with didn't show these log lines and had the appropriate mcc? Sorry about the confusion.

In any case, I agree with you that this needs to be fixed in two places:
1.) Make sure Phone Utils gets the appropriate mcc after the SIM get's read (also we should set lastKnownMcc here).
2.) The upgrade path needs to use the substringMatch settings (based on mcc) when building the search index.

One of these fixes without the other will cause problems, and so I agree these both belong in the same bug.
Comment 8 Gregor Wagner [:gwagner] 2013-07-09 18:41:54 PDT
Created attachment 773011 [details] [diff] [review]
WiP

LastKnownMCC is not ready here. We need something like this.
Comment 9 Gregor Wagner [:gwagner] 2013-07-09 18:42:44 PDT
Created attachment 773012 [details] [diff] [review]
patch

and now for real
Comment 10 Michael Henretty [:mhenretty] 2013-07-16 09:15:41 PDT
Created attachment 776455 [details] [diff] [review]
Add lastKnownSimMcc
Comment 11 Michael Henretty [:mhenretty] 2013-07-16 09:28:59 PDT
Created attachment 776467 [details] [diff] [review]
Add lastKnownSimMcc

Removing a debug line from the last patch.
Comment 12 Michael Henretty [:mhenretty] 2013-07-16 14:48:10 PDT
Created attachment 776716 [details] [diff] [review]
Add lastKnownSimMcc

Fix to when error when trying to fetch unset pref "ril.lastKnownSimMcc"
Comment 13 Michael Henretty [:mhenretty] 2013-07-16 17:38:57 PDT
Created attachment 776826 [details] [diff] [review]
Patch lastKnownSimMcc

Fixed upgrade path per gwagner's review.
Comment 14 Michael Henretty [:mhenretty] 2013-07-16 18:07:10 PDT
Created attachment 776843 [details] [diff] [review]
Patch lastKnownSimMcc

removing possible duplicates, correctly skipping upgrade step when no substring setting, turning off debug
Comment 15 Gregor Wagner [:gwagner] 2013-07-16 22:43:51 PDT
You also have to update:
http://mxr.mozilla.org/mozilla-central/source/dom/network/tests/marionette/test_mobile_networks.js
Line 39 and 48
Comment 16 Michael Henretty [:mhenretty] 2013-07-17 17:04:45 PDT
Created attachment 777485 [details] [diff] [review]
Patch lastKnownSimMcc

updating network tests to remove lastKnownSim
Comment 17 Gregor Wagner [:gwagner] 2013-07-17 20:26:13 PDT
Comment on attachment 777485 [details] [diff] [review]
Patch lastKnownSimMcc

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

\o/
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-07-18 13:18:21 PDT
https://hg.mozilla.org/projects/birch/rev/96fbc8fbf627

Please make sure that future patches have all the necessary commit information in them (including a commit message that describes what the patch is doing) before requesting checkin.
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-07-19 06:56:39 PDT
https://hg.mozilla.org/mozilla-central/rev/96fbc8fbf627
Comment 20 Ryan VanderMeulen [:RyanVM] 2013-07-19 07:23:21 PDT
Needs a branch-specific patch for uplift.
Comment 21 (Inactive after June) George Duan [:gduan] [:喬智] 2013-07-19 23:52:31 PDT
*** Bug 894739 has been marked as a duplicate of this bug. ***
Comment 22 Michael Henretty [:mhenretty] 2013-07-22 10:04:58 PDT
Created attachment 779264 [details] [diff] [review]
B2G-18 Patch

Here is the correlating patch for B2G-18. I'm still having trouble building this due to a configuration problem on my local machine (unrelated to this patch). I am working to get this resolved today.
Comment 23 Michael Henretty [:mhenretty] 2013-07-22 11:33:17 PDT
Comment on attachment 779264 [details] [diff] [review]
B2G-18 Patch

I was able to build and run the contacts tests for this patch on b2g-18. It's ready for review.
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-07-23 09:42:52 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/05c4e9a96d4a
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-07-23 14:08:39 PDT
Backed out for mass B2G test bustage. That was a fun all-day tree closure.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/126a53f59132

https://tbpl.mozilla.org/php/getParsedLog.php?id=25623532&tree=Mozilla-B2g18
Comment 26 Gregor Wagner [:gwagner] 2013-07-23 23:42:32 PDT
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> Backed out for mass B2G test bustage. That was a fun all-day tree closure.
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/126a53f59132
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=25623532&tree=Mozilla-B2g18

Well try server doesn't work at all for b2g18 so the b2g18 branch is our try server. We don't have another choice.

https://tbpl.mozilla.org/?tree=Try&rev=f30f310cf822
Comment 27 Ryan VanderMeulen [:RyanVM] 2013-07-24 05:51:49 PDT
You can always ping in #ateam for running the tests locally.
Comment 28 Julien Wajsberg [:julienw] 2013-07-26 04:07:31 PDT
So, what's left to do here ? Gregor, can I help ?
Comment 29 Michael Henretty [:mhenretty] 2013-07-26 09:18:17 PDT
Julien, we need to run the b2g reftests on a b2g emulator build manually to ensure they don't burn the b2g-18 tree. I'm working on that now.
Comment 30 Michael Henretty [:mhenretty] 2013-07-26 11:18:46 PDT
Created attachment 781823 [details] [diff] [review]
Updated Patch for B2G-18

I manually ran the reftests against the B2G emulator built on the TRY server with this patch. Tests got about 6% complete, which jgriffin said should be enough to show they won't burn in TBPL. Pending review, I believe this is ready for uplift.


REFTEST TEST-START | http://172.16.140.128:8888/tests/image/test/reftest/ico/ico-bmp-24bpp/ico-size-3x3-24bpp.png | 376 / 5736 (6%)
...
REFTEST TEST-PASS | http://172.16.140.128:8888/tests/image/test/reftest/ico/ico-bmp-24bpp/ico-size-3x3-24bpp.ico | image comparison (==)
Comment 31 Gregor Wagner [:gwagner] 2013-07-28 09:43:17 PDT
Comment on attachment 781823 [details] [diff] [review]
Updated Patch for B2G-18

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

::: dom/phonenumberutils/PhoneNumberUtils.jsm
@@ +38,5 @@
>      // Get network mcc
> +    let voice = mobileConnection.voiceConnectionInfo;
> +    if (voice && voice.network && voice.network.mcc) {
> +      mcc = voice.network.mcc;
> +     }

indentation is still wrong

@@ +44,5 @@
>      // Get SIM mcc
> +    let iccInfo = mobileConnection.iccInfo;
> +    if (!mcc && iccInfo.mcc) {
> +      mcc = iccInfo.mcc;
> +     }

same here

@@ +51,5 @@
>      if (!mcc) {
> +      try {
> +        mcc = Services.prefs.getCharPref("ril.lastKnownSimMcc");
> +      } catch (e) {}
> +     }

and here
Comment 32 Michael Henretty [:mhenretty] 2013-07-28 17:06:56 PDT
Created attachment 782329 [details] [diff] [review]
lastknownsimmcc.patch

fixed indentation
Comment 33 Gregor Wagner [:gwagner] 2013-07-28 19:19:09 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/8135299f3efd
Comment 35 Candice Serran (:cserran) 2013-07-31 05:41:40 PDT
Michael Henretty changed story state to started in Pivotal Tracker
Comment 36 Candice Serran (:cserran) 2013-07-31 05:41:44 PDT
Michael Henretty changed story state to accepted in Pivotal Tracker
Comment 37 Candice Serran (:cserran) 2013-07-31 05:41:49 PDT
Michael Henretty changed story state to accepted in Pivotal Tracker
Comment 38 Candice Serran (:cserran) 2013-07-31 05:41:55 PDT
Michael Henretty changed story state to accepted in Pivotal Tracker

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