929492 - Selecting a Contact for the Message app using Contacts picker fails

Status: RESOLVED FIXED

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

Description

[Zac C (:zac)]

STR:
1. Add a contact with name/phone number.
2. Kill contacts app
3. Load messages app, tap new message
4. Tap (+) to open contacts picker
5. Tap the contact added in step 1

Actual result:
Nothing happens (error in logcat)

Expected result:
Contacts picker closed and contact details added to Messages app

Device Hamachi
Gecko  http://hg.mozilla.org/mozilla-central/rev/177bf37a49f5
Gaia  50f544a7530c266aab9bbc9a893e00e15d34c02f
BuildID 20131022040242
Version 27.0a1



Logcat reads:

E/GeckoConsole(  921): [JavaScript Error: "'toJSON' called on an object that does not implement interface mozContact." {file: "jar:file:///system/b2g/omni.ja!/components/ActivityRequestHandler.js" line: 54}]
E/GeckoConsole(  921): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "'toJSON' called on an object that does not implement interface mozContact." {file: "jar:file:///system/b2g/omni.ja!/components/ActivityRequestHandler.js" line: 54}]' when calling method: [nsIDOMMozActivityRequestHandler::postResult]" {file: "app://communications.gaiamobile.org/contacts/gaia_build_defer_index.js" line: 14}]

Comment 1

[Reuben Morais [:reuben]]

Oh boy, this one is fun. I think what's happening here is that the code in activities.js is copying all the properties of a mozContact object over to a JS object - including the prototype. So mozContact.prototype.toJSON ends up getting called on the object when someone calls JSON.stringify on it.

This type of code exists because old mozContact objects couldn't be JSONified, so people resorted to manually copying the attributes to a new object before sending it over some channel that does JSON.stringify (notably, the message manager).

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

So we should be able to just remove the copy operation?

Comment 3

[Reuben Morais [:reuben]]

Yes.

Comment 4

[Reuben Morais [:reuben]]

So this is trickier to fix due to the way stringifiers work in WebIDL right now. I'll land a workaround instead, and fix the behavior of mozContact in Gecko.

Comment 5

[Reuben Morais [:reuben]]

Attachment: Gaia PR

Comment 6

[Jason Smith [:jsmith]]

Reuben - Why are you unnoming the blocking flag? This should be a blocker for 1.3 - it's a regression.

Comment 7

[Reuben Morais [:reuben]]

(In reply to Jason Smith [:jsmith] from comment #6)
> Reuben - Why are you unnoming the blocking flag? This should be a blocker
> for 1.3 - it's a regression.

Sorry, those were accidents. I keep tabs loaded for too long and then add comments without reloading first :(

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 821285 [details] [review]
Gaia PR

Thanks for following up on this Reuben!  Unfortunately, though, I think this will break web activity consumers in their current form.  They do not currently expect an array of values to be returned.

This was caught in the unit test:

  https://travis-ci.org/mozilla-b2g/gaia/jobs/12954613

Its unclear to me if the web activity "standard" is to return a real mozContact or if its supposed to be returning some kind of normalized result object divorced from the mozContacts api.

Comment 10

[Reuben Morais [:reuben]]

What do you think is the best way forward here? I think consumers should expect to get a proper mozContact object. SMS does it right: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L530

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

I'd like Francisco to chime in here.  He has more of the history than I do.  From IRC, however, it sounds like SMS is a bit of an exception.

Comment 12

[Julien Wajsberg [:julienw]]

We do it that way in SMS because we actually got bitten by this in the past already :)

Comment 13

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

Hi folks,

Difficult point here, I would leave the activities information as it is for the following reasons:

- Compatibility, not just our core apps, that we could decide to modify, but also being web activities something that can be use by rd party developers, don't want to make them angry
- Data sent back in the activity: Some apps won't need a whole contact, maybe just a phone number or just an email. Some apps will need the full contact, that behavior is happening right now with the current code.
- Unfortunately there is no place where we document web activities, but they don't have to return the mozObject, right now we support both cases, perhaps this should be documented ... (don't ask me were :S).

So for the sake of compatibility, please someone talk now if don't agree, I think we should keep the current return values.

Cheers!
F.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Reuben, based on Francisco's comments in comment 13, can you make a new patch that maintains the old API result object format?  Thanks!

Comment 15

[Reuben Morais [:reuben]]

Sure. Fixing this the proper way is tricky, returning just a single value will do for now. Regardless, we really should start returning some specified interface, even if that means consumers will ask for "webcontacts2/contact" or something.

Comment 16

[Reuben Morais [:reuben]]

Comment on attachment 821285 [details] [review]
Gaia PR

PR updated.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 821285 [details] [review]
Gaia PR

Looks good, passes the tests, and works on the phone.  Thanks Reuben!

Comment 18

[Reuben Morais [:reuben]]

Thanks for the review!

https://github.com/mozilla-b2g/gaia/commit/30e1a18851334d628eaad3672bb2d5e4d47110cc

Comment 19

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

already in master (1.3). clear flag

Comment 20

[Jason Smith [:jsmith]]

(In reply to Joe Cheng [:jcheng] from comment #19)
> already in master (1.3). clear flag

Please don't clear flags that are clear blockers - this screws up QA's tracking later when we're trying to capture what regressed.