Closed Bug 788377 Opened 8 years ago Closed 8 years ago

SIM Contacts Import is broken

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: gwagner, Assigned: allstars.chh)

References

Details

(Whiteboard: [WebAPI:P0])

Attachments

(3 files, 6 obsolete files)

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.
blocking-basecamp: --- → ?
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.
Assignee: nobody → allstars.chh
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #658424 - Flags: review?(philipp)
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`?
Attachment #658424 - Flags: review?(philipp) → review-
Importing contacts from the SIM is a v1 requirement.
blocking-basecamp: ? → +
Attachment #658424 - Attachment is obsolete: true
Attachment #658753 - Flags: review?(philipp)
Attached patch Part 2: Get SIM Contacts (obsolete) — Splinter Review
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. :)
Attachment #658754 - Flags: review?(philipp)
Whiteboard: [WebAPI:P0]
update comments in the IDL.
Attachment #658753 - Attachment is obsolete: true
Attachment #658753 - Flags: review?(philipp)
Attachment #659110 - Flags: review?(philipp)
revise typos.
Attachment #659110 - Attachment is obsolete: true
Attachment #659110 - Flags: review?(philipp)
Attachment #659111 - Flags: review?
Attachment #659111 - Attachment is obsolete: true
Attachment #659111 - Flags: review?
Attachment #659111 - Attachment is obsolete: false
Attachment #659111 - Flags: review?(philipp)
Hi, gwagner
I just rename type to contactType in order to simplify implementatin(Part 3).
Could you review this for me ?

Thanks
Attachment #658754 - Attachment is obsolete: true
Attachment #658754 - Flags: review?(philipp)
Attachment #659179 - Flags: review?(anygregor)
Attached patch Part 3: Get SIM Contacts. (obsolete) — Splinter Review
Hi, philikon
This is the implementations,
can you review this for me?

Thanks
Attachment #659180 - Flags: review?(philipp)
Attachment #659179 - Flags: review?(anygregor) → review+
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.
Attachment #659111 - Flags: review?(philipp) → review+
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.
Attachment #659180 - Flags: review?(philipp) → review-
(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
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
OS: Mac OS X → All
Hardware: x86 → All
This is a pretty big dogfood blocker, would love to see this closed up asap.
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!
Attachment #659180 - Flags: review- → review+
Address to philikon's comments
1. update uuid of nsIRILContactCallback
2. update comments in IDL, to make them in the same line
Attachment #659111 - Attachment is obsolete: true
just update comments in the patch description.
Attachment #659180 - Attachment is obsolete: true
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.