788377 - SIM Contacts Import is broken

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

Currently I see following if I enable the debug flag to import my 3 contacts from the SIM card:

I/Gecko   ( 1498): RIL Worker: We have at least one complete parcel.
I/Gecko   ( 1498): RIL Worker: Solicited response for request type 28, token 338, error 0
I/Gecko   ( 1498): RIL Worker: Handling parcel as REQUEST_SIM_IO
I/Gecko   ( 1498): RIL Worker: ADN[0] alphaId = Asdf number = 123456
I/Gecko   ( 1498): RIL Worker: ADN[1] alphaId = Ghjk number = 987654
I/Gecko   ( 1498): RIL Worker: ADN[2] alphaId = Namr?e number = 123
I/Gecko   ( 1498): RIL Worker: Parcel handling threw DataCloneError: The object could not be cloned.
I/Gecko   ( 1498): undefined
I/Gecko   ( 1498): RIL Worker: Next parcel size unknown, going to sleep.

Comment 1

[Gregor Wagner [:gwagner]]

It fails in postmessage with following message:

I/Gecko   ( 2063): RIL Worker: {"command":178,"fileId":20282,"pathId":"3f007f105f3a","p1":250,"p2":4,"p3":32,"data":null,"pin2":null,"type":1,"loadAll":true,"requestId":138,"rilRequestType":28,"rilRequestError":0,"recordSize":32,"totalRecords":250,"rilMessageType":"icccontacts","contactType":"ADN","contacts":[{"alphaId":"Asdf","number":"123456"},{"alphaId":"Ghjk","number":"987654"},{"alphaId":"Namr?e","number":"123"}]}
I/Gecko   ( 2063): RIL Worker: Parcel handling threw DataCloneError: The object could not be cloned.

Comment 2

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

Attachment: Patch

Hi, philikon
I found that if I reuse the options object, and the DataCloneError will happen.
But I don't know why. Maybe you can shed me some light here.
The original 'options' contains many properties, so in this patch I create another simple object.

Thanks

Comment 3

[Philipp von Weitershausen [:philikon]]

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

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

Ah yes, DataCloneError will be thrown if you're trying to an object that can't be represented as structured clone data from one thread to another. I'm guessing in this case it's the callback functions (`options.callback` and `options.onerror`). In which case your fix scratches only the tip of the iceberg. How about before invoking these callbacks, we delete them from `options`?

Comment 4

[Andrew Overholt [:overholt]]

Importing contacts from the SIM is a v1 requirement.

Comment 5

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

Attachment: Part 1: Rename type to contactType in getICCContacts

Comment 6

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

Attachment: Part 2: Get SIM Contacts

Hi, philikon
Thanks for your comments, it was caused by function object in options.
So now I delete them first so I can reuse the options object now.

Thanks for your valuable comments. :)

Comment 7

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

Attachment: Part 1: Rename type to contactType in getICCContacts. v2

update comments in the IDL.

Comment 8

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

Attachment: Part 1: Rename type to contactType in getICCContacts. v3

revise typos.

Comment 9

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

Attachment: Part 2: Rename type to contactType in Contact

Hi, gwagner
I just rename type to contactType in order to simplify implementatin(Part 3).
Could you review this for me ?

Thanks

Comment 10

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

Attachment: Part 3: Get SIM Contacts.

Hi, philikon
This is the implementations,
can you review this for me?

Thanks

Comment 11

[Philipp von Weitershausen [:philikon]]

Comment on attachment 659111 [details] [diff] [review]
Part 1: Rename type to contactType in getICCContacts. v3

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

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +127,5 @@
>     *        Type of the dialling number, i.e. ADN, FDN.
>     * @param contacts
>     *        Array of the ICC contacts of the specified type.
>     */
> +  void receiveContactsList(in DOMString contactType, in jsval contacts);

Don't forget to rev the UUID of nsIRILContactCallback

@@ +292,5 @@
> +   *        "FDN" (Fixed Dialling Numbers)
> +   * @param callback
> +   *        A nsIRILContactCallback object.
> +   */
> +  void getICCContacts(in DOMString contactType, in nsIRILContactCallback callback);

Nit: overlong line. We often put each parameter in IDLs on its own line anyway.

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 659180 [details] [diff] [review]
Part 3: Get SIM Contacts.

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

::: dom/system/gonk/ril_worker.js
@@ +1291,5 @@
>                                  " number = " + this.iccInfo.fdn[i].number);
>            }
>          }
> +        delete options.callback;
> +        delete options.onerror;

I would like to delete these from the options object before calling `callback()` or `onerror()`. That way we have to do it only in one place and not everywhere.

Comment 13

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

(In reply to Philipp von Weitershausen [:philikon] from comment #12)
> I would like to delete these from the options object before calling
> `callback()` or `onerror()`. That way we have to do it only in one place and
> not everywhere.

Hi, philikon:
No, callback cannot be deleted before calling, (at least in this case, getting sim contact) because it will be called in each iteration.
See parseDiallingNumber in [1].

The reason to do this is all SIM contacts are stored in EF_ADN,
and in EF_ADN it contains 254(or 508) records (Each record is a phonebook entry)
So we need to call iccIO with options.p1 increased to get each record so the callback will be invoked in each iteration.


[1]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#1265

Comment 14

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

Hi, philikon:
  Do you have any option?
  I think you're asking me to delete callback in _processICCIORead{Record|Binary}, right[1]? But I think it cannot work because we need to call the callback iteratively.

Or if you're talking about different thing please let me know.

Thanks


[1]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2441

Comment 15

[Dietrich Ayala (:dietrich)]

This is a pretty big dogfood blocker, would love to see this closed up asap.

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 659180 [details] [diff] [review]
Part 3: Get SIM Contacts.

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

Begrudgingly, r=me

Sorry for the delay, Yoshi, and thanks for your explanation. Makes total sense!

Comment 17

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

Attachment: Part 1: Rename type to contactType in getICCContacts. v4

Address to philikon's comments
1. update uuid of nsIRILContactCallback
2. update comments in IDL, to make them in the same line

Comment 18

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

Attachment: Part 3: Get SIM Contacts.

just update comments in the patch description.

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a39bb13adde
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd30e265c0ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/8593043a22c5

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6a39bb13adde
https://hg.mozilla.org/mozilla-central/rev/cd30e265c0ac
https://hg.mozilla.org/mozilla-central/rev/8593043a22c5