Closed
Bug 914182
Opened 11 years ago
Closed 11 years ago
WebTelephony: Hide Telephony API from regular Web pages
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: emk, Assigned: emk)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
khuey
:
review+
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
I don't think we can ship this yet.
Updated•11 years ago
|
Assignee: nobody → vyang
Summary: Hide Telephony API behind a pref, at least on desktop → WebTelephony: Hide Telephony API behind a pref, at least on desktop
Assignee | ||
Comment 1•11 years ago
|
||
Vicamo, please review Telephony bits. Kyle, please review the interface change. Green except a known failure: https://tbpl.mozilla.org/?tree=Try&rev=14ed6cb1d3d7
Attachment #801883 -
Flags: review?(vyang)
Attachment #801883 -
Flags: review?(khuey)
Comment on attachment 801883 [details] [diff] [review] patch Review of attachment 801883 [details] [diff] [review]: ----------------------------------------------------------------- Thank you Masatoshi. r=me
Attachment #801883 -
Flags: review?(khuey) → review+
Comment 3•11 years ago
|
||
Comment on attachment 801883 [details] [diff] [review] patch Review of attachment 801883 [details] [diff] [review]: ----------------------------------------------------------------- Thank you.
Attachment #801883 -
Flags: review?(vyang) → review+
Updated•11 years ago
|
Assignee: vyang → VYV03354
Assignee | ||
Comment 4•11 years ago
|
||
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
Attachment #801883 -
Attachment is obsolete: true
Attachment #803084 -
Flags: review?(vyang)
Attachment #803084 -
Flags: review?(khuey)
Assignee | ||
Updated•11 years ago
|
Summary: WebTelephony: Hide Telephony API behind a pref, at least on desktop → WebTelephony: Hide Telephony API behind a permission
Comment 5•11 years ago
|
||
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?
Attachment #803084 -
Flags: review?(vyang)
Assignee | ||
Comment 6•11 years ago
|
||
(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.
Assignee | ||
Comment 7•11 years ago
|
||
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.
Assignee | ||
Comment 8•11 years ago
|
||
Per hg annotate, :bz write the function and :bent and :sicking reviewed.
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bent.mozilla)
Assignee | ||
Comment 9•11 years ago
|
||
Changing the subject based on what I really meant to.
Summary: WebTelephony: Hide Telephony API behind a permission → WebTelephony: Hide Telephony API from regular Web pages
![]() |
||
Comment 10•11 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 11•11 years ago
|
||
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.
Attachment #803084 -
Flags: review?(vyang)
Comment 12•11 years ago
|
||
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.
Attachment #803084 -
Flags: review?(vyang) → review+
Comment 13•11 years ago
|
||
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?
Flags: needinfo?(VYV03354)
(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.
Flags: needinfo?(bent.mozilla)
Assignee | ||
Comment 15•11 years ago
|
||
(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.
Flags: needinfo?(VYV03354)
Comment on attachment 803084 [details] [diff] [review] patch v2 Review of attachment 803084 [details] [diff] [review]: ----------------------------------------------------------------- r=me Thanks Masatoshi.
Attachment #803084 -
Flags: review?(khuey) → review+
![]() |
||
Comment 17•11 years ago
|
||
> 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.
Assignee | ||
Comment 18•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/492fdd7c06fd
Status: NEW → ASSIGNED
Flags: needinfo?(jonas) → in-testsuite+
Assignee | ||
Comment 19•11 years ago
|
||
Reverted unrelated changes: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b84bbad669d
Assignee | ||
Comment 20•11 years ago
|
||
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?
Flags: needinfo?(VYV03354)
Assignee | ||
Comment 22•11 years ago
|
||
Need a backout for the marionette failure. (I thought I ran marionette on try, but needed a different token from desktop.)
Flags: needinfo?(VYV03354)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/68d2ab5621f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/87300a32ff77
Assignee | ||
Comment 26•11 years ago
|
||
Looks like the patch was broken by something in the following range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5417e5da2ceb&tochange=d5fc994ca2ed
Assignee | ||
Comment 27•11 years ago
|
||
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).
Assignee | ||
Comment 28•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/014958e3c8c9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 32•11 years ago
|
||
Aready in mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/014958e3c8c9
blocking-b2g: koi? → ---
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•