971651 - Keyboard Should only send key press events to the webpage that uses the keyboard

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Mason Chang [Inactive] [:mchang]]

On a tarako device, while typing the keyboard, the NUWA process gets the keyboard events and uses 10-20% CPU usage.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: keyboard-ipc.patch

With this patch we save 2 ipc messages on each tap per extra app. So typically that saves us 4 messages (not sending to nuwa and the homescreen).

I also did a bit of renaming to make things easier to follow, and switch from ObjectWrapper.wrap() to Cu.cloneInto which is faster.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+. need this for demo

Comment 4

[Mason Chang [Inactive] [:mchang]]

Just tested on Tarako, with this patch on mozilla_b2g28_t on a Tarako device, reference-workload-light Gaia, w/ MMS and the gaia debug keyboard, we no longer see the NUWA process in top while typing. Looks good!

Comment 5

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

Comment on attachment 8375229 [details] [diff] [review]
keyboard-ipc.patch

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

::: dom/inputmethod/MozKeyboard.js
@@ +325,5 @@
>      cpmm.addMessageListener('Keyboard:FocusChange', this);
>      cpmm.addMessageListener('Keyboard:SelectionChange', this);
>      cpmm.addMessageListener('Keyboard:GetContext:Result:OK', this);
>      cpmm.addMessageListener('Keyboard:LayoutsChange', this);
> +    cpmm.sendAsyncMessage('Keyboard:Register', {});

I think we should send the |register| and |unregister| messages when the keyboard is activated or deactivated, but not when the keyboard is initialized or uninitialized. Because there may be tow or more keyboard apps initialized in the same time. Only the activated one is able to register as the message receiver.

Comment 6

[[:fabrice] Fabrice Desré]

ok, where do we activate the keyboard?

Comment 7

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

We activate the keyboard with MozInputMethod#setActive method in MozKeyboard.js:

http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/MozKeyboard.js#419

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: keyboard-ipc.patch v2

Comment 9

[[:fabrice] Fabrice Desré]

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

Comment 10

[[:fabrice] Fabrice Desré]

backed out for M3 failures: https://hg.mozilla.org/integration/b2g-inbound/rev/8d428546a964

Comment 11

[[:fabrice] Fabrice Desré]

Yuan, any idea why we fail here? I added logging in the test and we go into inputmethod_cleanup() but still timeout...

Comment 12

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

Comment on attachment 8375286 [details] [diff] [review]
keyboard-ipc.patch v2

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

::: dom/inputmethod/Keyboard.jsm
@@ +281,5 @@
>      // cache the layouts so on init we can respond immediately instead
>      // of going back and forth between keyboard_manager
>      this._layouts = layouts;
>  
> +    this.sendToForm('Keyboard:LayoutsChange', layouts);

This should be |this.sendToKeyboard(...)|.

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Yuan Xulei [:yxl] from comment #12)
> Comment on attachment 8375286 [details] [diff] [review]
> keyboard-ipc.patch v2
> 
> Review of attachment 8375286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/inputmethod/Keyboard.jsm
> @@ +281,5 @@
> >      // cache the layouts so on init we can respond immediately instead
> >      // of going back and forth between keyboard_manager
> >      this._layouts = layouts;
> >  
> > +    this.sendToForm('Keyboard:LayoutsChange', layouts);
> 
> This should be |this.sendToKeyboard(...)|.

Yep, I had that fixed in the version that I landed: https://hg.mozilla.org/integration/b2g-inbound/rev/b0eaf0e9fa9c#l1.235

Comment 14

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

There is a bug with MozKeyboard.js. MozInputMethod#uninit doesn't call setActive(false), if MozInputMethod reinitialized with the same window, calling setActive(true) will do nothing.

Please apply the follow patch:

diff --git a/dom/inputmethod/MozKeyboard.js b/dom/inputmethod/MozKeyboard.js
--- a/dom/inputmethod/MozKeyboard.js
+++ b/dom/inputmethod/MozKeyboard.js
@@ -324,18 +324,17 @@ MozInputMethod.prototype = {
     Services.obs.addObserver(this, "inner-window-destroyed", false);
     cpmm.addMessageListener('Keyboard:FocusChange', this);
     cpmm.addMessageListener('Keyboard:SelectionChange', this);
     cpmm.addMessageListener('Keyboard:GetContext:Result:OK', this);
     cpmm.addMessageListener('Keyboard:LayoutsChange', this);
   },
 
   uninit: function mozInputMethodUninit() {
-    // Unregister in case setActive(false) was not called.
-    cpmm.sendAsyncMessage('Keyboard:Unregister', {});
+    this.setActive(false);
     Services.obs.removeObserver(this, "inner-window-destroyed");
     cpmm.removeMessageListener('Keyboard:FocusChange', this);
     cpmm.removeMessageListener('Keyboard:SelectionChange', this);
     cpmm.removeMessageListener('Keyboard:GetContext:Result:OK', this);
     cpmm.removeMessageListener('Keyboard:LayoutsChange', this);
 
     this._window = null;
     this._mgmt = null;

Comment 15

[[:fabrice] Fabrice Desré]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for making bug 970239 a perma-fail.
https://hg.mozilla.org/integration/b2g-inbound/rev/004feaa02d13

Comment 17

[Ryan VanderMeulen [:RyanVM]]

FWIW, the first attempt at landing this also made it perma-fail...

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Also mochitest-6 perma-fail.
https://tbpl.mozilla.org/php/getParsedLog.php?id=34622191&tree=B2g-Inbound

07:46:28     INFO -  [Parent 682] WARNING: waitpid failed pid:728 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
07:46:28     INFO -  Keyboard message Keyboard:Unregister from a content process with no 'input' privileges.############ ErrorPage.js
07:46:28     INFO -  ************************************************************
07:46:28     INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
07:46:28     INFO -  [Exception... "[JavaScript Error: "sendToKeyboard is not defined" {file: "resource://gre/modules/Keyboard.jsm" line: 71}]'[JavaScript Error: "sendToKeyboard is not defined" {file: "resource://gre/modules/Keyboard.jsm" line: 71}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
07:46:28     INFO -  ************************************************************
07:46:28     INFO -  ************************************************************
07:46:28     INFO -  * Call to xpconnect wrapped JSObject produced this error:  *
07:46:28     INFO -  [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://specialpowers/content/SpecialPowersObserver.js :: SpecialPowersObserver.prototype._sendAsyncMessage :: line 96"  data: no]
07:46:28     INFO -  ************************************************************
07:46:28     INFO -  [Parent 682] WARNING: waitpid failed pid:728 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
07:46:28     INFO -  [Parent 682] WARNING: waitpid failed pid:728 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
07:46:28     INFO -  [Parent 682] WARNING: Failed to deliver SIGKILL to 728!(3).: file ../../../gecko/ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118
07:46:28     INFO -  [Parent 682] WARNING: waitpid failed pid:728 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
07:46:28     INFO -  [Parent 682] WARNING: Failed to deliver SIGKILL to 728!(3).: file ../../../gecko/ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118

Comment 19

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

Keyboard mochitests only fails when OOP is enabled.

Comment 20

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

Comment on attachment 8375286 [details] [diff] [review]
keyboard-ipc.patch v2

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

::: dom/inputmethod/Keyboard.jsm
@@ +69,3 @@
>          // The application has been closed unexpectingly. Let's tell the
>          // keyboard app that the focus has been lost.
> +        sendToKeyboard('Keyboard:FocusChange', { 'type': 'blur' });

Should be |this.sendToKeyboard(...)| :)

Comment 21

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

When the keyboard app is unloaded and the last "Keyboard:Unregister" arrives Keyboard.jsm, the keyboard app message manager doesn't have the 'input' permission any more. The |assertPermission| check in keyboard.jsm will kill the keyboard app process: 

https://hg.mozilla.org/integration/b2g-inbound/file/adac868d387f/dom/inputmethod/Keyboard.jsm#l122

It may be the reason that OOPed mochitests failed.

Comment 22

[[:fabrice] Fabrice Desré]

try build with fixes:

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

Comment 23

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/integration/b2g-inbound/rev/4665cf1d88d9

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4665cf1d88d9

Comment 25

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e9f8f8b65bea

Comment 26

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

Comment on attachment 8375286 [details] [diff] [review]
keyboard-ipc.patch v2

We should consider taking this on 1.3 due to comment 1.

Comment 27

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

I'm going to mark this security-sensitive.  Andrew, can you give this a sec rating?

Comment 28

[Andrew McCreight [:mccr8]]

In the worst case, an app could grab passwords somebody is entering into a webpage or something, right?  So that sounds sec-high to me.

Comment 29

[Jason Smith [:jsmith]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #26)
> Comment on attachment 8375286 [details] [diff] [review]
> keyboard-ipc.patch v2
> 
> We should consider taking this on 1.3 due to comment 1.

Absolutely agree. sec-high = blocker. Let's get the approval form filled out, get approval, and land it.

Comment 30

[[:fabrice] Fabrice Desré]

I don't have a 1.3 tree available to uplift, but I think the 1.3t version (landed at https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e9f8f8b65bea) should apply there.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/20b1cc5deb79

Comment 32

[Andrew McCreight [:mccr8]]

So, I want to make sure I understand here... is the keyboard event only being sent to the Nuwa process, or to all processes spawned from the Nuwa process?  I think the Nuwa process isn't actually running any user code, so the former wouldn't be as terrible.  And if you can compromise the Nuwa process, you can probably compromise the actual process that is supposed to get the events.

Comment 33

[Andrew McCreight [:mccr8]]

It also sounds like this is a regression from Nuwa, so it won't affect branches that don't have Nuwa enabled.  Is that right?

Comment 34

[[:fabrice] Fabrice Desré]

The keyboard messages were broadcasted to all content processes, forked from nuwa or not. We noticed it because the nuwa process which should be very idle was chewing CPU, but the issue was real.

Comment 35

[Andrew McCreight [:mccr8]]

Thanks for the explanation.  It sounds like everything is affected, then.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/d809af26b278

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Crap, this fix depends on bug 964293, which isn't on v1.2. Backed out.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6391325ba95b

Is there an alternative to using Cu.cloneInto on that branch?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Honestly, attachment 8368321 [details] [diff] [review] seems trivial enough. Can we just take it on v1.2 too?

Comment 39

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #38)
> Honestly, attachment 8368321 [details] [diff] [review] seems trivial enough.
> Can we just take it on v1.2 too?

Scratch that, I forgot that that attachment was folded in with the original patch. So we either need an implementation that doesn't use Cu.cloneInto or we need to wontfix this for v1.2.

Comment 40

[Daniel Veditz [:dveditz]]

Nominating for 1.2: if we have any carriers ship a 1.2 device we need this fixed there. But at the moment it doesn't appear anyone's biting on that branch so maybe we don't need to do the work.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

This was actually an easy fix. The cloneInto changes were just a perf optimization that could be ignored for the branch backport (confirmed on IRC with baku and fabrice). I confirmed that this is green on Try and went ahead and re-pushed.

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/844d4412f59c