784646 - B2G: Cannot import SIM contacts

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

STR:
Insert a SIM card with contacts in it 
Launch Contact app
Press 'Import SIM Contacts'

From log, I see 

E/GeckoConsole( 1122): [JavaScript Error: "cyclic object value" {file: "jar:file:///system/b2g/omni.ja!/components/ContactManager.js" line: 517}]
E/GeckoConsole( 1122): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "cyclic object value" {file: "jar:file:///system/b2g/omni.ja!/components/ContactManager.js" line: 517}]' when calling method: [nsIRILContactCallback::receiveContactsList]" {file: "jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js" line: 427}]

This seems to be a regression from Bug 779794

Also I am wondering how we store the phone number? 

From IDL, that should be tel[index].value, 
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/contacts/nsIDOMContactProperties.idl#25

but the patch from 779794 uses 'number'
http://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#514

Also from gaia, it also says 'number'
https://github.com/mozilla-b2g/gaia/blob/master/apps/contacts/js/utilities/import_sim_contacts.js#L52

whereas other files use 'value'
https://github.com/mozilla-b2g/gaia/blob/master/apps/contacts/js/contacts_list.js

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

CC to kaze and gwanger for comments

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

also see https://github.com/mozilla-b2g/gaia/issues/3196

Comment 3

[Fabien Cazenave [:kaze]]

FTR, the number/value confusion should not affect gaia, as the code involved in gaia only stores mozContact elements without digging through its properties.

Comment 4

[Gregor Wagner [:gwagner]]

Does this still happen with a new gecko build? It should be fixed now.

Comment 5

[Fabien Cazenave [:kaze]]

Works for me now.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

It works now,
Should I change to state to WORKSFORME?

Thanks

Comment 7

[Fabien Cazenave [:kaze]]

Reopening on Francisco Jordano’s request.

Comment 8

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

Hi,

unfortunately the import contacts from sim (actually reading sim from contacts) is not working with last builds.
When performing the dom request we always get the onerror callback.

I'm attaching a small gaia app that just tries to call :

var request = navigator.mozContacts.getSimContacts('ADN');

and shows the result in the screen.

Cheers!

Comment 9

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

Attachment: Simple gaia app to test to read the contacts from the sim

Despite being called 'simimport' this is not importing anything, just testing the call to the method:

navigator.mozContacts.getSimContacts('ADN')

Comment 10

[Andrew Overholt [:overholt]]

Importing SIM contacts is a blocking issue.

Comment 11

[Gregor Wagner [:gwagner]]

(In reply to Fabien Cazenave (:kaze) from comment #7)
> Reopening on Francisco Jordano’s request.

Yoshi, can you take a look? The new error callback landed yesterday.

In the future please don't reopen a bug. Please file a new one because the old comments are most likely wrong for the current bug.

Comment 12

[Philipp von Weitershausen [:philikon]]

(In reply to francisco.jordano from comment #8)
> unfortunately the import contacts from sim (actually reading sim from
> contacts) is not working with last builds.

Note that this may also be a SIM-card specific issue.

> When performing the dom request we always get the onerror callback.

Can you please turn on DEBUG_ALL in gecko/dom/system/gonk/ril_consts.js [1], reflash Gecko, run your tests again, and attach the output of adb logcat as a file to this bug? Thanks!

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#17

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Hi, gwanger
Could you help to review this?
The cause is undefined will become 'undefined' (string) after converted to json.

BTW I still see a gaia error after this patch.

E/GeckoConsole(14195): [JavaScript Error: "TypeError: this is undefined" {file: "app://communications.gaiamobile.org/contacts/js/contacts_list.js" line: 84}]

Thanks

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665534 [details] [diff] [review]
Patch

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

::: dom/contacts/fallback/ContactService.jsm
@@ +151,5 @@
>        case "Contacts:GetSimContacts":
>          mRIL.getICCContacts(
>            msg.options.contactType,
>            function (aErrorMsg, aType, aContacts) {
> +            if (aErrorMsg !== 'undefined') {

Wuuut? How would aErrorMsg be the *string* 'undefined'?

Comment 15

[Gregor Wagner [:gwagner]]

(In reply to Philipp von Weitershausen [:philikon] from comment #14)
> Comment on attachment 665534 [details] [diff] [review]
> Patch
> 
> Review of attachment 665534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/contacts/fallback/ContactService.jsm
> @@ +151,5 @@
> >        case "Contacts:GetSimContacts":
> >          mRIL.getICCContacts(
> >            msg.options.contactType,
> >            function (aErrorMsg, aType, aContacts) {
> > +            if (aErrorMsg !== 'undefined') {
> 
> Wuuut? How would aErrorMsg be the *string* 'undefined'?

Some cross process/thread json stringify magic. I also saw it with the message manager that just declared properties become 'undefined' on the other side.

Comment 16

[Gregor Wagner [:gwagner]]

Ah actually that comes from nsIRILContactCallback calling with an undefined value. Lets check first that this behavior will stay the same in the future.

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to Gregor Wagner [:gwagner] from comment #16)
> Ah actually that comes from nsIRILContactCallback calling with an undefined
> value. Lets check first that this behavior will stay the same in the future.

Yeah, I feel like there's an actual bug lurking in the RIL here. Let's fix it at the root.

Comment 18

[Gregor Wagner [:gwagner]]

Comment on attachment 665534 [details] [diff] [review]
Patch

I talked to bz about it and it turns out that the proper fix is to annotate our IDL like [Undefined(Null)] in DOMString errorMsg. But this also doesn't work because there is some double wrapping going on.

Once we switch to webIDL it should just work :)

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I cannot push this to inbound now, will do later.

remote: Tree mozilla-inbound is CLOSED! (https://treestatus.mozilla.org/mozilla-inbound?format=json) - Power outage in MTV
remote: To push despite the closed tree, include "CLOSED TREE" in your push comment
remote: transaction abort!
remote: rollback completed
remote: abort: pretxnchangegroup.a_treeclosure hook failed

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c637ee14bccd

Comment 21

[Ed Morley [:emorley]]

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