869615 - Sanitize Contact properties everywhere, not just in init()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Bartosz Piec]

Attachment: Contacts order screenshot

Currently contacts sorting sucks. See the attached screenshot and look at the order. "Andrzej" definitely should be before "Artur" and after "Adrian"…

Unagi, v1.0.1

Comment 1

[Gregor Wagner [:gwagner]]

(In reply to Bartosz Piec from comment #0)
> Created attachment 746574 [details]
> Contacts order screenshot
> 
> Currently contacts sorting sucks. See the attached screenshot and look at
> the order. "Andrzej" definitely should be before "Artur" and after "Adrian"…
> 
> Unagi, v1.0.1

Does this also happen if you restart the contacts app? We have a known bug for inserting.

Comment 2

[Bartosz Piec]

Yes, it happens all the time, I don't even remember if it has been working correct ever.

Note that I have ~350 contacts.

Comment 3

[Gregor Wagner [:gwagner]]

How did you import your contacts?
Manually, sim card, facebook, ...?

Comment 4

[Bartosz Piec]

Gmail (in v1-train, than switched back to v1.0.1).

Comment 5

[Bartosz Piec]

I can send you my contacts if you tell me how can I do this and promise that you will delete them after the tests :)

Comment 6

[Reuben Morais [:reuben]]

Hey Bartosz, for the contacts that are out of order, if you go into the details page and edit them, do they have Name and Last Names, or just a Company name?

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to Bartosz Piec from comment #4)
> Gmail (in v1-train, than switched back to v1.0.1).

There is a reason why we don't have the Google import in v1.0.1. We would have to uplift additional patches. So your setup isn't fully supported.

Comment 8

[Bartosz Piec]

(In reply to Reuben Morais [:reuben] from comment #6)
> Hey Bartosz, for the contacts that are out of order, if you go into the
> details page and edit them, do they have Name and Last Names, or just a
> Company name?

They have first and last name.

(In reply to Gregor Wagner [:gwagner] from comment #7)
> There is a reason why we don't have the Google import in v1.0.1. We would
> have to uplift additional patches. So your setup isn't fully supported.

I can test it on pure v1.0.1 but I need to import my contacts in any way. Is any other way to import large contacts number from Android?

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

@Bartosz, if you want to try in v1.0.1 and import the contacts from Android you can install the stand alone application for doing this:

https://github.com/arcturus/firefoxos-contacts-importer

You can push to the device using the firefox simulator.
As well you could import from facebook, that is present in v1.0.1

Cheers,
F.

Comment 10

[Gregor Wagner [:gwagner]]

We tried with 300 random google contacts and the sorting worked fine.
If you still see a random sort order after a re-import and not switching branches I would like to take a look at your contactsDB.

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

Closing as works for me, please reopen if you are able to reproduce again.

Thanks a lot,
F.

Comment 12

[Bartosz Piec]

(In reply to Francisco Jordano [:arcturus] from comment #9)
> @Bartosz, if you want to try in v1.0.1 and import the contacts from Android
> you can install the stand alone application for doing this:
> 
> https://github.com/arcturus/firefoxos-contacts-importer
> 
> You can push to the device using the firefox simulator.

Can you tell me (or point to some resource) how to push the contacts from the simulator to the device or how to add this app to gaia before flashing?


> As well you could import from facebook, that is present in v1.0.1

Obviously I don't have all of my contacts on Facebook (along with their phone numbers).

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Hi 

(In reply to Bartosz Piec from comment #12)
> (In reply to Francisco Jordano [:arcturus] from comment #9)
> > @Bartosz, if you want to try in v1.0.1 and import the contacts from Android
> > you can install the stand alone application for doing this:
> > 
> > https://github.com/arcturus/firefoxos-contacts-importer
> > 
> > You can push to the device using the firefox simulator.
> 
> Can you tell me (or point to some resource) how to push the contacts from
> the simulator to the device or how to add this app to gaia before flashing?
> 

Sorry, you are right, I wasn't being really clear on my explanation, you push the application that allows you to import from gmail.

Thanks.

Comment 14

[Bartosz Piec]

(In reply to Francisco Jordano [:arcturus] from comment #13)
> Sorry, you are right, I wasn't being really clear on my explanation, you
> push the application that allows you to import from gmail.

Ok, but can you point me to a resource explaining on how to do this?

Btw, I just switched to v1-train branch, imported my contacts from Gmail and the sorting is still broken. Does that confirm this bug?

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

Still not able to reproduce, but reopening and asking QA to verify this.

Thanks!

Comment 16

[Jens Bannmann]

I can reproduce this my Peak (1.0.1.0-prerelease 20130418124521).

The order I see each time starting the contacts app is:
 - Arthur
 - André
 - Antje
 - Arnd
 - Anja

All names above are the given names; all contacts also have a family name. The "name" property is also filled with the combination of Given+Family name. I imported these manually with a script using the mozContacts API.

If you tell me how, I can send my DB as-is to a developer. Alternatively, I can send my importer app including the data.

Comment 17

[Jason Smith [:jsmith]]

Somebody needs to test the STR given in comment 16 are a more recent build to confirm this.

Comment 18

[alberto.pastor]

I just tested SRT on comment 16 with a recent build (on gaia master/v1-train/v1.0.1) and not able to reproduce. The order I see is:

- André
- Anja
- Antje
- Arnd
- Arthur

Does it happen the same if you create those contacts manually? I'm wondering if it has something to do with the importer. Could you post that script as well?
Thanks!

Comment 19

[Jens Bannmann]

Gregor, did you already have a chance to look into the database dump I sent to you?

Comment 20

[Jens Bannmann]

Attachment: example batch import algorithm, parallel and sequential variants

(In reply to Alberto Pastor [:albertopq] from comment #18)
> Does it happen the same if you create those contacts manually? I'm wondering
> if it has something to do with the importer. Could you post that script as
> well?

The script I originally used took advantage of the asynchronous nature of the contacts API and started the requests for each of my ~170 contacts in one go.

I suspected that the bug might be caused by a lack of thread-safety (or similar issue) somewhere, so I went ahead and imported the same data using a strictly sequential algorithm. It turned out that the contacts sorting is completely correct using the latter approach!

While I don't know whether this behavior caused the original reporter's problem, I think it's very likely. Adding so many contacts at once probably just increases the chance of the bug manifesting itself.

For reference, I attached both algorithms. I guess one can create a test case using synthesized data.

Comment 21

[Jens Bannmann]

Oh, one thing I forgot to mention: I could reproduce the behaviour of sequential (okay) and parallel (broken) import variants in Firefox OS simulator using my data set each and every time.

Comment 22

[Reuben Morais [:reuben]]

Thanks for the test case, Jens. Your guess is probably right, IndexedDB is hard and we could be doing many things wrong in this case. Gregor is out this week, so I'm taking this one.

Comment 23

[Reuben Morais [:reuben]]

So what's going on is that somehow in your DB, the givenName/familyName properties were saved as strings instead of arrays, which deeply confuses our search code. I'm looking into some weirdness that I found while looking at this, but in the mean time, could you try clearing your database, then re-running the import script on a newer version of Gecko?

Comment 24

[Reuben Morais [:reuben]]

The weirdness was bug 877777, FWIW.

Comment 25

[Bartosz Piec]

(In reply to Reuben Morais [:reuben] from comment #23)
> So what's going on is that somehow in your DB, the givenName/familyName
> properties were saved as strings instead of arrays, which deeply confuses
> our search code. I'm looking into some weirdness that I found while looking
> at this, but in the mean time, could you try clearing your database, then
> re-running the import script on a newer version of Gecko?

So the Gmail import is also affected by that as my contacts have been imported that way.

Comment 26

[Reuben Morais [:reuben]]

(In reply to Bartosz Piec from comment #25)
> So the Gmail import is also affected by that as my contacts have been
> imported that way.

How exactly did you import them? Can you post the full importer code? That shouldn't ever happen unless you call directly into ContactDB or write the sqlite file yourself.

Comment 27

[Bartosz Piec]

(In reply to Reuben Morais [:reuben] from comment #26)
> How exactly did you import them? Can you post the full importer code? That
> shouldn't ever happen unless you call directly into ContactDB or write the
> sqlite file yourself.

Via the built-in gmail import app (see #c4).

Comment 28

[Bartosz Piec]

I've done this again, this time using https://github.com/arcturus/firefoxos-contacts-importer and pushing it via Simulator. Everything was done using pure 1.0.1 on factory-default Keon.

The sorting is still broken (order by first name):
‒ Artur W***
‒ Aneta L***
‒ Asia W***

Comment 29

[Jens Bannmann]

Attachment: code constructing mozContact instances

(In reply to Reuben Morais [:reuben] from comment #23)
> So what's going on is that somehow in your DB, the givenName/familyName
> properties were saved as strings instead of arrays, which deeply confuses
> our search code.

Actually, my code sets these properties to strings (see attachment). The reason is that due to a lack of documentation, I had to figure out the details of using the Contacts API by myself. When I looked at nsIContactProperties.idl, I carelessly ignored the array notation in the comments (e.g. <http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/contacts/nsIContactProperties.idl#54>), and because the import seemed to work fine, I never thought about it again.

This leads me to two remarks:
1) If these properties are supposed to be arrays of strings, why does passing scalar strings not produce any error?
2) The string-vs-array problem does not explain why importing in parallel fashion leads to broken sorting while sequential importing does not.

> I'm looking into some weirdness that I found while looking
> at this, but in the mean time, could you try clearing your database, then
> re-running the import script on a newer version of Gecko?

I did not manage to successfully build Gecko for my Peak yet. Does the current Firefox OS Simulator build (1.1.0.0-prerelease, build 20130426094324) use a newer Gecko? I can reproduce the problem there.

Comment 30

[Reuben Morais [:reuben]]

(In reply to Jens Bannmann from comment #29)
> Actually, my code sets these properties to strings (see attachment). The
> reason is that due to a lack of documentation, I had to figure out the
> details of using the Contacts API by myself. When I looked at
> nsIContactProperties.idl, I carelessly ignored the array notation in the
> comments (e.g.
> <http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/contacts/
> nsIContactProperties.idl#54>), and because the import seemed to work fine, I
> never thought about it again.
>
> This leads me to two remarks:
> 1) If these properties are supposed to be arrays of strings, why does
> passing scalar strings not produce any error?

A-ha! That's a bug in our code, yes :(
So if you call mozContact.init(properties), and properties.givenName/familyName/etc are strings, the code will fix everything as best as it can, including turning those things into arrays. The bug here is that instead of implementing the fixups in property setters, we only do that in the init method, so code like yours fails silently.

> 2) The string-vs-array problem does not explain why importing in parallel
> fashion leads to broken sorting while sequential importing does not.

That seems like a different bug. Let's fix the one we identified first and then see if that one still happens.

> > I'm looking into some weirdness that I found while looking
> > at this, but in the mean time, could you try clearing your database, then
> > re-running the import script on a newer version of Gecko?
> 
> I did not manage to successfully build Gecko for my Peak yet. Does the
> current Firefox OS Simulator build (1.1.0.0-prerelease, build
> 20130426094324) use a newer Gecko? I can reproduce the problem there.

I think so, since the Peak build is from 2013-04-18.

Comment 31

[Reuben Morais [:reuben]]

Attachment: Move sanitization code to individual setters

Comment 32

[Reuben Morais [:reuben]]

Attachment: Moar tests

Comment 33

[Bartosz Piec]

Will that fix contacts imported from Gmail? Gmail importer is in marketplace now (https://marketplace.firefox.com/app/importer) so it's more important.

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 756221 [details] [diff] [review]
Move sanitization code to individual setters

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

::: dom/contacts/ContactManager.js
@@ +325,5 @@
> +  get category() {
> +    return this._category;
> +  },
> +
> +  validateArrayField: function (data, field, name, createCb) {

Problems here:

1. what if data is null/undefined?
2. debug message is weird, "Not a plain object"?
3. Why not make it return an array so it doesn't have to mess with 'this'

@@ +420,5 @@
> +
> +  set bday(aBday) {
> +    if (aBday !== undefined && aBday !== null) {
> +      this._bday = new Date(aBday);
> +    }

What about if it is null/undefined? Same for other dates

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 756226 [details] [diff] [review]
Moar tests

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

rs=me on more tests, provided that they work ;)

Comment 36

[Reuben Morais [:reuben]]

(In reply to ben turner [:bent] from comment #34)
> ::: dom/contacts/ContactManager.js
> @@ +325,5 @@
> > +  get category() {
> > +    return this._category;
> > +  },
> > +
> > +  validateArrayField: function (data, field, name, createCb) {
> 
> Problems here:
> 
> 1. what if data is null/undefined?

Then the getter returns undefined. This is expected.

> 2. debug message is weird, "Not a plain object"?

Only happens when someone passes, say, a HTMLElement instead of an object.

> 3. Why not make it return an array so it doesn't have to mess with 'this'

Because it doesn't know how to create the ContactField/ContactTelField/ContactAddress interfaces, and I didn't want to return an array that is then iterated again in the setter. I can do that if you think that's better.

> @@ +420,5 @@
> > +
> > +  set bday(aBday) {
> > +    if (aBday !== undefined && aBday !== null) {
> > +      this._bday = new Date(aBday);
> > +    }
> 
> What about if it is null/undefined? Same for other dates

Then the getter returns undefined. This is expected.

Comment 37

[Reuben Morais [:reuben]]

Attachment: Move sanitization code to individual setters, v2

With changes to validateArrayField as discussed on IRC.

Comment 38

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 756820 [details] [diff] [review]
Move sanitization code to individual setters, v2

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

r=me with this:

::: dom/contacts/ContactManager.js
@@ +223,5 @@
> +function isVanillaObj(aObj) {
> +  return Object.prototype.toString.call(aObj) == "[object Object]";
> +}
> +
> +function validateArrayField(data, createCb) {

You need to return null/undefined if !data, otherwise this generates a warning

Comment 39

[Reuben Morais [:reuben]]

Thanks!

https://hg.mozilla.org/projects/birch/rev/a608b5d810e6

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a608b5d810e6

Comment 41

[Wayne Chang [:wchang]]

Triage - blocking for common use in target markets.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Needs a patch for b2g18 uplift.

Comment 43

[Reuben Morais [:reuben]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/94f2c4aad8c7

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/94f2c4aad8c7

Comment 45

[:CSchmoeckel]

Added Contacts Suite Test Case #8983 - Contact entries created in the Contacts App are sorted alphabetically