[Settings] Call support of Help menu don't link with call app

RESOLVED FIXED in Firefox 23

Status

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: leo.bugzilla.gaia, Assigned: arthurcc)

Tracking

unspecified
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: testrun 5.1)

Attachments

(1 attachment, 2 obsolete attachments)

1. Title : Call support of Help menu don't link with call app
2. Precondition : Settings - Help - Call support 
3. Tester's Action : touch number(*8486)
4. Detailed Symptom (ENG.) : No action. But when touch 1058, go to dialer screen. It's differnt action.
6. Expected : 
7. Reproducibility: Y
1)Frequency Rate : 100%
8. Gaia revision : a03f7b532e9998e646d55f93a0fc03a04d7ca7d9
Call app is still not spawned and number auto population is unsuccessful when user selects the touch number (*8486) from the "Call Support" Help menu.

Unagi Device

Environmental  Variables:
Build ID: 20130221070203
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/effe37a77f18
Gaia: bb633c6f2deb829b2f3d5b9e7bac7fa24467d02a
Whiteboard: testrun 5.1
Call app is still not spawned and number auto population is unsuccessful when user selects the touch number (*8486) from the "Call Support" Help menu during testrun 5.1

Unagi Device

Environmental  Variables:
Build ID: 20130225070200
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3a5a27992a75
Gaia: 5691a16fff8e1403c75ed9d6f3a443b7e58198c6
blocking-b2g: tef? → leo?
The "call support" button in the phone should actually call support.
blocking-b2g: leo? → leo+
Assignee: nobody → arthur.chen
The number *8486 is ignored by gecko. This bug was regressed by bug 797034. There should be a better way examining invalid phone numbers. 

John, is there any plan of improving it?
Flags: needinfo?(jschoenick)
Bug 794034 was a rushed fix for this, then bug 797034 was opened to follow up and improve it, which is still open 

I'm not very involved with B2G work and probably wouldn't be the guy to fix the latter, unfortunately :(
Flags: needinfo?(jschoenick)
Posted patch Patch v1 (obsolete) — Splinter Review
Boris, this change improves the method of ignoring MMI/USSD codes. Could you help review the change? Thanks!
Attachment #724314 - Flags: review?(bzbarsky)
Attachment #724314 - Attachment is patch: true
Comment on attachment 724314 [details] [diff] [review]
Patch v1

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

::: b2g/components/TelURIParser.jsm
@@ +109,5 @@
>        }
>      }
>  
> +    // Ignore MWI and USSD codes. See 794034.
> +    if (number.match(/^[#\*].*#$/)) {

This would not block *#*#7780#*#* which is a factory reset on some android devices.

See https://bugzilla.mozilla.org/show_bug.cgi?id=794034#c20
Comment on attachment 724314 [details] [diff] [review]
Patch v1

I can't review this, unfortunately, because I have no idea what the goal of this code is on b2g and hence which things it needs to block...
Attachment #724314 - Flags: review?(bzbarsky)
I think we can consider this bug invalid -
1 > There is bug 797034 to track a better method of parsing/handling bad TEL formats
and
2 > as indicated in Bug 846197 the information displayed currently are for vivo and the device should not ship with this. This page should be customizable by carriers and it should be recommended that they use only appropriate number formats.

setting leo? to be revisited by triage
blocking-b2g: leo+ → leo?
Triage group agrees that we cannot ship in the current state.

We either need to:

1. not present unsupported tel: numbers as clickable

or 

2. support USSD in tel: links

Do we need a new bug to track #2?

Daniel: Does Vivo require USSD help number? Is that number the right one?
blocking-b2g: leo? → leo+
Flags: needinfo?(dcoloma)
Backend does support USSD. Blocking USSD here was preventing users from clicking on a link with a factory reset code by mistake (although they still need to press the dial button). The question is there seems no exact pattern of such numbers because it is determined by the carrier. Thus after the patch of bug 794034, all numbers with '*' or '#' get blocked. It seems like there are two options:

1. Suggest carriers not using numbers with '*' or '#'.
2. Only block numbers of USSD format plus numbers of the pattern *#*#7780#*#*.
not sure if :dpv can provide more input here?
Flags: needinfo?(dpv)
Hi, 

Not sure if it could help this. 

Sometimes carriers use MSISDNs like (*number, f.i. "*86", "*123" are used for voice mailbox). Not sure about more complex structures. 

I think at least we should allow strings such as "*" + "number". 

Thks!
David
Flags: needinfo?(dpv)
Let's block numbers starting with '#' or '*' and ending with '#' or '*'. Does it make sense?
(In reply to Arthur Chen (OOO 3/28 ~ 3/29) [:arthurcc] from comment #14)
> Let's block numbers starting with '#' or '*' and ending with '#' or '*'.
> Does it make sense?

##7764726 – Hidden service menu for Motorola Droid

http://www.redmondpie.com/hidden-android-secret-codes-for-samsung-htc-motorola-sony-lg-and-other-devices/

It seems like a whitelist approach of allowed patterns would be safer, e.g. c13
Posted patch Patch v2 (obsolete) — Splinter Review
Ignore numbers containing '*' or '#' except for ones in the format of "*" and "#" following by numbers. John, could you help review it?
Attachment #724314 - Attachment is obsolete: true
Attachment #731789 - Flags: review?(jschoenick)
Flags: needinfo?(dcoloma)
Comment on attachment 731789 [details] [diff] [review]
Patch v2

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

::: b2g/components/TelURIParser.jsm
@@ +109,5 @@
>        }
>      }
>  
> +    // Ignore MWI and USSD codes. See 794034.
> +    if (number.match(/[#\*]/) && !number.match(/^[#\*]\d+$/)) {

Does the number potentially contain other characters (spaces, commas) at this point?

I don't see any immediately abusive codes this allows (though there's nothing technically stopping the carrier from having *1234 be a device reset)

I'm also not a peer on any of the android stuff, and am mostly working off of google here, so somebody on the android team should probably give the final r+ here.
Attachment #731789 - Flags: review?(jschoenick) → feedback+
Comment on attachment 731789 [details] [diff] [review]
Patch v2

Philipp, I'm not sure if you are able to review this change, or maybe you can redirect it to the right one? Thanks!
Attachment #731789 - Flags: review?(philipp)
Arthur, it's unlikely philikon will be able to review this soon. Who else can review this patch?
Comment on attachment 731789 [details] [diff] [review]
Patch v2

Blake, could you help review this change? Thanks!
Attachment #731789 - Flags: review?(mrbkap)
Comment on attachment 731789 [details] [diff] [review]
Patch v2

I'm sorry, I don't know nearly enough about special numbers to review this. Gregor might have a better idea.
Attachment #731789 - Flags: review?(mrbkap)
Comment on attachment 731789 [details] [diff] [review]
Patch v2

Gregor, could you help review the change or add qualified reviewers? Thanks.
Attachment #731789 - Flags: review?(anygregor)
(In reply to Arthur Chen [:arthurcc] from comment #22)
> Comment on attachment 731789 [details] [diff] [review]
> Patch v2
> 
> Gregor, could you help review the change or add qualified reviewers? Thanks.

Can you add tests here?
https://mxr.mozilla.org/mozilla-central/source/b2g/components/test/unit/test_bug793310.js
Posted patch Patch v3Splinter Review
New patch with tests added.
Attachment #731789 - Attachment is obsolete: true
Attachment #731789 - Flags: review?(philipp)
Attachment #731789 - Flags: review?(anygregor)
Attachment #736171 - Flags: review?(anygregor)
Comment on attachment 736171 [details] [diff] [review]
Patch v3

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

Thanks!
Attachment #736171 - Flags: review?(anygregor) → review+
Thanks Gregor!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9ee76e6261e6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
Blocks: 863868
Component: Gaia::Settings → General
Flags: in-moztrap?
Creating a new test case.
- https://moztrap.mozilla.org/manage/case/8469/
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.