914182 - WebTelephony: Hide Telephony API from regular Web pages

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Masatoshi Kimura [:emk]]

I don't think we can ship this yet.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: patch

Vicamo, please review Telephony bits.
Kyle, please review the interface change.

Green except a known failure:
https://tbpl.mozilla.org/?tree=Try&rev=14ed6cb1d3d7

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 801883 [details] [diff] [review]
patch

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

Thank you Masatoshi.

r=me

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 801883 [details] [diff] [review]
patch

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

Thank you.

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: patch v2

I found a better function to determine the availability of Telephony API. We don't have to add a pref to complicate things further. Also this patch will reduce the use of the compile-time option.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4bb73289d110

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 803084 [details] [diff] [review]
patch v2

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

Today I have a problem to deal with.  In B2G tablet build, we runs master Gaia and master Gecko.  Gaia expects |navigator.mozTelephony == undefined| on tablet builds and Gaia dialer app certainly has "telephony" permission.  This patch simply overthrows one possible solution to this problem.  See bug 911684.

Besides, why is the existence of a certain navigator attribute controlled by the app?  Should we treat it a bug that when an app happens to be able to claim the permission to a certain API set which we want to explicitly disable on a platform/build, and navigator does allow it?  I know the pre-condition itself is a serious security bug, but will its result be a bug, too?

Comment 6

[Masatoshi Kimura [:emk]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> Today I have a problem to deal with.  In B2G tablet build, we runs master
> Gaia and master Gecko.  Gaia expects |navigator.mozTelephony == undefined|
> on tablet builds and Gaia dialer app certainly has "telephony" permission. 
> This patch simply overthrows one possible solution to this problem.  See bug
> 911684.

Actually this patch will help to fix bug 911684. You can change Navigator::HasTelephonySupport function whatever you like (for example, always return false on desktop and tablet). This is much more flexible than a sigle pref.

> Besides, why is the existence of a certain navigator attribute controlled by
> the app?

This patch doesn't change the behavior of a navigator attribute. This patch will just change the existence of global objects (Telephony, TelephonyCall, TelephonyCallGroup, CallEvent). These objects will be present iff navigator.mozTelephony is present. There is no point exposing these global objects when the navigator property is not present.

Comment 7

[Masatoshi Kimura [:emk]]

FYI, I didn't write Navigator::HasTelephonySupport function.
> Besides, why is the existence of a certain navigator attribute controlled by
> the app?
So this behavior was introduced by someone wrote Navigator::HasTelephonySupport function, not me.

Comment 8

[Masatoshi Kimura [:emk]]

Per hg annotate, :bz write the function and :bent and :sicking reviewed.

Comment 9

[Masatoshi Kimura [:emk]]

Changing the subject based on what I really meant to.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The pre-webidl code exposed the property unconditionally #ifdef MOZ_B2G_RIL.  Attempting to get the property would check the "telephony" permission and if granted return the object.  

The post-webidl code does something similar: the mozTelephony pref on Navigator is #ifdef MOZ_B2G_RIL and even then only defined in globals with the permission.  Basically, I tried to not rock the boat too much.  ;)  The difference was that lack of permission means undefined, not null, after the WebIDL patch.

> Gaia expects |navigator.mozTelephony == undefined| 

The attached patch does not change the behavior of navigator.mozTelephony.

> Should we treat it a bug that when an app happens to be able to claim the permission to
> a certain API set which we want to explicitly disable on a platform/build,

I'm not sure what this is asking...  .mozTelephony is always undefined #ifndef MOZ_B2G_RIL, no matter what HasTelephonySupport() returns.

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 803084 [details] [diff] [review]
patch v2

This patch does not change the behavior of navigator.mozTelephony. If you are not comfortable with the current navigator.mozTelephony, please file a bug about that.
I don't care much about the behavior in apps as long as Telephony API is hidden from regular Web pages. Note that the v1 patch didn't hide the API from Web pages on B2G. It's impossible to distinguish apps from Web pages by using a simple pref. Again, please file a separate bug and change Navigator::HasTelephonySupport as you wish.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 803084 [details] [diff] [review]
patch v2

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

::: dom/base/Navigator.h
@@ +87,2 @@
>  class CellBroadcast;
>  #endif

This might need a rebase on bug 910568, which is still in b2g-inbound though.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Hi Masatoshi, could you also confirm we need to apply the same thing to other permission-required DOM APIs like MobileConnection, Voicemail, MobileMessage, CellBroadcast, etc., as well?

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Masatoshi Kimura [:emk] from comment #8)
> Per hg annotate, :bz write the function and :bent and :sicking reviewed.

I'm not sure what you're asking for exactly, but since this is all behind an #ifdef I don't see us gaining much from this change. It doesn't seem like it will hurt either, though.

Comment 15

[Masatoshi Kimura [:emk]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #13)
> Hi Masatoshi, could you also confirm we need to apply the same thing to
> other permission-required DOM APIs like MobileConnection, Voicemail,
> MobileMessage, CellBroadcast, etc., as well?

Yes, but I'll do it in a follow-up bug because the priority is lower than this bug. Unlike Telephony, global objects for those APIs are (unfortunately) already exposed on release versions of Firefox.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 803084 [details] [diff] [review]
patch v2

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

r=me

Thanks Masatoshi.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> but since this is all behind an #ifdef

The existence of window.Telephony/TelephonyCall/TelephonyCallGroup is not behind an ifdef.  It's just there in desktop Firefox.

Comment 18

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/492fdd7c06fd

Comment 19

[Masatoshi Kimura [:emk]]

Reverted unrelated changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b84bbad669d

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: 492fdd7c06fd - 4b84bbad669d for record

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=27793283&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27793063&tree=Mozilla-Inbound

These were failures on the original commit. Were they fixed in the followup, or do we need to back it all out?

Comment 22

[Masatoshi Kimura [:emk]]

Need a backout for the marionette failure. (I thought I ran marionette on try, but needed a different token from desktop.)

Comment 23

[Masatoshi Kimura [:emk]]

But not sure why mochitest failed... (need clobber?)

Comment 24

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=df815e46092c

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in 
https://hg.mozilla.org/integration/mozilla-inbound/rev/68d2ab5621f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/87300a32ff77

Comment 26

[Masatoshi Kimura [:emk]]

Looks like the patch was broken by something in the following range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5417e5da2ceb&tochange=d5fc994ca2ed

Comment 27

[Masatoshi Kimura [:emk]]

Mochtest-8 starts failing since https://hg.mozilla.org/mozilla-central/rev/564f0718eefe (bug 912612).
Marionette starts failing since https://hg.mozilla.org/mozilla-central/rev/dbf7b8418bd2 (bug 909638).

Comment 28

[Masatoshi Kimura [:emk]]

I was unable to make marionette test work.
I gave up and landed the v1 patch for now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/014958e3c8c9

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/014958e3c8c9

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

nominate for koi+ for it blocks koi+ bug 914182.

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

correct: bug 915604

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

Aready in mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/014958e3c8c9