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.

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

Can look at it this week, np.

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 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.

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?

(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

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

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

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

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.

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 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?

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

> 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'.

Iteration 3 :-)

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