920977 - Limit the usage of the (deprecated) mozKeyboard API to System app only

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Since the new IME WebAPI is ready, we should remove the deprecated mozKeyboard API before v1.2 shipping.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

If bug 906096 lands it will be removed from FF nightly, so if nothing breaks we can remove the codez from b2g/ as well.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

We shouldn't expose the (deprecated) mozKeyboard API to 3rd-party apps.

Comment 3

[Yuan Xulei [:yxl] (Leave from May 18)]

Before completely removing it, we need to remove the reference of mozKeyboard from keyboard.js first. 
And as the system value_selector relies on mozKeyboard API, we may keep it for system app temporarily.
For a temporary solution, I'd like to change the permission of mozKeyboard to 'inputmethod-manage',
which can only be granted to certified app.

FYI, the following are references of mozKeyboard of gaia.

grep -r mozKeyboard
keyboard/js/keyboard.js: *   navigator.mozKeyboard
keyboard/js/keyboard.js: *   them into synthetic key events using the navigator.mozKeyboard API.
keyboard/js/keyboard.js: *      keycode to mozKeyboard.sendKey(). The IM could call
keyboard/js/keyboard.js: *      mozKeyboard.sendKey() directly, but doing it this way allows
keyboard/js/keyboard.js:// If we get a focuschange event from mozKeyboard for an element with
keyboard/js/keyboard.js:        window.navigator.mozKeyboard.removeFocus();
keyboard/js/keyboard.js:// This is called when we get an event from mozKeyboard.
system/test/unit/value_selector_test.js:    realKeyboard = window.navigator.mozKeyboard;
system/test/unit/value_selector_test.js:    window.navigator.mozKeyboard = sinon.stub();
system/test/unit/value_selector_test.js:    window.navigator.mozKeyboard = realKeyboard;
system/js/value_selector/value_selector.js:      window.navigator.mozKeyboard.setSelectedOption(singleOptionIndex);
system/js/value_selector/value_selector.js:      window.navigator.mozKeyboard.setSelectedOptions(optionIndices);
system/js/value_selector/value_selector.js:    window.navigator.mozKeyboard.removeFocus();
system/js/value_selector/value_selector.js:        window.navigator.mozKeyboard.setValue(timeValue);
system/js/value_selector/value_selector.js:        window.navigator.mozKeyboard.setValue(dateValue);
system/js/value_selector/value_selector.js:          window.navigator.mozKeyboard.setValue(datetimeValue);
system/js/value_selector/value_selector.js:    window.navigator.mozKeyboard.removeFocus();
system/js/keyboard_manager.js:// If we get a focuschange event from mozKeyboard for an element with
system/js/keyboard_manager.js:    if (navigator.mozKeyboard) {
system/js/keyboard_manager.js:      navigator.mozKeyboard.onfocuschange = function onfocuschange(evt) {

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

I removed the code from keyboard_manager and keyboard.js already in bug 906096.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

I removed the code from keyboard_manager and keyboard.js already in bug 906096.

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

Create! bug 906096 is indeed a fat bug.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Rudy, could you track this bug and find a person to work on it once bug 906096 is resolved? Thanks!

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Yes, sure, will help to take care of this until we remove the mozKeyboard API.

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

Stealing this.

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

So the problem here is that we have a couple of functions |setSelectedOption|, |setSelectedOptions|, |setValue| that are used to control stuff like drop down lists etc. And we don't have equivalents in the new API. I'm also struggling to think of a logical API for this. Guys, any idea?

Comment 11

[Yuan Xulei [:yxl] (Leave from May 18)]

My suggestion is that we keep mozKeyboard for system app for internal usage, removing unused functions from mozKeyboard and only leaving those required.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Yuan Xulei [:yxl] from comment #11)
> My suggestion is that we keep mozKeyboard for system app for internal usage,
> removing unused functions from mozKeyboard and only leaving those required.

Yeah that probably make the most sense if we want to solve this bug in koi+ time frame.

Who will be reviewing the patch? I am trying to find a proper person to ni? before people actually writing that patch.

Comment 13

[Yuan Xulei [:yxl] (Leave from May 18)]

I'd like to review the patch.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Stealing.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP

Let's see if this works ...

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I can confirm the patch works locally (by testing with console.log() and manual test on value selector and keyboard).

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

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 823836 [details] [diff] [review]
WIP

I am not sure my try config is sufficient enough, could you verify?

Also, I am also not sure whether or not I should modify this test
http://dxr.mozilla.org/mozilla-central/source/dom/permission/tests/test_keyboard.html
(the only test that mentioned mozKeyboard.

Comment 18

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 823836 [details] [diff] [review]
WIP

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

Hi Tim, we already have limited mozKeyboard to certified app. So privileged app, such as 
the third party app cannot use mozKeyboard. Will that be enough? Do we still need these changes?

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #17)
> Comment on attachment 823836 [details] [diff] [review]
> WIP
> 
> I am not sure my try config is sufficient enough, could you verify?
We need also run mochitests on emulator. The try command will be like this:

 try: -b o -p emulator, unagi,linux_gecko,linux64_gecko,macosx64_gecko -u mochitests, marionette-webapi,gaia-unit,gaia-ui-test -t none

> 
> Also, I am also not sure whether or not I should modify this test
> http://dxr.mozilla.org/mozilla-central/source/dom/permission/tests/
> test_keyboard.html
> (the only test that mentioned mozKeyboard.

The patch will break this test. If we want to limit the usage of mozKeyboard to system app only, we should remove mozKeyboard from this test.

::: dom/inputmethod/MozKeyboard.js
@@ +51,5 @@
>      }
>  
> +    // Make sure the deprecated mozKeyboard API does not
> +    // expose to any page in apps other than System app.
> +    Cu.import("resource://gre/modules/PermissionSettings.jsm");

You can import the jsm lazily by using |XPCOMUtils.defineLazyServiceGetter|.
And it seems that this module can only be used in main process, but not the content process.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks for the swift feedback!

(In reply to Yuan Xulei [:yxl] from comment #18)
> Comment on attachment 823836 [details] [diff] [review]
> WIP
> 
> Review of attachment 823836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Tim, we already have limited mozKeyboard to certified app. So privileged
> app, such as 
> the third party app cannot use mozKeyboard. Will that be enough? Do we still
> need these changes?

I don't understand. Are you saying we have already allow mozInputMethod in privileged app but mozKeyboard certified only? How do we do that? I couldn't find anything on that in the codebase ...

If so we could probably mark this bug fixed.

> 
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #17)
> > Comment on attachment 823836 [details] [diff] [review]
> > WIP
> > 
> > I am not sure my try config is sufficient enough, could you verify?
> We need also run mochitests on emulator. The try command will be like this:
> 
>  try: -b o -p emulator, unagi,linux_gecko,linux64_gecko,macosx64_gecko -u
> mochitests, marionette-webapi,gaia-unit,gaia-ui-test -t none
> 
> > 
> > Also, I am also not sure whether or not I should modify this test
> > http://dxr.mozilla.org/mozilla-central/source/dom/permission/tests/
> > test_keyboard.html
> > (the only test that mentioned mozKeyboard.
> 
> The patch will break this test. If we want to limit the usage of mozKeyboard
> to system app only, we should remove mozKeyboard from this test.
> 

Sure.

> ::: dom/inputmethod/MozKeyboard.js
> @@ +51,5 @@
> >      }
> >  
> > +    // Make sure the deprecated mozKeyboard API does not
> > +    // expose to any page in apps other than System app.
> > +    Cu.import("resource://gre/modules/PermissionSettings.jsm");
> 
> You can import the jsm lazily by using |XPCOMUtils.defineLazyServiceGetter|.
> And it seems that this module can only be used in main process, but not the
> content process.

OK, I am running out of Gecko tricks then. :-/ Let's figure out whether or not we really should do this or not first.

Comment 20

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #19)
> > Hi Tim, we already have limited mozKeyboard to certified app. So privileged
> > app, such as 
> > the third party app cannot use mozKeyboard. Will that be enough? Do we still
> > need these changes?
> 
> I don't understand. Are you saying we have already allow mozInputMethod in
> privileged app but mozKeyboard certified only? How do we do that? I couldn't
> find anything on that in the codebase ...

Sorry, I made a mistake. I forgot we had exposed mozKeyboard to privileged app.

We have a certified-app-only permission 'inputmethod-manage', which is used to
let sysetem app to set the active keyboard frame(see bug 905573 for details).
Currently only system app has this permission. We may use this permission
to limit mozKeyboard to system app.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Tentative patch v2

Manually verified. Try submitted:

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

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 824486 [details] [diff] [review]
Tentative patch v2

>+    // Limited the deprecated to certified apps only

should be
// Limited the deprecated mozKeyboard API to certified apps only

Comment 23

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 824486 [details] [diff] [review]
Tentative patch v2

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

Thanks for enhancing the permission test. It's almost good for checking in.

::: dom/inputmethod/MozKeyboard.js
@@ +43,5 @@
>    init: function mozKeyboardInit(win) {
>      let principal = win.document.nodePrincipal;
> +    // Limited the deprecated to certified apps only
> +    let perm = Services.perms.testExactPermissionFromPrincipal(principal,
> +                                                               "inputmethod-manage");

It will be better if you break this long line ;)

::: dom/permission/tests/test_keyboard.html
@@ +19,5 @@
>  var gData = [
>    {
>      perm: ["keyboard"],
> +    obj: "mozInputMethod",
> +    idl: "nsIB2GKeyboard",

Since mozInputMethod is implemented by webidl other than idl, we need change this line to:
 webidl: "InputMethod",

Comment 24

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Yuan Xulei [:yxl] from comment #23)
> ::: dom/inputmethod/MozKeyboard.js
> @@ +43,5 @@
> >    init: function mozKeyboardInit(win) {
> >      let principal = win.document.nodePrincipal;
> > +    // Limited the deprecated to certified apps only
> > +    let perm = Services.perms.testExactPermissionFromPrincipal(principal,
> > +                                                               "inputmethod-manage");
> 
> It will be better if you break this long line ;)
I just noticed that you had broken the line into two, but the second line is longer than 80 characters and is
wrapped to the third line strangely by bugzilla diff view.

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Yuan Xulei [:yxl] from comment #24)
> (In reply to Yuan Xulei [:yxl] from comment #23)
> > ::: dom/inputmethod/MozKeyboard.js
> > @@ +43,5 @@
> > >    init: function mozKeyboardInit(win) {
> > >      let principal = win.document.nodePrincipal;
> > > +    // Limited the deprecated to certified apps only
> > > +    let perm = Services.perms.testExactPermissionFromPrincipal(principal,
> > > +                                                               "inputmethod-manage");
> > 
> > It will be better if you break this long line ;)
> I just noticed that you had broken the line into two, but the second line is
> longer than 80 characters and is
> wrapped to the third line strangely by bugzilla diff view.

Yeah, judging by the code present in the file I don't think we have a hard requirement on 80 chars there. So I am keeping the alignment. Going to push to try again now.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Tentative patch v2.1

This is probably ready for review once Try passes.

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

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 824546 [details] [diff] [review]
Tentative patch v2.1

Try failed

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

According to file_framework.js and file_shim.html:

 *         The IDL describing the navigator object. The returned object
 *         during tests /must/ be an instanceof this


According to the error

TypeError: invalid 'instanceof' operand window[this.webidl]

It looks like we did not expose the constructor function to the frame?

Alternatively, I did not set the pref dom.mozInputMethod.enabled. Going to try that.

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #28)
> Alternatively, I did not set the pref dom.mozInputMethod.enabled. Going to
> try that.

https://tbpl.mozilla.org/?tree=Try&rev=798a9d48492d not working.

Comment 30

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #28)
> According to file_framework.js and file_shim.html:
> 
>  *         The IDL describing the navigator object. The returned object
>  *         during tests /must/ be an instanceof this
> 
> 
> According to the error
> 
> TypeError: invalid 'instanceof' operand window[this.webidl]
> 
> It looks like we did not expose the constructor function to the frame?
> 
> Alternatively, I did not set the pref dom.mozInputMethod.enabled. Going to
> try that.

The WebIDL interface name should be MozInputMethod :-)

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Yeah, I figured out that too.

Comment 32

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v2.2

Comment 33

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #31)
> https://tbpl.mozilla.org/?tree=Try&rev=4fbc098b66cf

The failure on test 9 seems unrelated but I just hit rerun anyway.

Comment 34

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch for commit

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4ded57b1a15c

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ded57b1a15c

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/40ec01fa1947