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

RESOLVED FIXED in Firefox 30

Status

P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mchang, Assigned: fabrice)

Tracking

({perf, sec-high})

unspecified
1.4 S1 (14feb)
ARM
Gonk (Firefox OS)
perf, sec-high

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, firefox-esr24 disabled, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [c=effect p= s= u=tarako] [demo][adv-main30-])

Attachments

(1 attachment, 1 obsolete attachment)

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

Updated

5 years ago
Summary: NUWA should ignore keyboard events → Keyboard Should only send key press events to the webpage that uses the keyboard
(Reporter)

Updated

5 years ago
blocking-b2g: --- → 1.3T?
(Assignee)

Comment 2

5 years ago
Created attachment 8375229 [details] [diff] [review]
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.
Assignee: nobody → fabrice
(Assignee)

Updated

5 years ago
Attachment #8375229 - Flags: review?(xyuan)
triage: 1.3T+. need this for demo
blocking-b2g: 1.3T? → 1.3T+
Whiteboard: [c=effect p= s= u=tarako][tarako] → [c=effect p= s= u=tarako][demo]
(Reporter)

Comment 4

5 years ago
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 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.
Attachment #8375229 - Flags: review?(xyuan)
(Assignee)

Comment 6

5 years ago
ok, where do we activate the keyboard?
(Assignee)

Updated

5 years ago
Flags: needinfo?(xyuan)
We activate the keyboard with MozInputMethod#setActive method in MozKeyboard.js:

http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/MozKeyboard.js#419
Flags: needinfo?(xyuan)
(Assignee)

Comment 8

5 years ago
Created attachment 8375286 [details] [diff] [review]
keyboard-ipc.patch v2
Attachment #8375229 - Attachment is obsolete: true
Attachment #8375286 - Flags: review?(xyuan)
Attachment #8375286 - Flags: review?(xyuan) → review+
(Assignee)

Comment 11

5 years ago
Yuan, any idea why we fail here? I added logging in the test and we go into inputmethod_cleanup() but still timeout...
Flags: needinfo?(xyuan)
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(...)|.
(Assignee)

Comment 13

5 years ago
(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
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;
Flags: needinfo?(xyuan)
FWIW, the first attempt at landing this also made it perma-fail...
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
Keyboard mochitests only fails when OOP is enabled.
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(...)| :)
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.
Component: Gaia::Keyboard → General

Updated

5 years ago
Status: NEW → ASSIGNED
Whiteboard: [c=effect p= s= u=tarako][demo] → [c=effect p= s= u=tarako] [demo]
Target Milestone: --- → 1.4 S1 (14feb)
https://hg.mozilla.org/mozilla-central/rev/4665cf1d88d9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8375286 [details] [diff] [review]
keyboard-ipc.patch v2

We should consider taking this on 1.3 due to comment 1.
Attachment #8375286 - Flags: approval-mozilla-b2g28?
I'm going to mark this security-sensitive.  Andrew, can you give this a sec rating?
Group: core-security
Flags: needinfo?(continuation)
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.
Flags: needinfo?(continuation)
Keywords: sec-high
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
blocking-b2g: 1.3T+ → 1.3+
(Assignee)

Updated

5 years ago
Attachment #8375286 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
(Assignee)

Comment 30

5 years ago
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.
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/20b1cc5deb79
status-b2g-v1.3: --- → fixed
status-b2g-v1.4: --- → fixed
status-firefox28: --- → wontfix
status-firefox29: --- → wontfix
status-firefox30: --- → fixed
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.
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?
(Assignee)

Comment 34

5 years ago
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.
Thanks for the explanation.  It sounds like everything is affected, then.
status-b2g-v1.1hd: --- → wontfix
status-b2g-v1.2: --- → affected
status-firefox-esr24: --- → disabled
status-b2g18: --- → unaffected
status-b2g-v1.1hd: wontfix → unaffected
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?
status-b2g-v1.2: fixed → affected
Flags: needinfo?(fabrice)
Honestly, attachment 8368321 [details] [diff] [review] seems trivial enough. Can we just take it on v1.2 too?
(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.
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.
tracking-b2g-v1.2: --- → ?
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
status-b2g-v1.2: affected → fixed
tracking-b2g-v1.2: ? → ---
Flags: needinfo?(fabrice)
Whiteboard: [c=effect p= s= u=tarako] [demo] → [c=effect p= s= u=tarako] [demo][adv-main30-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.