Closed Bug 929652 Opened 11 years ago Closed 11 years ago

webidl mozContacts fails to convert some reference workload images correctly

Categories

(Core Graveyard :: DOM: Contacts, defect)

Product:
Core Graveyard
Component:
DOM: Contacts
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla28

People

(Reporter: bkelly, Assigned: julienw)

References

Details

Attachments

(1 file, 4 obsolete files)

patch v5
(deleted), patch
reuben
: review+
Details | Diff | Splinter Review 
Currently its not possible to load the reference-workload-medium dataset.  Steps to reproduce:

  1) cd gaia && make reference-workload-medium
  2) Launch contacts app
  3) Wait for database upgrade to occur.

Observe the following in log:

  E/GeckoConsole( 2086): [JavaScript Error: "Element of 'photo' member of ContactProperties is not an object." {file: "jar:file:///system/b2g/omni.ja!/components/ContactManager.js" line: 397}]

This is in _convertContact().

Note this does NOT occur with reference-workload-light which also has images.  It seems to be a problem with a particular contact.
I added some debug and in the reference-workload-medium dataset the problem contact appears to be:

I/Gecko   ( 3765): ### ### BAD: {"properties":{"name":["Marty M. Peterson"],"honorificPrefix":[],"givenName":["Marty"],"additionalName":["M."],"familyName":["Peterson"],"honorificSuffix":[],"nickname":[],"email":[{"value":"MartyMPeterson@dodgit.com","type":[""]}],"photo":[null],"url":[],"category":[],"adr":[{"countryName":"Norway","locality":"RÅDAL","postalCode":"5239","region":"","streetAddress":"Snekkevikvegen 58","type":[null]}],"tel":[{"value":"48545516","type":[""]}],"org":["Present Company"],"jobTitle":["Computer hardware engineer"],"bday":"1965-09-22T00:00:00.000Z","note":[],"impp":[],"anniversary":null,"sex":null,"genderIdentity":null},"updated":"2013-07-30T19:32:06.244Z","published":"2013-07-30T19:32:06.244Z","id":"849e39d737ee49f7a053aeae43450697"}

Which has [null] for its photo property.
I'm currently working around this problem with this patch:

   _convertContact: function(aContact) {
+    if (Array.isArray(aContact.properties.photo) && aContact.properties.photo.length && !aContact.properties.photo[0]) {
+      dump("### ### FIX BAD CONTACT\n");
+      aContact.properties.photo = [];
+    }
+
     let newContact = new this._window.mozContact(aContact.properties);
     newContact.setMetadata(aContact.id, aContact.published, aContact.updated);
     return newContact;
   },
Component: DOM: Device Interfaces → DOM: Contacts
Blocks: 850430
Hey Ben,

do you know if the photo property was [null] before the WebIDL upgrade path runs ?
ok, for some reason, the photo property was [null] before the upgrade run.
Attached patch patch v1 (obsolete) (deleted) — — Splinter Review 
This fixes the problem for me.
Assignee: nobody → felash
Attachment #822832 - Flags: review?(anygregor)
Attachment #822832 - Flags: review?(reuben.bmo)
Comment on attachment 822832 [details] [diff] [review]
patch v1

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

Thanks for the patch, r=me with these fixed.

::: dom/contacts/fallback/ContactDB.jsm
@@ +567,5 @@
>            let cursor = event.target.result;
>            let changed = false;
>            if (cursor) {
>              let props = cursor.value.properties;
> +

nit: whitespace.

@@ +597,5 @@
>        },
>        function upgrade15to16() {
>          if (DEBUG) debug("Fix Date properties");
>          if (!objectStore) {
> +          objectStore = aTransaction.objectStore(STORE_NAME);

Thanks for fixing these.

@@ +647,5 @@
> +          if (cursor) {
> +            let props = cursor.value.properties;
> +
> +            for (let prop of ARRAY_PROPERTIES) {
> +	            if (props[prop] && props[prop].length) {

Please use Array.isArray and fix the indentation here

@@ +657,5 @@
> +
> +                if (PROPERTIES_WITH_TYPE.indexOf(prop) !== -1) {
> +                  let subprop = props[prop];
> +                  for (let i = 0; i < subprop.length; ++i) {
> +                    subprop[i].type = subprop[i].type.filter(filterInvalidValues);

You should probably do the same check you did above to replace empty arrays with null here, for consistency.
Attachment #822832 - Flags: review?(reuben.bmo) → review+
(In reply to Reuben Morais [:reuben] from comment #7)
> Comment on attachment 822832 [details] [diff] [review]
> patch v1
> 
> Review of attachment 822832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch, r=me with these fixed.
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +567,5 @@
> >            let cursor = event.target.result;
> >            let changed = false;
> >            if (cursor) {
> >              let props = cursor.value.properties;
> > +
> 
> nit: whitespace.

Question: Just to clarify, do you want that I remove this empty line? We're not supposed to add whitespace on code we don't touch? This one won't disturb the blame.

> 
> @@ +647,5 @@
> > +          if (cursor) {
> > +            let props = cursor.value.properties;
> > +
> > +            for (let prop of ARRAY_PROPERTIES) {
> > +	            if (props[prop] && props[prop].length) {
> 
> Please use Array.isArray

Actually, this was not a check to know if this is an array. We already know that either it's absent, either it's an array. So here I was simply checking if 1. the property is used and 2. the array is not empty.

> @@ +657,5 @@
> > +
> > +                if (PROPERTIES_WITH_TYPE.indexOf(prop) !== -1) {
> > +                  let subprop = props[prop];
> > +                  for (let i = 0; i < subprop.length; ++i) {
> > +                    subprop[i].type = subprop[i].type.filter(filterInvalidValues);
> 
> You should probably do the same check you did above to replace empty arrays
> with null here, for consistency.

Mmm is a "null" type valid? I would say an empty array is more valid but I may be wrong...
If a "null" type is valid, then the `upgrade14to15` can fail when we get the `length` and I should fix this there too, right ?
Flags: needinfo?(reuben.bmo)
blocking-b2g: --- → koi?
(In reply to Julien Wajsberg [:julienw] from comment #8)
> > nit: whitespace.
> 
> Question: Just to clarify, do you want that I remove this empty line? We're
> not supposed to add white-space on code we don't touch? This one won't
> disturb the blame.

It's generally recommended not to change blame with whitespace fixes unless you're changing the code around it already. I don't feel strongly about this one since it's just adding a line.

> > @@ +647,5 @@
> > > +          if (cursor) {
> > > +            let props = cursor.value.properties;
> > > +
> > > +            for (let prop of ARRAY_PROPERTIES) {
> > > +	            if (props[prop] && props[prop].length) {
> > 
> > Please use Array.isArray
> 
> Actually, this was not a check to know if this is an array. We already know
> that either it's absent, either it's an array. So here I was simply checking
> if 1. the property is used and 2. the array is not empty.

Ok, I missed the fact that the length != 0 check was intended there, it wasn't just length != undefined.

> > @@ +657,5 @@
> > > +
> > > +                if (PROPERTIES_WITH_TYPE.indexOf(prop) !== -1) {
> > > +                  let subprop = props[prop];
> > > +                  for (let i = 0; i < subprop.length; ++i) {
> > > +                    subprop[i].type = subprop[i].type.filter(filterInvalidValues);
> > 
> > You should probably do the same check you did above to replace empty arrays
> > with null here, for consistency.
> 
> Mmm is a "null" type valid? I would say an empty array is more valid but I
> may be wrong...

Yes, both null and empty array are valid values for the type property.

> If a "null" type is valid, then the `upgrade14to15` can fail when we get the
> `length` and I should fix this there too, right ?

We only access length of eg. |adr|, not |adr[n].type|.
Flags: needinfo?(reuben.bmo)
Renom to 1.3? since webidl changes won't be uplifted to 1.2 branch
blocking-b2g: koi? → 1.3?
Noemi, the upgrade path from WebIDL (and only this part) will be uplifted as part of bug 898337, and therefore this will need to go there too. I think that's why Jose asked for koi here.
(In reply to Reuben Morais [:reuben] from comment #9)
> > If a "null" type is valid, then the `upgrade14to15` can fail when we get the
> > `length` and I should fix this there too, right ?
> 
> We only access length of eg. |adr|, not |adr[n].type|.

Ah right, I misread that code, thanks !
(In reply to Julien Wajsberg [:julienw] from comment #11)
> Noemi, the upgrade path from WebIDL (and only this part) will be uplifted as
> part of bug 898337, and therefore this will need to go there too. I think
> that's why Jose asked for koi here.


ok, then renom to koi? again. Thanks for the clarification.
blocking-b2g: 1.3? → koi?
Attached patch patch v2 (obsolete) (deleted) — — Splinter Review 
carrying r=reuben

fixed follow-ups, factorized the array filtering bit in its own function to make things clearer.

New try is https://tbpl.mozilla.org/?tree=Try&rev=10b7478162ad
And will test on device with the reference workload before asking checkin-needed anyway.
Attachment #822832 - Attachment is obsolete: true
Attachment #822832 - Flags: review?(anygregor)
Attachment #823871 - Flags: review+
A little bit of context to koi triagers: koi+ bug 989337 lands on central after the WebIDL change, therefore the safest way to handle a proper upgrade from 1.2 to 1.3 is to take every previous upgrade paths on the branch too (including WebIDL and this one), especially because these upgrade paths are only data sanitization and therefore should be fully compatible with the existing code.
Hi folks,

sorry for arriving late to the conversation, again, the WebIDL change for contacts app was a bit of overhead in terms of bugs that we are still solving. Bugs that were not even marked for koi? as long as the WebIDL change wasn't about to land.

Asking Ben Kelly for needinfo since he fixed several of them to add his opinion on the risk here.

Thanks a lot!
Flags: needinfo?(bkelly)
Francisco, we won't uplift all the WebIDL patch, but only the upgrade part in the contact DB.
Attached patch patch v3 (obsolete) (deleted) — — Splinter Review 
carrying r=reuben

patch v2 had a bad syntax exception, so here is the fix. Tried on the device with the workload and works fine for me. Will try also with my own contact db from 1.1 before landing.

New try is https://tbpl.mozilla.org/?tree=Try&rev=a19f4024beae
Attachment #823871 - Attachment is obsolete: true
Attachment #823943 - Flags: review+
(In reply to Julien Wajsberg [:julienw] from comment #17)
> Francisco, we won't uplift all the WebIDL patch, but only the upgrade part
> in the contact DB.

:)

Sorry for my ignorance, as long as the bad use of the api that we are doing in the contact app will be compatible ... I'll be a happy person.

Thanks a lot for all the effort, we know it's a pain in the ass and you are working hard for delivering what we need.

F.
Francisco: I'm fairly sure it will be, but I'll test before landing on aurora anyway, and QA will too, so if there is a sign of breakage we'll back out :)
Uplifting the database normalization sounds relatively safe to me.  I guess if there is some code somewhere expecting scalars, and only scalars, then it might get unexpectedly broken when values change to arrays.  As far as I can tell, however, all of our code consuming mozContacts handles arrays correctly.
Flags: needinfo?(bkelly)
So, I was gonna ask for a checkin here, but then I noticed my own contact db was not converted correctly, because of a name that was the empty string.

I changed my patch to account for this, but I think it doesn't work yet, so will finish this tomorrow, and will probably ask for another review from reuben then.
Attached patch patch v4 (obsolete) (deleted) — — Splinter Review 
Hey Reuben,

here is an updated patch. Since I changed it more than for nits, I think I'd like that you look at it.

I tried it with both the reference-workload-medium and with my own contact DB, and it fixes correctly all errors I saw. I also added some code to fix potential mistakes with the Date upgrade path (especially when we could have bad date strings).

I started writing a test file to test migrations, but it's not finished yet, and I don't want to wait too much here, so I'll file a new bug for this work and continue it there.

Here is the new try: https://tbpl.mozilla.org/?tree=Try&rev=c587a010a4e0
Attachment #823943 - Attachment is obsolete: true
Attachment #824661 - Flags: review?(reuben.bmo)
Can you test the other reference workloads as well?  I believe reference-workload-heavy and reference-workload-xheavy were also failing.

Also, I was thinking about the uplift last night.  Would it be possible to bump the version number and force an "upgrade" in 1.3 as well?  I worry about a third party app saving contacts in v1.2 without the strict formatting required by webidl.  This might cause some small percentage of phones to blow up when they start using v1.3 and their database is not correct.
Comment on attachment 824661 [details] [diff] [review]
patch v4

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

Looking good; I only have a question about the new code for fixing up Dates.

::: dom/contacts/fallback/ContactDB.jsm
@@ +647,5 @@
> +          function filteredArray(array) {
> +            array = array.filter(filterInvalidValues);
> +
> +            if (!array.length) {
> +              array = null;

No need to set to null here, empty arrays are valid and we don't want to transform |[]| to |null|.

@@ +673,5 @@
> +                props[prop] = filteredArray(props[prop]);
> +
> +                if (PROPERTIES_WITH_TYPE.indexOf(prop) !== -1) {
> +                  let subprop = props[prop];
> +                  if (!subprop) {

This check is redundant (after my comment to filteredArray is addressed), you already check |if (props[prop]…| above.

@@ +680,5 @@
> +
> +                  for (let i = 0; i < subprop.length; ++i) {
> +                    let curSubprop = subprop[i];
> +                    // in upgrade14to15 we forced this into an array whatever
> +                    // the value was

Please fix this cliffhanger :)

@@ +694,5 @@
> +              if (props[prop] != null) { // null or undefined is ok
> +                if (props[prop] instanceof Date) {
> +                  // props[prop] got converted in upgrade15to16
> +                  if (isNaN(props[prop].getTime())) {
> +                    // but props[prop] was incorrect in upgrade15to16

We only want to cleanup things that don't match the interface here. What benefit does removing Dates with NaN values give us?

@@ +699,5 @@
> +                    props[prop] = null;
> +                    changed = true;
> +                  }
> +                } else {
> +                  // props[prop] is '' and was not converted in upgrade15to16

Ouch. Good catch.
Attachment #824661 - Flags: review?(reuben.bmo)
(In reply to Ben Kelly [:bkelly] from comment #24)
> Can you test the other reference workloads as well?  I believe
> reference-workload-heavy and reference-workload-xheavy were also failing.
> 
> Also, I was thinking about the uplift last night.  Would it be possible to
> bump the version number and force an "upgrade" in 1.3 as well? 

That was one of my tries, I still have the patch.

> I worry
> about a third party app saving contacts in v1.2 without the strict
> formatting required by webidl.  This might cause some small percentage of
> phones to blow up when they start using v1.3 and their database is not
> correct.

Yep, this is a valid concern.

Oh god I'm going to dislike this.
(In reply to Reuben Morais [:reuben] from comment #25)
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +647,5 @@
> > +          function filteredArray(array) {
> > +            array = array.filter(filterInvalidValues);
> > +
> > +            if (!array.length) {
> > +              array = null;
> 
> No need to set to null here, empty arrays are valid and we don't want to
> transform |[]| to |null|.

For property values, since I first check if length is not null, we won't transform [] to null, but we will transform `[null]` to `null`. Do you think it's better to transform this to [] instead then?

> 
> @@ +673,5 @@
> > +                props[prop] = filteredArray(props[prop]);
> > +
> > +                if (PROPERTIES_WITH_TYPE.indexOf(prop) !== -1) {
> > +                  let subprop = props[prop];
> > +                  if (!subprop) {
> 
> This check is redundant (after my comment to filteredArray is addressed),
> you already check |if (props[prop]…| above.

Yep, if we don't set null, then I'll gladly remove it :)

> 
> @@ +680,5 @@
> > +
> > +                  for (let i = 0; i < subprop.length; ++i) {
> > +                    let curSubprop = subprop[i];
> > +                    // in upgrade14to15 we forced this into an array whatever
> > +                    // the value was
> 
> Please fix this cliffhanger :)

My vim is configured to stay inside the 80 char limit ;) I can put everything on a line though (will need a new .vimrc for my gecko work).

> 
> @@ +694,5 @@
> > +              if (props[prop] != null) { // null or undefined is ok
> > +                if (props[prop] instanceof Date) {
> > +                  // props[prop] got converted in upgrade15to16
> > +                  if (isNaN(props[prop].getTime())) {
> > +                    // but props[prop] was incorrect in upgrade15to16
> 
> We only want to cleanup things that don't match the interface here. What
> benefit does removing Dates with NaN values give us?

What benefit gives keeping them ?
But you're right, we don't do that at import anyway. Will remove this.

Reuben, I've been thinking something else too: should we try to convert all values that are not strings to strings ? I'm asking because in my patch for bug 898337 I call `toLowerCase` on values, and you asked me to remove the check that it is really a string.

Also, what do you think about comment 25 (although this is really a comment for bug 898337)?
Flags: needinfo?(reuben.bmo)
ok, we won't uplift bug 898337 after all. Life is good :-)
Ben: both workloads heavy and x-heavy work great :)
blocking-b2g: koi? → 1.3?
Blocks: 898337
(In reply to Julien Wajsberg [:julienw] from comment #27)
> (In reply to Reuben Morais [:reuben] from comment #25)
> > 
> > ::: dom/contacts/fallback/ContactDB.jsm
> > @@ +647,5 @@
> > > +          function filteredArray(array) {
> > > +            array = array.filter(filterInvalidValues);
> > > +
> > > +            if (!array.length) {
> > > +              array = null;
> > 
> > No need to set to null here, empty arrays are valid and we don't want to
> > transform |[]| to |null|.
> 
> For property values, since I first check if length is not null, we won't
> transform [] to null, but we will transform `[null]` to `null`. Do you think
> it's better to transform this to [] instead then?

But you don't check the length of |curSubprop.type| before filtering, so in that case [] will be transformed to null. And yes, I think it's better to do [null] -> []. We want to enforce validity of the layout while changing as little as we can.

> > @@ +680,5 @@
> > > +
> > > +                  for (let i = 0; i < subprop.length; ++i) {
> > > +                    let curSubprop = subprop[i];
> > > +                    // in upgrade14to15 we forced this into an array whatever
> > > +                    // the value was
> > 
> > Please fix this cliffhanger :)
> 
> My vim is configured to stay inside the 80 char limit ;) I can put
> everything on a line though (will need a new .vimrc for my gecko work).

I meant that the comment sounded incomplete, but now I understand what you mean. Maybe use something like:

// upgrade14to15 transformed |type| props to arrays without filtering invalid data.

> > @@ +694,5 @@
> > > +              if (props[prop] != null) { // null or undefined is ok
> > > +                if (props[prop] instanceof Date) {
> > > +                  // props[prop] got converted in upgrade15to16
> > > +                  if (isNaN(props[prop].getTime())) {
> > > +                    // but props[prop] was incorrect in upgrade15to16
> > 
> > We only want to cleanup things that don't match the interface here. What
> > benefit does removing Dates with NaN values give us?
> 
> What benefit gives keeping them ?

We avoid changing the data unless it's necessary. Who knows what kind of wicked consumers the API has, maybe some of them depend on NaN Dates :)

> Reuben, I've been thinking something else too: should we try to convert all
> values that are not strings to strings ? I'm asking because in my patch for
> bug 898337 I call `toLowerCase` on values, and you asked me to remove the
> check that it is really a string.

We've always sanitized string values in one way or another. Pre-WebIDL code had stringOrBust and older code did something along the lines of:

  if (typeof prop[i] !== "string") prop[i] = String(prop[i]);

> Also, what do you think about comment 25 (although this is really a comment
> for bug 898337)?

You mean comment 24? Like Ben said, uplifting just versions 15 and 16 without the other changes puts us in a tricky situation. We'd have to also run those migrations on v1.2 -> v1.3, and that's definitely doable, but we'd have to be very very careful; our upgrade code is confusing enough as it is without having to deal with upgrades that apply to more than one version.
Flags: needinfo?(reuben.bmo)
(In reply to Reuben Morais [:reuben] from comment #30)
> And yes, I think it's better to do [null] -> [].

Even better would be not changing anything in that case (photo is the only attribute where null values are technically incorrect), but given our experiences with existing API consumers in Gaia, I agree with the change.
Attached patch patch v5 (deleted) — — Splinter Review 
new try is https://tbpl.mozilla.org/?tree=Try&rev=1f9f14c33ccf

I'm building locally and I'll test on the various workloads before asking review, but I think I handled every review comments :)
Attachment #824661 - Attachment is obsolete: true
Comment on attachment 825178 [details] [diff] [review]
patch v5

This works for me on the various workloads I tried.

Asking review!
Attachment #825178 - Flags: review?(reuben.bmo)
Comment on attachment 825178 [details] [diff] [review]
patch v5

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

r=me, thanks for the patch!
Attachment #825178 - Flags: review?(reuben.bmo) → review+
Try is fine :) checkin-needed !
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c38e10b1be8b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
fixed in master 1.3. clear flag
blocking-b2g: 1.3? → ---
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: