Closed Bug 1101366 Opened 5 years ago Closed 5 years ago

[B2G][RIL] Contacts can't be imported from SIM card to device when EF_PBR contains more than one record

Categories

(Firefox OS Graveyard :: RIL, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(b2g-v1.4 wontfix, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 affected, b2g-v2.2r affected, b2g-master fixed)

RESOLVED FIXED
2.2 S4 (23jan)
Tracking Status
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- affected
b2g-v2.2r --- affected
b2g-master --- fixed

People

(Reporter: shiwei.zhang, Assigned: jdai)

References

Details

(Whiteboard: [sprd367262] [FFOS7715 v2.1])

Attachments

(1 file, 7 obsolete files)

*** Build Information
Gaia-Rev        2d2dd5b8155c4747da59e4bdd0874146837bc606
Gecko-Rev       e8d9b2bcb118b656f52e915093e1bc321fd535dd
Device-Name     tarako

*** Description
The thumbnail is slowly appeared when scrolls contact list.

*** Steps to Reproduce
1.Insert a du SIM card.
2.Contacts -> Import Contacts -> SIM card. 

*** Expected Results
Import contacts successfully

*** Actual Results
Fail to import contacts from sim card.

*** SIM Card & Network Info 
Du (Dubai)

*** Reproduction Frequency: 
100%
Attached file main.log (obsolete) —
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

We found some gecko exception in logs.

01-14 03:21:45.240    86   479 I Gecko   : readPhonebook@resource://gre/modules/ril_worker.js:1268
01-14 03:21:45.240    86   479 I Gecko   : readField@resource://gre/modules/ril_worker.js:1268
01-14 03:21:45.240    86   479 I Gecko   : readPhonebookField@resource://gre/modules/ril_worker.js:1270
01-14 03:21:45.240    86   479 I Gecko   : readField@resource://gre/modules/ril_worker.js:1270
01-14 03:21:45.240    86   479 I Gecko   : readPhonebookField@resource://gre/modules/ril_worker.js:1270
01-14 03:21:45.240    86   479 I Gecko   : readField@resource://gre/modules/ril_worker.js:1270
01-14 03:21:45.240    86   479 I Gecko   : readSupportedPBRFields@resource://gre/modules/ril_worker.js:1270
01-14 03:21:45.240    86   479 I Gecko   : gotAdnCb@resource://gre/modules/ril_worker.js:1268
01-14 03:21:45.240    86   479 I Gecko   : callback@resource://gre/modules/ril_worker.js:1078
01-14 03:21:45.240    86   479 I Gecko   : processICCIOReadRecord@resource://gre/modules/ril_worker.js:1068
01-14 03:21:45.240    86   479 I Gecko   : ICC_COMMAND_READ_RECORD@resource://gre/modules/ril_worker.js:1071
01-14 03:21:45.240    86   479 I Gecko   : processICCIO@resource://gre/modules/ril_worker.js:1059
01-14 03:21:45.240    86   479 I Gecko   : REQUEST_SIM_IO@resource://gre/modules/ril_worker.js:511
01-14 03:21:45.240    86   479 I Gecko   : handleParcel@resource://gre/modules/ril_worker.js:471
01-14 03:21:45.240    86   479 I Gecko   : processParcel@resource://gre/modules/ril_worker.js:10
01-14 03:21:45.240    86   479 I Gecko   : processIncoming@blob:84970997-5076-4860-926c-2fdcc6a2901c:39
01-14 03:21:45.240    86   479 I Gecko   : handleRilMessage@resource://gre/modules/ril_wor
We found changing ril_worker.js like this can handle this issue, 

--- a/dom/system/gonk/ril_worker.js
+++ b/dom/system/gonk/ril_worker.js
@@ -13910,10 +13910,10 @@ ICCContactHelperObject.prototype = {
         return;
       }

-      this.readPhonebookSet(pbrs[pbrIndex], readPhonebook, onerror);
-    }.bind(this);
+      this.readPhonebookSet(pbrs[pbrIndex], readPhonebook.bind(this), onerror);
+    }

-    this.readPhonebookSet(pbrs[pbrIndex], readPhonebook, onerror);
+    this.readPhonebookSet(pbrs[pbrIndex], readPhonebook.bind(this), onerror);
   },

and highly doubt that the same issue will happen on v2.1

Please give a reivew to this patch.
thanks.
Summary: [Contacts][1.3t] Contacts can't be imported from SIM card to device in Du network → [Contacts][tarako][1.3t] Contacts can't be imported from SIM card to device in Du network
Summary: [Contacts][tarako][1.3t] Contacts can't be imported from SIM card to device in Du network → [Contacts][dolphin][1.4][2.1] Contacts can't be imported from SIM card to device in Du network
Flags: needinfo?(vicamo)
Flags: needinfo?(allstars.chh)
  readAllPhonebookSets: function(pbrs, onsuccess, onerror) {
    let allContacts = [], pbrIndex = 0;
    let readPhonebook = function readPhonebook(contacts) {
      if (contacts) {
        allContacts = allContacts.concat(contacts);
      }

      let cLen = contacts ? contacts.length : 0;
      for (let i = 0; i < cLen; i++) {
        contacts[i].pbrIndex = pbrIndex;
      }

      pbrIndex++;
      if (pbrIndex >= pbrs.length) {
        if (onsuccess) {
          onsuccess(allContacts);
        }
        return;
      }

// "this" is undefined when recursive calling at the second time
      this.readPhonebookSet(pbrs[pbrIndex], readPhonebook, onerror);
    }.bind(this);

    this.readPhonebookSet(pbrs[pbrIndex], readPhonebook, onerror);
  },
Forward to Hsinyi's team.
Flags: needinfo?(vicamo)
Flags: needinfo?(htsai)
Flags: needinfo?(allstars.chh)
Change component to proper one and keep ni? to me for tracking.
Component: Gaia::Contacts → RIL
Flags: needinfo?(htsai) → needinfo?(echen)
Flags: needinfo?(echen)
Flags: needinfo?(echen)
Hi John, if you need any help, please let me know. :)
Assignee: nobody → jdai
Flags: needinfo?(echen)
Learned from Vance, change the project name to "FFOS7715 v2.1" for filtering efficiently.
Summary: [Contacts][dolphin][1.4][2.1] Contacts can't be imported from SIM card to device in Du network → [Contacts][dolphin][1.4][FFOS7715 v2.1] Contacts can't be imported from SIM card to device in Du network
Hi John -

Any update on this one?

Thanks
Flags: needinfo?(jdai)
Hi Vance,

I already have a patch on it. 


Thanks
Flags: needinfo?(jdai)
1. Fixed contacts can't be imported from SIM card to device in Du network. Address comment 3.
2. Update the related testcase.
Attachment #8525119 - Attachment is obsolete: true
Attachment #8542000 - Flags: review?(echen)
Comment on attachment 8542000 [details] [diff] [review]
Fixed contacts can't be imported from SIM card to device in Du network.

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

Please revise the commit message a bit to make it clearer, e.g. `... Fix contacts can't be imported when EF_PBR contains more than one record ...` .

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +138,5 @@
>        onsuccess(1);
>      };
>  
>      record.readPBR = function readPBR(onsuccess, onerror) {
> +      onsuccess([{adn:{fileId: 0x6f3a}, email: {}, anr0: {}},

Thanks for handling the test cases, but with this change, we lost the test for the case that EF_PBR contains only one recorder. Could we have both of them?
Attachment #8542000 - Flags: review?(echen)
(In reply to Edgar Chen [:edgar][:echen] from comment #11)
> Comment on attachment 8542000 [details] [diff] [review]
> Fixed contacts can't be imported from SIM card to device in Du network.
> 
> Review of attachment 8542000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please revise the commit message a bit to make it clearer, e.g. `... Fix
> contacts can't be imported when EF_PBR contains more than one record ...` .
> 
> ::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
> @@ +138,5 @@
> >        onsuccess(1);
> >      };
> >  
> >      record.readPBR = function readPBR(onsuccess, onerror) {
> > +      onsuccess([{adn:{fileId: 0x6f3a}, email: {}, anr0: {}},
> 
> Thanks for handling the test cases, but with this change, we lost the test
> for the case that EF_PBR contains only one recorder. Could we have both of
                                             ^^^^^^^^
                                             typo: should be record
> them?
(In reply to Edgar Chen [:edgar][:echen] from comment #12)
> (In reply to Edgar Chen [:edgar][:echen] from comment #11)
> > Comment on attachment 8542000 [details] [diff] [review]
> > Fixed contacts can't be imported from SIM card to device in Du network.
> > 
> > Review of attachment 8542000 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please revise the commit message a bit to make it clearer, e.g. `... Fix
> > contacts can't be imported when EF_PBR contains more than one record ...` .
> > 

Will do.

> > ::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
> > @@ +138,5 @@
> > >        onsuccess(1);
> > >      };
> > >  
> > >      record.readPBR = function readPBR(onsuccess, onerror) {
> > > +      onsuccess([{adn:{fileId: 0x6f3a}, email: {}, anr0: {}},
> > 
> > Thanks for handling the test cases, but with this change, we lost the test
> > for the case that EF_PBR contains only one recorder. Could we have both of
>                                              ^^^^^^^^
>                                              typo: should be record
> > them?

Will do.
1. Revise commit message.
2. Update the related testcase base on comment 12
Attachment #8542000 - Attachment is obsolete: true
Attachment #8542495 - Flags: review?(echen)
Comment on attachment 8542495 [details] [diff] [review]
Fix contacts can't be imported when EF_PBR contains more than one record.(V2)

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

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +189,5 @@
>      email:    "hello@mail.com",
>      anr:      "123456"
>    };
>  
> +  let expectedContact3 = {

Please have a data structure putting the test data and expected result together, something like http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/tests/test_ril_worker_icc_IconLoader.js#21-128, it makes the code clearer. Thank you.

@@ +206,5 @@
> +    anr:      "123456"
> +  };
> +
> +  let PBRRecords = [{adn:{fileId: 0x6f3a}, email: {}, anr0: {}},
> +                    {adn:{fileId: 0x6f4a}, email: {}, anr1: {}},

We don't support `anr1` and `anr2`, please correct it.

@@ +222,4 @@
>    // SIM
>    do_print("Test read SIM adn contacts");
> +  do_test(CARD_APPTYPE_SIM, GECKO_CARDCONTACT_TYPE_ADN, expectedContact1, PBRRecord, ADNRecord);
> +  do_test(CARD_APPTYPE_SIM, GECKO_CARDCONTACT_TYPE_ADN, expectedContact3, PBRRecords, ADNRecords);

2G SIM card doesn't have EF_PBR actually.
But the expected result is `expectedContact3` that contains `pbrIndex = 2`. To me, this test should get error in this kinds of setup. If not, I think the test has some problem and we should fix it.

@@ +226,4 @@
>  
>    do_print("Test read SIM fdn contacts");
> +  do_test(CARD_APPTYPE_SIM, GECKO_CARDCONTACT_TYPE_FDN, expectedContact1, PBRRecord, ADNRecord);
> +  do_test(CARD_APPTYPE_SIM, GECKO_CARDCONTACT_TYPE_FDN, expectedContact3, PBRRecords, ADNRecords);

Ditto, reading FDN won't have to access EF_PBR.

@@ +230,5 @@
>  
>    // USIM
>    do_print("Test read USIM adn contacts");
> +  do_test(CARD_APPTYPE_USIM, GECKO_CARDCONTACT_TYPE_ADN, expectedContact2, PBRRecord, ADNRecord);
> +  do_test(CARD_APPTYPE_USIM, GECKO_CARDCONTACT_TYPE_ADN, expectedContact4, PBRRecords, ADNRecords);

This looks a little weird to me, why we expect pbrIndex is 2?

And in do_test() we only check this first contact, do we have to check all contacts?

@@ +235,4 @@
>  
>    do_print("Test read USIM fdn contacts");
> +  do_test(CARD_APPTYPE_USIM, GECKO_CARDCONTACT_TYPE_FDN, expectedContact2, PBRRecord, ADNRecord);
> +  do_test(CARD_APPTYPE_USIM, GECKO_CARDCONTACT_TYPE_FDN, expectedContact4, PBRRecords, ADNRecords);

Ditto.

@@ +244,4 @@
>  
>    do_print("Test read RUIM fdn contacts");
> +  do_test(CARD_APPTYPE_RUIM, GECKO_CARDCONTACT_TYPE_FDN, expectedContact2, PBRRecord, ADNRecord);
> +  do_test(CARD_APPTYPE_RUIM, GECKO_CARDCONTACT_TYPE_FDN, expectedContact4, PBRRecords, ADNRecords);

Ditto.

@@ +253,2 @@
>  
>    do_print("Test read RUIM fdn contacts with enhanced phone book");

Hmm, the enhanced phone book is for ADN only, this test is meaningless, please help to remove it.
Attachment #8542495 - Flags: review?(echen)
Nominate this one to 2.1s blocker such that we won't forget to uplift when the patch is ready
blocking-b2g: --- → 2.1S?
good to phase in 2.1S.
Status: NEW → ASSIGNED
blocking-b2g: 2.1S? → 2.1S+
Update the related testcase base on comment 15
Attachment #8542495 - Attachment is obsolete: true
Attachment #8546377 - Flags: review?(echen)
Comment on attachment 8546377 [details] [diff] [review]
Fix contacts can't be imported when EF_PBR contains more than one record.(V3)

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

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +128,5 @@
> +  let test_data = [
> +    //Record 1.
> +    {
> +      rawData: {
> +          simType: CARD_APPTYPE_SIM,

Nit: use 2 spaces for indentations, here and elsewhere.

@@ +132,5 @@
> +          simType: CARD_APPTYPE_SIM,
> +          contactType: GECKO_CARDCONTACT_TYPE_ADN,
> +          recordId: 1,
> +          adnLike: [{recordId: 1, alphaId: "name", number: "111111"}],
> +          enhancedPhoneBook: false,

Leave |enhancedPhoneBook| undefined for SIM, just remove this attribute.

@@ +134,5 @@
> +          recordId: 1,
> +          adnLike: [{recordId: 1, alphaId: "name", number: "111111"}],
> +          enhancedPhoneBook: false,
> +        },
> +      expectedContact:[{

Nit: space after ':', here and elsewhere.

@@ +140,5 @@
> +          recordId: 1,
> +          alphaId:  "name",
> +          number:   "111111"
> +        }],
> +      comment: "Test read SIM adn contact",

Nit: Please help to move `comment` to first attribute, here and elsewhere

@@ +149,5 @@
> +          simType: CARD_APPTYPE_SIM,
> +          contactType: GECKO_CARDCONTACT_TYPE_FDN,
> +          recordId: 1,
> +          adnLike: [{recordId: 1, alphaId: "name", number: "111111"}],
> +          enhancedPhoneBook: false,

Ditto, leave |enhancedPhoneBook| undefined for SIM.

@@ +169,5 @@
> +          pbrs: [{adn:{fileId: 0x6f3a}, email: {}, anr0: {}}],
> +          adnLike: [{recordId: 1, alphaId: "name", number: "111111"}],
> +          email: "hello@mail.com",
> +          anr:"123456",
> +          enhancedPhoneBook: false,

Ditto, leave |enhancedPhoneBook| undefined for USIM.

@@ +188,5 @@
> +          simType: CARD_APPTYPE_USIM,
> +          contactType: GECKO_CARDCONTACT_TYPE_ADN,
> +          recordId: 1,
> +          pbrs: [{adn:{fileId: 0x6f3a}, email: {}, anr0: {}},
> +                 {adn:{fileId: 0x6f3b}, email: {}, anr0: {}},],

Nit: Remove the ',' between '}' and ']'.

pbrs: [{ .... },
       { .... }],

@@ +193,5 @@
> +          adnLike: [{recordId: 1, alphaId: "name1", number: "111111"},
> +                    {recordId: 2, alphaId: "name2", number: "222222"}],
> +          email: "hello@mail.com",
> +          anr:"123456",
> +          enhancedPhoneBook: false,

Ditto, leave |enhancedPhoneBook| undefined for USIM.

@@ +195,5 @@
> +          email: "hello@mail.com",
> +          anr:"123456",
> +          enhancedPhoneBook: false,
> +      },
> +      expectedContact:[{

Nit: Please use below indention for array, here and elsewhere

expectedContanct: [
  {
    ....
  }, {
    ....
  }
],

@@ +236,5 @@
> +          simType: CARD_APPTYPE_USIM,
> +          contactType: GECKO_CARDCONTACT_TYPE_FDN,
> +          recordId: 1,
> +          adnLike: [{recordId: 1, alphaId: "name", number: "111111"}],
> +          enhancedPhoneBook: false,

Ditto, leave |enhancedPhoneBook| undefined for USIM.

@@ +270,5 @@
> +          simType: CARD_APPTYPE_RUIM,
> +          contactType: GECKO_CARDCONTACT_TYPE_FDN,
> +          recordId: 1,
> +          adnLike: [{recordId: 1, alphaId: "name", number: "111111"}],
> +          enhancedPhoneBook: false,

Ditto, leave |enhancedPhoneBook| undefined for FDN.

@@ +368,3 @@
>      // Override some functions to test.
>      contactHelper.getContactFieldRecordId = function(pbr, contact, field, onsuccess, onerror) {
> +      onsuccess(aTestData.recordId);

|aTestData.recordId| is always 1. It seems we could just constant it as 1, instead of carrying it in test data.

@@ -140,3 @@
>      // Override some functions to test.
>      contactHelper.getContactFieldRecordId = function(pbr, contact, field, onsuccess, onerror) {
> -      onsuccess(1);

Ditto, just keep this.

@@ +389,5 @@
>      let onsuccess = function onsuccess(contacts) {
> +      for (let i = 0; i < contacts.length; i++) {
> +        let contact = contacts[i];
> +        let expectedContact = aExpectedContact[i];
> +        for (let j = 0; j < expectedContact.length; j++) {

|expectedContact| is not an array and it doesn't have length attribute, so the test won't run into this for-loop actually.
Attachment #8546377 - Flags: review?(echen)
Suggestion: you can edit the target milestone which supposedly maps to the expected sprint of landing.
Flags: needinfo?(jdai)
Flags: needinfo?(jdai)
Target Milestone: --- → 2.2 S4 (23jan)
Update the related testcase base on comment 19.
Attachment #8546377 - Attachment is obsolete: true
Attachment #8547396 - Flags: review?(echen)
Comment on attachment 8547396 [details] [diff] [review]
Fix contacts can't be imported when EF_PBR contains more than one record.(V4)

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

We are almost there, please see my comments below. Thank you.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +298,5 @@
> +        email: "hello@mail.com",
> +        anr:"123456",
> +        enhancedPhoneBook: true,
> +      },
> +      expectedContact: [{

Please use below indention for a array contains more than one record,

expectedContact: [
  {
    ....
  }, {
    ....
  }
],

@@ +333,2 @@
>  
> +

nit: remove this blank line.

@@ +340,1 @@
>                                      [0x0, 0x0C, 0x0, 0x0, 0x0]:

I guess you need to rebase for bug 1111990.

@@ +366,5 @@
> +        let contact = contacts[i];
> +        let expectedContact = aExpectedContact[i];
> +        for (let key in contact) {
> +          do_print("check " + key);
> +          do_check_eq(contact[key], expectedContact[key]);

do_check_eq() use `==` to check the equality, https://dxr.mozilla.org/mozilla-central/source/testing/modules/Assert.jsm#225-227.

contact['anr'] is an array, use `==` to check the equality of an array isn't a good way though current configuration pass the check.

@@ +381,3 @@
>    }
>  
> +  for(let i = 0; i < test_data.length; i++) {

nit: one space between `for` and `(`.
Attachment #8547396 - Flags: review?(echen)
Update the related testcase base on comment 22.
Attachment #8547396 - Attachment is obsolete: true
Attachment #8547876 - Flags: review?(echen)
Miss modified code, re-upload correct patch.
Attachment #8547876 - Attachment is obsolete: true
Attachment #8547876 - Flags: review?(echen)
Attachment #8547921 - Flags: review?(echen)
Comment on attachment 8547921 [details] [diff] [review]
Fix contacts can't be imported when EF_PBR contains more than one record.(V6)

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

r=me with below comments addressed. Thank you.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCContactHelper.js
@@ +188,5 @@
> +                  {recordId: 2, alphaId: "name2", number: "222222"}],
> +        email: "hello@mail.com",
> +        anr: "123456",
> +      },
> +      expectedContact: [{

Nit: Please use below indention for an array contains more than one record,

expectedContact: [
  {
    ....
  }, {
    ....
  }
],

@@ -137,3 @@
>                                      [0x20, 0x0, 0x0, 0x0, 0x0]:
>                                      [0x2, 0x0, 0x0, 0x0, 0x0];
> -

Nit: Keep this blank line.

@@ +367,5 @@
> +        let contact = contacts[i];
> +        let expectedContact = aExpectedContact[i];
> +        for (let key in contact) {
> +          do_print("check " + key);
> +          deepEqual(contact[key], expectedContact[key]);

Nit: Probably we could just check contact by |deepEqual(contacts[i], aExpectedContact[i])|.

And since many xpcshell assertions are deprecated [1], let's clean up this in bug 1120805. Thank you.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Assertions_and_logging
Attachment #8547921 - Flags: review?(echen) → review+
This is a generic bug, changing the title accordingly.
Summary: [Contacts][dolphin][1.4][FFOS7715 v2.1] Contacts can't be imported from SIM card to device in Du network → [B2G][RIL] Contacts can't be imported from SIM card to device when EF_PBR contains more than one record
Whiteboard: [sprd367262] → [sprd367262] [FFOS7715 v2.1]
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d11a95fc0335
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
This is marked as blocking v2.1S but never got backported anywhere after it landed on trunk. Do we still want/need this for v2.1S and/or v2.2(r)?
Flags: needinfo?(vchen)
Flags: needinfo?(jocheng)
No, we don't need this one on 2.1s any more

Thanks
Flags: needinfo?(vchen)
blocking-b2g: 2.1S+ → ---
Flags: needinfo?(jocheng)
You need to log in before you can comment on or make changes to this bug.