921318 - B2G RIL: incorrect recordId in ICCContactHelper.findUSimFreeADNRecordId

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

This is reported by Vasanth.

ICCContactHelper.findUSimFreeADNRecordId returns recordId, but it didn't consider current Phonebook Set index, it should do something like 
recordId = recordId + pbrIndex * ICC_MAX_LINEAR_FIXED_RECORDS.

Comment 1

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

Now met errors when running tests, filed Bug 921388 for this.

Comment 2

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

Attachment: Part 1: Patch

Comment 3

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

Attachment: Part 2: xpcshell tests.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 812454 [details] [diff] [review]
Part 1: Patch

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

Please separate the two variable names instead.  Let 'recordId' be the id of the record in one specific phone book set, and 'recordIndex', for example, be the index of the record of all phone book sets.

Comment 5

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

Attachment: Part 1: Use contactId. v2

Comment 6

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

Attachment: Part 2: xpcshell tests cases. v2.

Comment 7

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

Attachment: Part 2: xpcshell tests cases. v2.

uploaded wrong patch last time.

Comment 8

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

Attachment: Part 1. use contactId. v3

Comment 9

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

Try
https://tbpl.mozilla.org/?tree=Try&rev=03a98b273973

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 818386 [details] [diff] [review]
Part 1. use contactId. v3

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

::: dom/system/gonk/ril_worker.js
@@ +877,5 @@
> +        for (let i = 0; i < contacts.length; i++) {
> +          let contact = contacts[i];
> +          let pbrIndex = contact.pbrIndex || 0;
> +          let recordId = pbrIndex * ICC_MAX_LINEAR_FIXED_RECORDS + contact.recordId;
> +          contact.contactId = this.iccInfo.iccid + recordId;

How about rename |recordId| here to |recordIndex| or something alike?  Just don't have another name conflict, please.

@@ +918,5 @@
>      let iccid = RIL.iccInfo.iccid;
> +    if (contact.contactId.startsWith(iccid)) {
> +      let recordId = contact.contactId.substring(iccid.length);
> +      contact.pbrIndex = Math.floor(recordId / ICC_MAX_LINEAR_FIXED_RECORDS);
> +      contact.recordId = recordId % ICC_MAX_LINEAR_FIXED_RECORDS;

s/recordId/recordIndex/, and keeps |contact.recordId|.

@@ +12456,5 @@
>     * @param onerror       Callback to be called when error.
>     */
>    findFreeICCContact: function findFreeICCContact(appType, contactType, onsuccess, onerror) {
> +    let successCb = function (recordId, pbrIndex) {
> +      // For 'fdn' or 'adn' on SIM, theres' no PBR, so default to 0.

typo: there's

@@ +12459,5 @@
> +    let successCb = function (recordId, pbrIndex) {
> +      // For 'fdn' or 'adn' on SIM, theres' no PBR, so default to 0.
> +      pbrIndex = pbrIndex || 0;
> +      onsuccess(recordId, pbrIndex);
> +    }.bind(this);

Please remove this function completely.

@@ +12465,4 @@
>      switch (contactType) {
>        case "adn":
>          if (!this.hasDfPhoneBook(appType)) {
> +          ICCRecordHelper.findFreeRecordId(ICC_EF_ADN, successCb, onerror);

Instead, do:

  ICCRecordHelper.findFreeRecordId(ICC_EF_ADN, onsuccess.bind(null, 0), onerror);
  ICCRecordHelper.findFreeRecordId(ICC_EF_FDN, onsuccess.bind(null, 0), onerror);

From now on, the callback function of |ICCContactHelper.findFreeICCContact| takes two arguments -- one for pbrIndex, probably zero, another for recordId.

@@ +12503,5 @@
>        ICCRecordHelper.findFreeRecordId(
>          pbr.adn.fileId,
> +        function (recordId) {
> +          onsuccess(recordId, pbrIndex);
> +        }.bind(this),

Can we have |onsuccess.bind(this, pbrIndex)| instead?

@@ +12508,3 @@
>          function (errorMsg) {
>            findFreeRecordId.bind(this, pbrIndex + 1);
>          }.bind(this));

The original lines here are wrong.  Just replace these three lines with |findFreeRecordId.bind(null, pbrIndex + 1)|.

@@ +12521,5 @@
>     * @param onsuccess     Callback to be called when success.
>     * @param onerror       Callback to be called when error.
>     */
>    addICCContact: function addICCContact(appType, contactType, contact, pin2, onsuccess, onerror) {
> +    let foundFreeCb = function foundFreeCb(recordId, pbrIndex) {

That makes this |functionFreeCb(pbrIndex, recordId)|.  So applies to |successCb| in |findFreeICCContact|.

@@ +12527,3 @@
>        contact.recordId = recordId;
>        ICCContactHelper.updateICCContact(appType, contactType, contact, pin2, onsuccess, onerror);
>      }.bind(this);

You don't need "this".

Comment 11

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

Attachment: Part 1: Use contactId. v4

Addressed review comments

Comment 12

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

Attachment: Part 2: xpcshell tests cases. v3

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 827181 [details] [diff] [review]
Part 1: Use contactId. v4

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

Thank you :)

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 827182 [details] [diff] [review]
Part 2: xpcshell tests cases. v3

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

Having 'expectedPbrIndex' in the process is really strange for me.  We should imitate the behaviour in a simplified yet not interfering way and examine whether the result fits our expectation perfectly.

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1610,5 @@
>    let recordHelper = worker.ICCRecordHelper;
>    let contactHelper = worker.ICCContactHelper;
> +  let pbrs = [{adn:{fileId: 0x6f3a}, email: {}, anr0: {}},
> +              {adn:{fileId: 0x6f3b}, email: {}, anr0: {}}];
> +  let pbrIndex = 0;

don't need it.

@@ +1616,4 @@
>  
> +  function do_test(expectedPbrIndex) {
> +    recordHelper.readPBR = function (onsuccess, onerror) {
> +      onsuccess(pbrs.slice(0, expectedPbrIndex + 1));

|onsuccess(pbrs.slice(0))|

@@ +1621,2 @@
>  
> +    recordHelper.findFreeRecordId = function (fileId, onsuccess, onerror) {

for (let pbr of pbrs) {
  if (pbr.adn.fileId != fileId) {
    continue;
  }
  if (pbr.email.xxx) {
    continue;
  }

  onsuccess(RECORD_ID);
  return;
}
onerror();

@@ +1630,4 @@
>  
> +    let successCb = function (pbrIndex, recordId) {
> +      do_check_eq(pbrIndex, expectedPbrIndex);
> +      do_check_eq(recordId, RECORD_ID);

pbrs[pbrIndex].email.xxx = "[e-mail]";

Comment 15

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

Comment on attachment 827182 [details] [diff] [review]
Part 2: xpcshell tests cases. v3

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1610,5 @@
>    let recordHelper = worker.ICCRecordHelper;
>    let contactHelper = worker.ICCContactHelper;
> +  let pbrs = [{adn:{fileId: 0x6f3a}, email: {}, anr0: {}},
> +              {adn:{fileId: 0x6f3b}, email: {}, anr0: {}}];
> +  let pbrIndex = 0;

I'll remove the email and anr0 from pbrs.
Since this test is about finding free space on ICC.
So I won't address testing email as you commented below.

Comment 16

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

Attachment: Part 2: xpcshell tests cases. v4

Addressed some comments from Vicamo,
but also removed some unneccesary code for testing like email, anr.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 827909 [details] [diff] [review]
Part 2: xpcshell tests cases. v4

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

Had a discuss in person.  Yoshi is to provide another revision.

Comment 18

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

Comment on attachment 827181 [details] [diff] [review]
Part 1: Use contactId. v4

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1619,5 @@
>    recordHelper.findFreeRecordId = function (fileId, onsuccess, onerror) {
>      onsuccess(RECORD_ID);
>    };
>  
> +  let successCb = function (recordId, pbrIndex) {

The positions should be swapped now.

Comment 19

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

Attachment: Part 1: Use contactId. v5

Address Comment 18

Comment 20

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

Attachment: Part 2: xpcshell tests cases. v5

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 830071 [details] [diff] [review]
Part 2: xpcshell tests cases. v5

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1615,5 @@
>    const PBR_INDEX = 0;
>  
> +  recordHelper.findFreeRecordId = function (fileId, onsuccess, onerror) {
> +    if (records.length > MAX_RECORDS) {
> +      let error = onerror || do_print;

When will we have undefined/null onerror?  Please just have |onerror(...)|.

@@ +1676,5 @@
> +  recordHelper.findFreeRecordId = function (fileId, onsuccess, onerror) {
> +    let pbr = (fileId == ADN1_FILE_ID ? pbrs[0]: pbrs[1]);
> +    if (pbr.adn.records.length > MAX_RECORDS) {
> +      let error = onerror || do_print;
> +      error("No free record found.");

ditto

Comment 22

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

Attachment: Part 2: xpcshell tests cases. v6

Address Vicamo's comments

Comment 23

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

https://hg.mozilla.org/integration/b2g-inbound/rev/8f78ae808207
https://hg.mozilla.org/integration/b2g-inbound/rev/5df162e4721f

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8f78ae808207
https://hg.mozilla.org/mozilla-central/rev/5df162e4721f