If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

NEW
Unassigned

Status

Firefox OS
RIL
--
major
3 years ago
3 years ago

People

(Reporter: jinghua.xing, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8545899 [details]
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"]
}]
(Reporter)

Comment 1

3 years ago
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.
Summary: [FFOS7715 V2.1]Parse the xml document error for oma cp message → [FFOS7715 V2.1]Parse the xml document with same_appid_different_provider-id, in settings we only get one wrong apn in apn list
(Reporter)

Comment 2

3 years ago
Hi Gabriele, 

Can you help me check this issue?

Thank you
Flags: needinfo?(gsvelto)
(Reporter)

Comment 3

3 years ago
felash:

Can you help me check this?

Thank you
Flags: needinfo?(felash)
(Reporter)

Comment 4

3 years ago
The issue of this case is mainly the second and the third one in comment1
Hey Jessica, can you help finding a good assignee for this APN-related issue?
Flags: needinfo?(felash) → needinfo?(jjong)
Component: Gaia::Wappush → RIL
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.
Flags: needinfo?(jjong) → needinfo?(arthur.chen)

Updated

3 years ago
Severity: normal → major
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.
Flags: needinfo?(arthur.chen) → needinfo?(Jinghua.Xing)
Flags: needinfo?(gsvelto)
(Reporter)

Comment 8

3 years ago
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.
Flags: needinfo?(Jinghua.Xing)
(Reporter)

Comment 9

3 years ago
Created attachment 8547442 [details] [diff] [review]
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.
Attachment #8547442 - Flags: review?(arthur.chen)
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.
Attachment #8547442 - Flags: review?(arthur.chen) → review?(gsvelto)
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.
Attachment #8547442 - Flags: review?(gsvelto) → review-
(Reporter)

Comment 12

3 years ago
Created attachment 8548815 [details] [review]
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.
Attachment #8548815 - Flags: review?(gsvelto)
(Reporter)

Comment 13

3 years ago
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.
Attachment #8547442 - Attachment is obsolete: true
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.
Attachment #8548815 - Flags: review?(gsvelto) → review-

Updated

3 years ago
Blocks: 1123554
(Reporter)

Comment 15

3 years ago
Created attachment 8551628 [details] [review]
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
Attachment #8551628 - Flags: review?(gsvelto)
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.
Attachment #8551628 - Flags: review?(gsvelto) → review+
You need to log in before you can comment on or make changes to this bug.