931711 - mozContact constructor creates unconsistent objects with the 'undefined' string as 'id'

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Jose Manuel Cantera]

Under certain conditions I've not been able to determine, the mozContact Constructor creates objects with id containing the string 'undefined'.

This can be debugged at https://github.com/jmcanterafonseca/gaia/blob/fix_931691/apps/communications/contacts/js/views/form.js#L443

How to repro is easy. Just import a FB Contact and then remove it. The referenced branch will show in the console what I'm reporting

Comment 1

[Jose Manuel Cantera]

Reuben,

I have not heard of this and it is causing important regressions in the contacts app. Please could you check?

thanks!

Comment 2

[Reuben Morais [:reuben]]

The circumstances causing this are clear:

 var c = new mozContact();
 c.id = undefined;

Or, less obviously:

 var obj = {
   name: ["Foo"],
 };
 var c = new mozContact();
 for (prop in c) {
   if (prop in obj) {
     c[prop] = obj[prop];
   }
 }

Or something of that nature.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

is there any end user impact on this?

Comment 4

[Jose Manuel Cantera]

yes

bug 931691

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

triage: regression,1.3+

Comment 6

[Julien Wajsberg [:julienw]]

José, can you please point me the code where the mozContact object is created in the Contacts app?

In [1] this is not clear to me if we're in the situation of directly getting a mozContact or if we create a new mozContact instance in this location.

[1] https://github.com/jmcanterafonseca/gaia/blob/fix_931691/apps/communications/contacts/js/views/form.js#L441

Comment 7

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> José, can you please point me the code where the mozContact object is
> created in the Contacts app?
> 
> In [1] this is not clear to me if we're in the situation of directly getting
> a mozContact or if we create a new mozContact instance in this location.
> 
> [1]
> https://github.com/jmcanterafonseca/gaia/blob/fix_931691/apps/communications/
> contacts/js/views/form.js#L441

We try to remove the mozContact at 

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/fb/fb_contact.js#L634

and the mozContact has been previously obtained when trying to remove it at 
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/form.js#L448

and the 'contact' object should have been obtained via a request to navigator.mozContacts.find 

I hope this clarifies

Comment 8

[Julien Wajsberg [:julienw]]

ok, because of the other issues we have right now, I really can't debug this now.

Comment 9

[Julien Wajsberg [:julienw]]

When I debug, I see a correct id in the "contact" so I'd say the facebook import is overriding this id somehow.

Let's wait until the slices are working correctly before we move forward here.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> c.id = undefined;

So this is supported right now, but will start throwing when bug 946294 happens, right?

Comment 11

[Julien Wajsberg [:julienw]]

"id" won't be a Sequence, so I guess this will still work.

But anyway, if there is a "c.id = undefined" somewhere, this is wrong.

Comment 12

[Julien Wajsberg [:julienw]]

This code is failing:

  var data = {"id":"f0a97447a8e949a0a4c4a6cceb1b1d89","name":["Julien Wajsberg"]};
  var contact = new mozContact(data);
  console.log(contact.id); // "undefined"

(note: not `undefined` the value, vue `"undefined"` the string)

Actually, merely doing this gives the same thing:

  var contact = new mozContact();
  console.log(contact.id); // "undefined"

So there are 2 things here:

1. The webidl is defining "id" as a DOMString (without '?') so I think `undefined` the value is being automatically converted to "undefined" the string by the WebIDL glue. Is it what we want?
2. We don't copy the id from the constructor argument.

Reuben, can you please comment on these 2 issues and tell me what you think of them? Is it worth it having 2 separate bugs?


BTW José, it would have saved a lot of time to everyone if you could have come up with such an easy test case to reproduce the issue. I lost nearly one day debugging into the Contacts app.

Comment 13

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> Reuben, can you please comment on these 2 issues and tell me what you think
> of them? Is it worth it having 2 separate bugs?

Both behaviors are expected, that's what the IDL has always said. If you think there's a compelling argument for behaving differently, yes, please file a bug for each one of those things. Note that both changes can break existing consumers subtly.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm.  So how does a mozContact get assigned an ID?  ContactProperties doesn't have "id" in it (which is why behavior #2 in comment 12 occurs).  But it seems like returning "" would be better if we have no id than returning "undefined", no?

Comment 15

[Julien Wajsberg [:julienw]]

I discussed this a little more with Gregor that is in Paris right now. I now think that we should not copy the id, like the current behavior. Instead the client code should retrieve the correct mozContact object from the DB using the id.

I'll file a bug about the "undefined" id though it's not something urgent.

Comment 16

[Julien Wajsberg [:julienw]]

(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #14)
> Hmm.  So how does a mozContact get assigned an ID?

I haven't checked but I'd say it's when it gets saved.

> ContactProperties
> doesn't have "id" in it (which is why behavior #2 in comment 12 occurs). 
> But it seems like returning "" would be better if we have no id than
> returning "undefined", no?

I was thinking `null` to make it clear that we have no id, but "" would work for me and would not make it mandatory to change the webidl. Filed bug 951829.

Comment 17

[Julien Wajsberg [:julienw]]

Assigning back to José since I think the Contacts code needs to be modified -- see comment 15.

Comment 18

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! I took a look at this bug (regarding the Gaia part) and as far as my understanding there's nothing to do on the Gaia side of the Contacts app since I checked that every new instance of MozContact is safely created. By safely I mean that it is not passed an already existent MozContact instance (which would cause the loosing of the id according to all you guys have comment before) or if it is passed the id is not used or needed for the processing of the contact (I found some cases when importing contacts before finding matching contacts and merging it with them, for example).

Julien, I guess you were referring to this kind of modifications in comment 16, right?

Apart from this and if you are fine with it, I had doubts regarding how to evolve the bug. Should I evolve it to RESOLVED - INVALID, WONTFIX, WORKSFORME? :)

Comment 19

[Julien Wajsberg [:julienw]]

I have a WIP patch in bug 951829 for the fact that we get "undefined" as id when id is not specified.

Now, the issue in the contacts app was producing bug 931691.

The issue was that we were trying to delete a mozContact that was not returned from the API.

If you checked that this is not done like this now, and if bug 931691 does not reproduce now, then I think we can close both this bug and bug 931691 WORKSFORME.

Comment 20

[Germán Toro del Valle (:gtorodelvalle)]

OK, it seems I went too far with my affirmation in comment 18 since I missed that one, Julien :-p

I have proposed a patch for bug 931691 which solves the issue. Basically, when creating a new instance of mozContact passing some data, the id of the data passed is preserved (copied to) in the new instance. This is: https://github.com/mozilla-b2g/gaia/pull/15358/files#diff-cb57a34ddd72655a1e903c448be79724L11

I will include that same mechanism as a patch to this bug to avoid the problems which may arise from 'loosing' the id.

BTW, are there any other mozContact properties which may show the same behaviour as the 'id' property, I mean being 'overwritten' even if the data passed to generate the new instance includes them?

Comment 21

[Julien Wajsberg [:julienw]]

> BTW, are there any other mozContact properties which may show the same behaviour as the 'id' property, I mean being 'overwritten' even if the data passed to generate the new instance includes them?

It's not really "overwritten", it's just not copied ;) And then you get "undefined" (the string) because it's really undefined (the value) internally.

Look at [1] and you'll see all properties that are copied. I think "id" is the only one that is not copied, because this property really points to a mozContact object that comes from the db.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/webidl/Contacts.webidl#27

That said, I'm quite open to introduce a "removeById" because this looks cumbersome to have to look up a contact to be able to remove it. What do you think Reuben ? But this won't likely happen for 1.3.

Comment 22

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 15366.html

Comment 23

[Reuben Morais [:reuben]]

(In reply to Julien Wajsberg [:julienw] from comment #21)
> That said, I'm quite open to introduce a "removeById" because this looks
> cumbersome to have to look up a contact to be able to remove it. What do you
> think Reuben ? But this won't likely happen for 1.3.

Making remove take a (DOMString or mozContact) is definitely doable, id is all that's needed to do a removal anyway. I guess people are saving lists of IDs and then want to operate on the corresponding contacts?

Comment 24

[Julien Wajsberg [:julienw]]

Yep that's basically this.

I'll file a bug tomorrow :)

Comment 26

[Jose Manuel Cantera]

Comment on attachment 8360479 [details]
15366.html

thanks Germán!

Comment 27

[Germán Toro del Valle (:gtorodelvalle)]

Merged in master: https://github.com/mozilla-b2g/gaia/commit/21ea73e5f17ddf769612942fc383200a63aa2c33

Comment 28

[Julien Wajsberg [:julienw]]

Filed Bug 960937 for the mozContacts.remove(id) case.

Comment 29

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 21ea73e5f17ddf769612942fc383200a63aa2c33 to:
v1.3: 07b587699761c43ca6c21b122aceee2a9b911a9a