Closed Bug 987549 Opened 7 years ago Closed 7 years ago

Keyboard cannot show up after resuming after invoking inline activity

Categories

(Firefox OS Graveyard :: Runtime, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S5 (11apr)

People

(Reporter: rudyl, Assigned: janjongboom)

References

Details

Attachments

(1 file, 2 obsolete files)

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.)
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.
Flags: needinfo?(xyuan)
Component: Gaia::System::Input Mgmt → Runtime
Sorry, I don't have bandwidth. Jan might have.
Flags: needinfo?(xyuan) → needinfo?(janjongboom)
Can look at it this week, np.
Assignee: nobody → janjongboom
Attached patch bug987549.diff (obsolete) — Splinter Review
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.
Attachment #8397829 - Flags: review?(xyuan)
Flags: needinfo?(janjongboom)
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.
Attachment #8397829 - Flags: review?(xyuan) → review-
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?
Flags: needinfo?(xyuan)
(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
Flags: needinfo?(xyuan)
(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.
Flags: needinfo?(xyuan)
Flags: needinfo?(janjongboom)
Yeah I tracked it down on Friday but got stuck, will look at it again today.
Flags: needinfo?(janjongboom)
Jan,

Thanks a lot for your help.
Please let me know if anything I need to check on gaia side.
Flags: needinfo?(xyuan)
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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).
Attachment #8397829 - Attachment is obsolete: true
Attachment #8399929 - Flags: review?(xyuan)
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?
Attachment #8399929 - Flags: review?(xyuan) → feedback+
> 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)
Flags: needinfo?(xyuan)
> 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.
(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.
(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'.
Flags: needinfo?(xyuan)
Attached patch Patch v3Splinter Review
Iteration 3 :-)
Attachment #8399929 - Attachment is obsolete: true
Attachment #8400470 - Flags: review?(xyuan)
Attachment #8400470 - Flags: review?(xyuan) → review+
https://hg.mozilla.org/mozilla-central/rev/9bf6ffacf4f6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S5 (11apr)
See Also: → 993394
You need to log in before you can comment on or make changes to this bug.