890909 - [Contacts API] migration code problem when matching using the substringMatching pref (eg in Brazil)

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

+++ 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]]

See bug 883770 for background, this is a follow-up. Especially see bug 883770 comment 34.

Comment 2

[Michael Henretty [:mikehenrty][:mhenretty]]

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][⌚️UTC+2]]

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]]

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]]

(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]]

(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 [:mikehenrty][:mhenretty]]

(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]]

Attachment: WiP

LastKnownMCC is not ready here. We need something like this.

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: patch

and now for real

Comment 10

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Add lastKnownSimMcc

Comment 11

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Add lastKnownSimMcc

Removing a debug line from the last patch.

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Add lastKnownSimMcc

Fix to when error when trying to fetch unset pref "ril.lastKnownSimMcc"

Comment 13

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Patch lastKnownSimMcc

Fixed upgrade path per gwagner's review.

Comment 14

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Patch lastKnownSimMcc

removing possible duplicates, correctly skipping upgrade step when no substring setting, turning off debug

Comment 15

[Gregor Wagner [:gwagner]]

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 [:mikehenrty][:mhenretty]]

Attachment: Patch lastKnownSimMcc

updating network tests to remove lastKnownSim

Comment 17

[Gregor Wagner [:gwagner]]

Comment on attachment 777485 [details] [diff] [review]
Patch lastKnownSimMcc

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

\o/

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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]]

https://hg.mozilla.org/mozilla-central/rev/96fbc8fbf627

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 22

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: 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 [:mikehenrty][:mhenretty]]

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]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/05c4e9a96d4a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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]]

(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]]

You can always ping in #ateam for running the tests locally.

Comment 28

[Julien Wajsberg [:julienw]]

So, what's left to do here ? Gregor, can I help ?

Comment 29

[Michael Henretty [:mikehenrty][:mhenretty]]

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 [:mikehenrty][:mhenretty]]

Attachment: 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]]

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 [:mikehenrty][:mhenretty]]

Attachment: lastknownsimmcc.patch

fixed indentation

Comment 33

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8135299f3efd

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/8135299f3efd

Comment 35

[Candice Serran (:cserran)]

Michael Henretty changed story state to started in Pivotal Tracker

Comment 36

[Candice Serran (:cserran)]

Michael Henretty changed story state to accepted in Pivotal Tracker

Comment 37

[Candice Serran (:cserran)]

Michael Henretty changed story state to accepted in Pivotal Tracker

Comment 38

[Candice Serran (:cserran)]

Michael Henretty changed story state to accepted in Pivotal Tracker