Closed Bug 971651 Opened 10 years ago Closed 10 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(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)

RESOLVED FIXED
1.4 S1 (14feb)
blocking-b2g 1.3+
Tracking Status
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

People

(Reporter: mchang, Assigned: fabrice)

Details

(Keywords: perf, sec-high, Whiteboard: [c=effect p= s= u=tarako] [demo][adv-main30-])

Attachments

(1 file, 1 obsolete file)

On a tarako device, while typing the keyboard, the NUWA process gets the keyboard events and uses 10-20% CPU usage.
Summary: NUWA should ignore keyboard events → Keyboard Should only send key press events to the webpage that uses the keyboard
blocking-b2g: --- → 1.3T?
Attached patch keyboard-ipc.patch (obsolete) — Splinter Review
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
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]
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)
ok, where do we activate the keyboard?
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)
Attachment #8375229 - Attachment is obsolete: true
Attachment #8375286 - Flags: review?(xyuan)
Attachment #8375286 - Flags: review?(xyuan) → review+
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(...)|.
(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
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
Closed: 10 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+
Attachment #8375286 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
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.
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?
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.
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?
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.
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
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.

Attachment

General

Created:
Updated:
Size: