Closed
Bug 1119209
Opened 10 years ago
Closed 7 years ago
[FFOS7715 V2.1]Parse the xml document with same_appid_different_provider-id, in settings we only get one wrong apn in apn list
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: Jinghua.Xing, Unassigned)
References
Details
Attachments
(3 files, 1 obsolete file)
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•10 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•10 years ago
|
||
Hi Gabriele,
Can you help me check this issue?
Thank you
Flags: needinfo?(gsvelto)
Reporter | ||
Comment 3•10 years ago
|
||
felash:
Can you help me check this?
Thank you
Flags: needinfo?(felash)
Reporter | ||
Comment 4•10 years ago
|
||
The issue of this case is mainly the second and the third one in comment1
Comment 5•10 years ago
|
||
Hey Jessica, can you help finding a good assignee for this APN-related issue?
Flags: needinfo?(felash) → needinfo?(jjong)
Updated•10 years ago
|
Component: Gaia::Wappush → RIL
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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)
Updated•10 years ago
|
Flags: needinfo?(gsvelto)
Reporter | ||
Comment 8•10 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•10 years ago
|
||
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 10•10 years ago
|
||
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 11•10 years ago
|
||
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•10 years ago
|
||
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•10 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.
Updated•10 years ago
|
Attachment #8547442 -
Attachment is obsolete: true
Comment 14•10 years ago
|
||
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-
Reporter | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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+
Comment 17•7 years ago
|
||
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•