1119209 - [FFOS7715 V2.1]Parse the xml document with same_appid_different_provider-id, in settings we only get one wrong apn in apn list

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[jinghua.xing]

Attachment: 01049_same_appid_different_provider-id.xml

As in the document in the attachment, it contains two apn. One is 

{  NAPID: "cmccwap_NAPID",
   TO-NAPID:["cmccwap_NAPID"],
   apn:"cmwap_01049",
   carrier:"cmccwap",
   port:"9201",
   proxy:"10.0.0.172",
   types:["default"]
}. 

And the other one is 

{  NAPID: "cmccwap_NAPID",
   TO-NAPID:["cmccwap_NAPID"],
   apn:"cmwap_01049",
   carrier:"cmccwap",
   port:"9203",
   proxy:"10.0.0.172",
   types:["default"]
}

But in FFOS, after we use parsedProvisioningDoc.getApns() function to get the apns in the document, we got:
[{  NAPID: "cmccwap_NAPID",
   TO-NAPID:["cmccwap_NAPID"],
   apn:"cmwap_01049",
   carrier:"cmccwap",
   port:"9203",
   proxy:"10.0.0.172",
   types:["default"]
}
{  NAPID: "cmccwap_NAPID",
   apn:"cmwap_01049",
   carrier:"cmccwap",
   types:["default"]
}]

Comment 1

[jinghua.xing]

There are three problem in this case：

1、This case contains two wsp access point，but we have not parse 		
<parm name="PROVIDER-ID" value="460002"/>

2、When we use parsedProvisioningDoc.getApns() function，the this._proxies array contains two proxy，one for the first apn, and the other for the second apn. But as the two apns have same NAPID, so we add the two proxy both to the first apn, and no one added to the second apn.

3、We added the two apns into the the 'ril.data.cp.apns' settings and put the second apn into 'ril.data.apnSettings' settings, then we go to settings to look at the apn list. We found only the second apn is added into the apn list and we can't find the first one. Only after we press the reset to default button, the two apns are showed on the apn list together. I think the first time we have not showed the apns stored in 'ril.data.apnSettings' settings in apn list.

Comment 2

[jinghua.xing]

Hi Gabriele, 

Can you help me check this issue?

Thank you

Comment 3

[jinghua.xing]

felash:

Can you help me check this?

Thank you

Comment 4

[jinghua.xing]

The issue of this case is mainly the second and the third one in comment1

Comment 5

[Julien Wajsberg [:julienw]]

Hey Jessica, can you help finding a good assignee for this APN-related issue?

Comment 6

[Jessica Jong [:jessica] (inactive)]

It looks related to the parsing of WAP provisioning doc, I think Jose (:jaoo) was the one mantaining that part, but he is not into this anymore, right? Arthur, can you help? Thanks.

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Jinghua,

May I know the expected behavior in your mind? When we set multiple apns to ril.data.apnSettings gecko only applies one of them (where I believe it's the last one). We will fix the bug of not showing the received apns in the first place. And allowing users to select the apn when storing the CP message should be considered as a new feature.

Comment 8

[jinghua.xing]

The bug of not showing the received apns is one cause of this issue, 

And the other cause is we didn't parse the document to get apns correctly as I describe in the Description. The document contains two APPLICATION characteristics and two PXLOGICAL characteristics, so it will first parse two apns and two proxy in ParsedProvisioningDoc.parse() and push the two proxy into this._proxies. As the two APPLICATION characteristics contains different TO-PROXY parm and the same as the PROXY-ID parm in the PXLOGICAL characteristics, so the two proxy parsed from the two PXLOGICAL characteristics should respectively corresponding to the two apns. As the TO-NAPID in the two  PXLOGICAL is same, so the NAPID of two apns and the 'TO-NAPID' in two proxy are all same.

When we call getApns() function, according to below code, we added the two proxy both to the first apn in this._apns, so one of the apns parsed from the document is wrong. 
>      for (var i = 0; i < this._proxies.length; i++) {
>        var proxy = this._proxies[i];
>        for (var j = 0; j < proxy['TO-NAPID'].length; j++) {
>          var TO_NAPID = proxy['TO-NAPID'][j];
>          for (var k = 0; k < this._apns.length; k++) {
>            var apn = this._apns[k];
>            if (TO_NAPID === apn.NAPID) {
>              addProperties(proxy, apn);
>              break;
>            }
>          }
>        } 

Arthur, Can you really get what I mean? Thank you.

Comment 9

[jinghua.xing]

Attachment: parse_same_napid.patch

Arthur,

I have tried to make a patch for the parsing error in WAPPUSH app. Can you help me to review it?

Thank you.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8547442 [details] [diff] [review]
parse_same_napid.patch

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

I'll take the review, I'll do it ASAP.

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8547442 [details] [diff] [review]
parse_same_napid.patch

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

I think I understand what you're trying to do but this patch doesn't work because it's missing the addProperties() function. You should also add a unit-test to cover this scenario to the tests that are in this file:

https://github.com/mozilla-b2g/gaia/blob/658f90d20962dae4813cc46a5c5a9bb4b7a43383/apps/wappush/test/unit/parsed_doc_test.js

Ask for review again once both issues are fixed.

::: apps/wappush/js/parsed_doc.js
@@ +305,5 @@
> +                  var proxy = this._proxies[p];
> +                  for (var q = 0; q < proxy['TO-NAPID'].length; q++) {
> +                    var TO_NAPID = proxy['TO-NAPID'][q];
> +                    if (TO_NAPID === apn.NAPID) {
> +                      addProperties(proxy, apn);

The function addProprties() is not defined here, you should add it back.

Comment 12

[jinghua.xing]

Attachment: pull request for this issue

Hi Gabriele, 

I have made a pull request for this issue. And I think we can add the proxy to the apn once it parsed from NAPDEF characteristic as it was parsed according to the proxy we just parsed out from the PXPHYSICAL characteristic.

Please help to review this patch. Thank you.

Comment 13

[jinghua.xing]

There are two two APPLICATION characteristics and each one has a TO-PROXY parameter and OMA-WAP-TS-ProvCont-V1_1-20090728-A protocol says "The TO-PROXY parameter refers to a logical proxy with a matching PROXY-ID. " But there is no proxy in the second apn parsed from the document. So the apns parsed from the document is wrong.

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8548815 [details] [review]
pull request for this issue

The code is OK and thanks for adding the test but there's two problems with it: the test contains a syntax error and it's also not properly testing for the issue here. If I remove your new code and only add the new test it still passes, but it shouldn't. You have to verify that the APNs contain the correct proxy in the tests, just checking the number of APNs available is not enough. See my comments on the PR for more details.

Comment 15

[jinghua.xing]

Attachment: pull request for this issue V2

Hi Gabriele, 

Thank you so much for your kindly review. I have corrected the code with your advices. Please help to review again.

Thank you

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8551628 [details] [review]
pull request for this issue V2

Looks good to me, but please change the bug review comment from r=Gabriele to r=gsvelto which is my IRC nick. Once the try run will turn green I'll merge your PR.

Comment 17

[BMO Automation]

Firefox OS is not being worked on