987549 - Keyboard cannot show up after resuming after invoking inline activity

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

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

After enabling keyboard OOP, we found a case that keyboard cannot be show up.
(Thanks to Steve for finding this.)

STR
===
1. Kill keyboard app process first.
   (adb shell b2g-ps, then kill the [PID] of keyboard process)

2. Go to message app to create a new message.

3. Click the header (the recipient name or number) in the thread list UI.
   It should prompt a menu, select "Create a new contact".

4. Click on the input field to invoke the keyboard.
  
Expected result: The keyboard will show up.
Actual result: No keyboard shows up. (Note: the keyboard process will be spawned.)

Comment 1

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

Here is a patch to add dump message, which shows that in this case keyboard app won't get inputcontextchange event to get a valid inputContext.
 - https://github.com/RudyLu/gaia/tree/keyboard/Bug987549-keyboard_not_showup


Yuan,

Do you have bandwidth to check this potential issue?
Thanks.

Comment 2

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

Sorry, I don't have bandwidth. Jan might have.

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

Can look at it this week, np.

Comment 4

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: bug987549.diff

So this broke in https://github.com/mozilla/gecko-dev/commit/840767de6ba06311798c69d3b426c79a64b8bd2e, because the |cpmm.sendAsyncMessage("Keyboard:GetContext", {});| call was moved into the |setActive| function, and this function does not get called on activation. I don't know if that is a bug deeper in platform or that this patch also solves the problem. We do a manual call to |setActive| on uninit.

Comment 5

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

Comment on attachment 8397829 [details] [diff] [review]
bug987549.diff

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

Jan, thanks for the help, but we cannot call setActive(true) unconditionally. There may be more than one keyboard apps running, but only one is active. The system app is responsible to activate the keyboard. To avoid activating a deactivated keyboard app mistakenly, we need to wait for the system app to re-activate the keyboard app after it crashes.

I think you need to check whether "setInputMethodActive" of the keyboard iframe was not called from gaia side after keyboard crashes.

Comment 6

[Jan Jongboom [:janjongboom] (inactive)]

On Gaia side everything gets called afaik. When we focus on an input field and there is no keyboard process this happens:

* Gaia keyboard app wakes up
* MozKeyboard.js is called and MozInputMethod objects are created

Then:

* Gaia keyboard checks if there is inputcontext on startup + attaches listener

Now Gecko doesn't do a state syncing when MozInputMethod object is created any more, and thus on the *Gecko* side we are unaware that there is an active input context.

Perhaps we should not call |setActive| but just call |cpmm.sendAsyncMessage("Keyboard:GetContext", {});| on startup so we have at least the state info correct? 

That said, I haven't seen any call to setActive, when should that be called, and from where?

Comment 7

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

(In reply to Jan Jongboom [:janjongboom] from comment #6)
> On Gaia side everything gets called afaik. When we focus on an input field
> and there is no keyboard process this happens:
> 
> * Gaia keyboard app wakes up
> * MozKeyboard.js is called and MozInputMethod objects are created
> 
> Then:
> 
> * Gaia keyboard checks if there is inputcontext on startup + attaches
> listener
> 
> Now Gecko doesn't do a state syncing when MozInputMethod object is created
> any more, and thus on the *Gecko* side we are unaware that there is an
> active input context.
> 
> Perhaps we should not call |setActive| but just call
> |cpmm.sendAsyncMessage("Keyboard:GetContext", {});| on startup so we have at
> least the state info correct? 
If the keyboard is not active, MozInputMethod will drop all the message received including "Keyboard:GetContext". So |setActive| should be called. Also the change of bug 971651 requires
MozInputMethod to send "Keyboard:Register" to Keyboard.jsm to register as message receiver. Keyboard.jsm doesn't forward keyboard messages from form.js to unregistered MozInputMethod.
> 
> That said, I haven't seen any call to setActive, when should that be called,
> and from where?

To active a keyboard app, we need the following steps.

-- gaia ---
1. keyboard_manager calls the setInputMethodActive of the keyboard app iframe.

https://github.com/mozilla-b2g/gaia/blob/dd8317e07e1b6eda07b997686559ebcda99f7bea/apps/system/js/keyboard_manager.js#L783

-- gecko --
2. _setInputMethodActive of BrowserElementParent.jsm is called:
https://github.com/mozilla/gecko-dev/blob/3948825ebe5f1ccf41db8d9a3355aecfdd889be0/dom/browser-element/BrowserElementParent.jsm#L709

3. BrowserElementParent.jsm sends an asynchronous message to BrowserElementChildPreload.js:
https://github.com/mozilla/gecko-dev/blob/3948825ebe5f1ccf41db8d9a3355aecfdd889be0/dom/browser-element/BrowserElementChildPreload.js#L995

4. BrowserElementChildPreload.js calls mozInputMethod#setActive:

https://github.com/mozilla/gecko-dev/blob/3948825ebe5f1ccf41db8d9a3355aecfdd889be0/dom/browser-element/BrowserElementChildPreload.js#L1012

Comment 8

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

(In reply to Yuan Xulei [:yxl] from comment #7)

> 
> To active a keyboard app, we need the following steps.
> 
> -- gaia ---
> 1. keyboard_manager calls the setInputMethodActive of the keyboard app
> iframe.
> 
> https://github.com/mozilla-b2g/gaia/blob/
> dd8317e07e1b6eda07b997686559ebcda99f7bea/apps/system/js/keyboard_manager.
> js#L783

I can confirm the above would be called in the case as I described in comment 0.

Jan, Xulei,

Are you still working on this issue or I need to ask for some other's help for us?
Thanks.

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

Yeah I tracked it down on Friday but got stuck, will look at it again today.

Comment 10

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

Jan,

Thanks a lot for your help.
Please let me know if anything I need to check on gaia side.

Comment 11

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

Jan and Rudy, thanks.

I'm not currently working on it. If keyboard_manager calls the setInputMethodActive, we need to check whether mozInputMethod#setActive isn't called.

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch v2

Alright, so in general we always hit this code when going from blur -> focus in BrowserElementParent.js: |let reqOld = XPCNativeWrapper.unwrap(activeInputFrame).setInputMethodActive(false);|

That's generally OK, but:

* If this resolves to onerror, we also fire an error on the current request and thus the keyboard doesn't pop up
* If we call |this._sendSetInputMethodActiveDOMRequest| before BrowserElementChildPreload.js has been loaded, req will also throw (NotInitialized error)

Both of these things happen in this scenario. First deactivating the unwrap'ed inputframe fails, thus quitting the sequence and not popping up the keyboard. When you explicitly call |sendSetInputMethodActiveDOMRequest| anyway, this will fail because we didn't load all the client scripts yet.

In this patch therefore:

- New even from BrowserElementChild -> BrowserElementParent called |ready|, which is sent right after all content scripts are fully loaded
- Waiting before |this._ready| is set (via the event) before asking the BrowserElementChild to make an input method active
- Always making input method active, also if de-activating the old one failed (because it died anyway).

Comment 13

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

Comment on attachment 8399929 [details] [diff] [review]
Patch v2

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

Good job for finding the cause of this bug. I guess it may resolve bug 987928 as well.

::: dom/browser-element/BrowserElementChild.js
@@ +57,5 @@
>  }
>  
>  var BrowserElementIsReady = true;
> +
> +sendSyncMessage('browser-element-api:call', { 'msg_name': 'ready' });

It seems we could reuse the "hello" message sent at the beginning of this file.

::: dom/browser-element/BrowserElementParent.jsm
@@ +730,5 @@
>          let reqOld = XPCNativeWrapper.unwrap(activeInputFrame)
>                                       .setInputMethodActive(false);
> +
> +        reqOld.onsuccess = reqOld.onerror = function() {
> +          let activate = function() {

I perfer to changing this name to setActive or something, as it is called either to activate or deactivate the child depending on the value of "isActive".

@@ +736,5 @@
> +            this._sendSetInputMethodActiveDOMRequest(req, isActive);
> +          }.bind(this);
> +
> +          if (this._ready) {
> +            activate();

nit: add a "return" after activate() and delete the following "else" to decrease the indentation level.

@@ +739,5 @@
> +          if (this._ready) {
> +            activate();
> +          }
> +          else {
> +            let self = this;

nit: use "bind" instead.

@@ -730,2 @@
>          }.bind(this);
> -        reqOld.onerror = function() {

If reqOld.onerror is fired, shouldn't we call |acitve| anyway?

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

> It seems we could reuse the "hello" message sent at the beginning of this
> file.
Urm, but that has it's own purpose already (there is a handler attached etc.)


> If reqOld.onerror is fired, shouldn't we call |acitve| anyway?
I do, see https://bugzilla.mozilla.org/attachment.cgi?id=8399929&action=diff line 733 (maybe a comment would be nice there :p)

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

> nit: use "bind" instead.

Yeah, I didn't do that because then I can't unbind the function inline. F.e.:

document.body.addEventListener('click', function onClick() { 
  console.log('onclick'); 
  document.body.removeEventListener('click', onClick); 
}.bind(this))

Won't unbind the handler. So we'll have to have a variable somewhere anyway, whether it's the binded function or self doesn't really matter there I'd think.

Comment 16

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

(In reply to Jan Jongboom [:janjongboom] from comment #14)
> > It seems we could reuse the "hello" message sent at the beginning of this
> > file.
> Urm, but that has it's own purpose already (there is a handler attached etc.)
Both messages are used to notify the parent when child is ready. Since IPC message is expensive, it's reasonable for me to merge the two messages.
> 
> > If reqOld.onerror is fired, shouldn't we call |acitve| anyway?
> I do, see https://bugzilla.mozilla.org/attachment.cgi?id=8399929&action=diff
> line 733 (maybe a comment would be nice there :p)
You did. I missed that.

Comment 17

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

(In reply to Jan Jongboom [:janjongboom] from comment #15)
> > nit: use "bind" instead.
> 
> Yeah, I didn't do that because then I can't unbind the function inline. F.e.:
> 
> document.body.addEventListener('click', function onClick() { 
>   console.log('onclick'); 
>   document.body.removeEventListener('click', onClick); 
> }.bind(this))
> 
> Won't unbind the handler. So we'll have to have a variable somewhere anyway,
> whether it's the binded function or self doesn't really matter there I'd
> think.

It is recommended to use bind, but for this case, it's ok for me to use 'self'.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch v3

Iteration 3 :-)

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9bf6ffacf4f6