793310 - Support sms:, tel: and mailto: URI schemes

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Donovan Preston [:fzzzy]]

Right now, to dial the phone on b2g requires using an Activity. There is some precedent in the wild for using the "tel:" url scheme to indicate phone numbers. We should support this protocol in b2g so existing code that dials phone numbers works.

http://www.ietf.org/rfc/rfc2806.txt

Comment 1

[Philipp von Weitershausen [:philikon]]

(In reply to Donovan Preston from comment #0)
> Right now, to dial the phone on b2g requires using an Activity. There is
> some precedent in the wild for using the "tel:" url scheme to indicate phone
> numbers. We should support this protocol in b2g so existing code that dials
> phone numbers works.

Note that using a tel: protocol link rarely actually results in *dialing* that number. It typically just triggers the dialer with that number pre-entered, so the user still sees which number gets dialed and has to consciously make that decision.

So yes, if we triggered a dialer-bound activity from tel: URLs, that'd be great.

Comment 2

[Andrew Overholt [:overholt]]

This seems like a new feature and while it'd be great to have, if it doesn't make the feature freeze, I suspect we'll have to cut it.

Andrea, can you take this?

Comment 3

[Donovan Preston [:fzzzy]]

It appears there is already a gaia bug which covers the same functionality. This seems to me more like a platform issue than a gaia issue, but I'm not sure.

https://github.com/mozilla-b2g/gaia/issues/2307

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Andrew Overholt [:overholt] from comment #2)
> This seems like a new feature and while it'd be great to have, if it doesn't
> make the feature freeze, I suspect we'll have to cut it.

I guess I'm confused why you marked it blocking+ then?

Comment 5

[Andrew Overholt [:overholt]]

(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> (In reply to Andrew Overholt [:overholt] from comment #2)
> > This seems like a new feature and while it'd be great to have, if it doesn't
> > make the feature freeze, I suspect we'll have to cut it.
> 
> I guess I'm confused why you marked it blocking+ then?

To give it until the feature freeze to land.  Jonas was concerned too much of the existing mobile web uses this so we didn't want to outright - it.

Comment 6

[Andrew Overholt [:overholt]]

Corresponding issue:  https://github.com/mozilla-b2g/gaia/issues/2307.

Comment 7

[Andrew Overholt [:overholt]]

David Veditz mentioned on IRC that we need a security review for this.

hub also mentioned that we should ensure we avoid things like [1].

[1]
http://9to5google.com/2012/09/25/major-samsung-galaxy-exploit-hard-resets-a-device-by-just-visiting-a-website/

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch 1

Is there a way to have mochitest for b2g?

Comment 9

[Donovan Preston [:fzzzy]]

Yes, the correct directory structure and makefile usually used for mochitests can be added. I don't know the technical details but I do know it can work.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

See also bug 794034 for some problems that we need to protect against in our tel: implementation.

Basically, we should reject any telephony numbers which include * or #.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 2

numbers with * and # are filtered out.

Comment 12

[cloos]

> numbers with * and # are filtered out.

Notwithstanding bug 794034, there are valid reasons to have tel: uris which include # and * in their dial strings.

ITAD numbers come to mind, (tel:8463*368 will, if the dialplan has itad support, dial my time-of-day test number).  And something which mozilla ought to *want* to support.

There are others.

The filtering should be more specific.

Otherwise, cool work!

Comment 13

[cloos]

Oh, and of course one would prefer, from the users’ perspective, were the uri to be passed to the dialer application such that an explicit act would be required to actually dial it.

And avoiding <frame src="tel:"/> altogether seems reasonable. (Or should that embed one’s dialer app in a frame?)

And it *might* be OK to ignore tel: uris which have BOTH * and #.  Ie, use && rather than || between the indexOf calls.  Presuming the percent escapes have been undone by then, of course.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

To keep things simple for now, we should stick to filtering out numbers with # and *. It's a much easier task to change that filter once we've done the initial landing. That way we'll also be able to broaden the discussion to include people that know our RIL internals.

Comment 15

[Philipp von Weitershausen [:philikon]]

(In reply to cloos from comment #13)
> Oh, and of course one would prefer, from the users’ perspective, were the
> uri to be passed to the dialer application such that an explicit act would
> be required to actually dial it.

I would argue that this will be absolutely necessary from a privacy/security point of view. This is somewhat outside the scope of this bug, though, since it's in the hands of the Dialer application in Gaia to react to the Web Activity.

> And avoiding <frame src="tel:"/> altogether seems reasonable. (Or should
> that embed one’s dialer app in a frame?)

This should not be possible. One should think of the tel: URI scheme like the mailto: scheme. <iframe src="mailto:[e-mail]"> should not be possible either. (On top of that, websites should never be able to embed other apps like this.)

> And it *might* be OK to ignore tel: uris which have BOTH * and #. Ie, use
> && rather than || between the indexOf calls.

Nope. We will HAVE TO ignore any tel: uris with any * or # as they could be MWI / USSD codes. #111# is a valid MWI code AFAIK.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Make sure to set the nsIProtocolHandler::URI_DOES_NOT_RETURN_DATA flag on the protocol. That will ensure that <iframe src="tel:..."> doesn't work.

Comment 17

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665284 [details] [diff] [review]
patch 2

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

This looks great! I just have a bunch of smaller notes on the organization of the code and one bigger note about the lack of unit tests. It's almost r+, but I think I'd like to see the patch again before it's committed. Thanks!

::: b2g/chrome/content/shell.js
@@ +372,5 @@
> +
> +    switch (message.name) {
> +      case 'content-handler':
> +        name = 'view';
> +        break;

Instead of sending different messages and translating them into slightly different activity names (which could be done more elegantly with a simple map object like {"content-handler": "view", "dial-handler": "dial", etc.}), could we perhaps send one IPC message with different contents? E.g. the XPCOM components in the child process would send something like

  cpmm.sendAsyncMessage("launch-activity", {
    URI: aURI.spec,
    type: "mail"
  });

And then here you just do

  new MozActivity({name: message.data.type,
                   data: message.data});

@@ +392,3 @@
>        return;
> +
> +    let data = message.json;

`message.json` is deprecated in favour of `message.data`.

::: b2g/components/MailtoProtocolHandler.js
@@ +5,5 @@
> +"use strict";
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

Hint: this can be done in one line:

  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

@@ +11,5 @@
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +
> +XPCOMUtils.defineLazyGetter(this, "cpmm", function() {
> +  return Cc["@mozilla.org/childprocessmessagemanager;1"]
> +           .getService(Ci.nsIMessageSender);

Please use XPCOMUtils.defineLazyServiceGetter. (ContentHandler.js is a terrible example here.)

All of the *ProtocolHandler.js files share quite a bit of boiler plate like this, and they're very short each. I would recommend folding them into one file (together with the YouTube one that already exists).

@@ +20,5 @@
> +
> +MailtoProtocolHandler.prototype = {
> +  get scheme() "mailto",
> +
> +  get defaultPort() -1,

There should be no need for these getters, you can simply do

  scheme: "mailto",
  defaultPort: -1,
  protocolFlags: Ci.nsIProtocolHandler.URI_NORELATIVE |
                 Ci.nsIProtocolHandler.URI_NOAUTH |
                 Ci.nsIProtocolHandler.URI_LOADABLE_BY_ANYONE,
  allowPort: false

XPConnect will make sure that the attributes marked as readonly in the IDL can't be changed.

@@ +31,5 @@
> +
> +  newURI: function Proto_newURI(aSpec, aOriginCharset) {
> +    let uri = Cc["@mozilla.org/network/simple-uri;1"].createInstance(Ci.nsIURI);
> +    uri.spec = aSpec;
> +    return uri;

I think you can just do

  return Services.io.newURI(aSpec, null, null);

@@ +39,5 @@
> +    cpmm.sendAsyncMessage("mail-handler", {
> +      URI: aURI.spec,
> +      type: "mail" });
> +
> +    throw Components.results.NS_ERROR_ILLEGAL_VALUE;

This doesn't make sense to me. We send the async message, but then throw an error?

Also, when throwing an XPCOM exception, you should do

  throw Components.Exception("Your error message here', Cr.NS_ERROR_ILLEGAL_VALUE);

::: b2g/components/SmsProtocolHandler.js
@@ +48,5 @@
> +
> +    if (number) {
> +      cpmm.sendAsyncMessage("sms-handler", {
> +        number: number,
> +        type: "websms/sms" });

Why not just "sms"?

::: b2g/components/TelURIParser.jsm
@@ +9,5 @@
> +/**
> + * Singleton providing functionality for parsing tel: and sms: URIs
> + */
> +let TelURIParser = {
> +  parseURI: function(scheme, uri) {

I'm not 100% sure why this function is a method on an object. There's no state that needs saving, so we could just export the function. I'm also not sure why the function isn't simply included in the only file where it's currently used (TelProtocolHandler.js).

That aside, this function seems like a perfect candidate for some unit-testing, and for that having it in a JSM rather than an XPCOM component file is handy. I would definitely like to see some xpcshell tests for this, *especially* for all the failure cases you're handling here.

@@ +12,5 @@
> +let TelURIParser = {
> +  parseURI: function(scheme, uri) {
> +    // Bug 794034
> +    if (uri.indexOf('*') != -1 || uri.indexOf('#') != -1)
> +      return null;

Nit: always need braces (please fix this here and everywhere else).

Also, perhaps you could put a sentence or two in that comment in addition to referring to the bug. That way people who read the code don't have to jump back and forth between the code and Bugzilla. E.g.

  // Ignore MWI and USSD codes. See 794034.

@@ +15,5 @@
> +    if (uri.indexOf('*') != -1 || uri.indexOf('#') != -1)
> +      return null;
> +
> +    // https://www.ietf.org/rfc/rfc2806.txt
> +    var subscriber = uri.substring((scheme + ':').length, uri.length);

I find the string.slice() method handier for this because you can omit the end-range argument:

  let subscriber = uri.slice(scheme.length + 1);

(I also changed your `var` to `let` and simplified the length computation a bit.)

@@ +73,5 @@
> +          else
> +            break;
> +        }
> +
> +        // FIXME: ignore subaddress

Please file a follow-up bug and refer to the bug in this comment.

@@ +103,5 @@
> +        number = subscriber.substring(pos, subscriber.length) + number;
> +      }
> +    }
> +
> +    return number.length ? number : null;

return number || null;

Comment 18

[Andrea Marchesini [:baku]]

>   cpmm.sendAsyncMessage("launch-activity", {
>     URI: aURI.spec,
>     type: "mail"
>   });

I wrote that code because I didn't know how many existing sendAsyncMessage("content-handler", ...); we have. I don't want to break existing code.

>   return Services.io.newURI(aSpec, null, null);

Using this code:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService2.newURI]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: file:///home/baku/Sources/m/tel/build/dist/bin/components/SmsProtocolHandler.js :: Proto_newURI :: line 39"  data: no]


> This doesn't make sense to me. We send the async message, but then throw an
> error?

We throw an exception because newChannel() must return a channel. But we do not want to create a channel: our propose is to send a async message to the shell.js. This code is taken from youtube protocol handler. Maybe sicking has some suggestion about this.

> > +    if (number) {
> > +      cpmm.sendAsyncMessage("sms-handler", {
> > +        number: number,
> > +        type: "websms/sms" });
> 
> Why not just "sms"?

websms/sms is the type accepted by the sms app. With 'sms' nothing happens, the message is ignored.

> I'm not 100% sure why this function is a method on an object. There's no
> state that needs saving, so we could just export the function. I'm also not
> sure why the function isn't simply included in the only file where it's
> currently used (TelProtocolHandler.js).

It's used also by SmsProtocolHandler.js. Then the reason why it's an object is because maybe we can add some better parser in the future.

Thank you for this review!

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch 2b

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch 3

xpcshell test added:
make -C BUILD_PATH/b2g/components/test xpcshell-tests

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to Andrea Marchesini (:baku) from comment #18)
> >   cpmm.sendAsyncMessage("launch-activity", {
> >     URI: aURI.spec,
> >     type: "mail"
> >   });
> 
> I wrote that code because I didn't know how many existing
> sendAsyncMessage("content-handler", ...); we have. I don't want to break
> existing code.

As a message name, it's only ever used in b2g: https://mxr.mozilla.org/mozilla-central/search?string=content-handler

> >   return Services.io.newURI(aSpec, null, null);
> 
> Using this code:
> 
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "Component returned failure code: 0x804b000a
> (NS_ERROR_MALFORMED_URI) [nsIIOService2.newURI]"  nsresult: "0x804b000a
> (NS_ERROR_MALFORMED_URI)"  location: "JS frame ::
> file:///home/baku/Sources/m/tel/build/dist/bin/components/SmsProtocolHandler.
> js :: Proto_newURI :: line 39"  data: no]

Ok. Interesting! Let's keep your code then :)

> > This doesn't make sense to me. We send the async message, but then throw an
> > error?
> 
> We throw an exception because newChannel() must return a channel. But we do
> not want to create a channel: our propose is to send a async message to the
> shell.js. This code is taken from youtube protocol handler. Maybe sicking
> has some suggestion about this.

Ah ok. That makes sense. Please do throw a Components.Exceptions instance, though. Just throwing an error code creates a really crappy JS backtrace.

> > > +    if (number) {
> > > +      cpmm.sendAsyncMessage("sms-handler", {
> > > +        number: number,
> > > +        type: "websms/sms" });
> > 
> > Why not just "sms"?
> 
> websms/sms is the type accepted by the sms app. With 'sms' nothing happens,
> the message is ignored.

Ok.

> > I'm not 100% sure why this function is a method on an object. There's no
> > state that needs saving, so we could just export the function. I'm also not
> > sure why the function isn't simply included in the only file where it's
> > currently used (TelProtocolHandler.js).
> 
> It's used also by SmsProtocolHandler.js. Then the reason why it's an object
> is because maybe we can add some better parser in the future.

I don't think that's a good enough reason, but I'm not going to fight you for it :). It's a minor detail for sure.

Comment 22

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #21)
> > > This doesn't make sense to me. We send the async message, but then throw an
> > > error?
> > 
> > We throw an exception because newChannel() must return a channel. But we do
> > not want to create a channel: our propose is to send a async message to the
> > shell.js. This code is taken from youtube protocol handler. Maybe sicking
> > has some suggestion about this.
> 
> Ah ok. That makes sense. Please do throw a Components.Exceptions instance,
> though. Just throwing an error code creates a really crappy JS backtrace.

I guess we're throwing to satisfy an internal API, not for a JS API consumer. So if this code works, I'm fine with it.

Comment 23

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665860 [details] [diff] [review]
patch 3

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

I still think we could easily collapse all ProtocolHandlers into one file, and just use the 'content-handler' message for all of these handlers. But I won't block the review on this. r=me with the nits below addressed. Thanks!

::: b2g/components/test/unit/test_bug793310.js
@@ +1,1 @@
> +function run_test() {

Nit: Public-domain boilerplate at the top of the file: https://www.mozilla.org/MPL/boilerplate-1.1/pd-c

@@ +1,5 @@
> +function run_test() {
> +  try {
> +    Components.utils.import("resource:///modules/TelURIParser.jsm")
> +  } catch(e) {
> +    return;

Hmm, should we silently skip this test if, say, TelURIParser.jsm contained a syntax error? Please get rid of the try/catch.

@@ +5,5 @@
> +    return;
> +  }
> +
> +  // global-phone-number
> +  do_check_true(TelURIParser.parseURI('tel', 'tel:+1234') == '+1234');

use do_check_eq(a, b) in these cases.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch 3a

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm getting merge issues when trying to land this patch.

Andrea: Can you update it against the latest mozilla-inbound

Comment 26

[Andrea Marchesini [:baku]]

Attachment: patch 3b

rebased

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff86ec766232

Comment 28

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/ff86ec766232

Should this have a test?

Comment 29

[Andrea Marchesini [:baku]]

there is just a xpcshell test for TelURIParser, then I wrote a test html page for testing. No more than this.

Comment 30

[Mounir Lamouri (:mounir)]

We could test that by checking that Web Activities code is actually called but that's quite some work. However, we should try to test as much stuff as we can. The simplest solution would have been to get Gaia tests...