1043828 - Switching IMEs doesn't always work after switching to 3rd-party keyboard and back

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[John Lu [:mnjul][Please NI or I'll miss]]

Please see http://www.youtube.com/watch?v=pWj6EU_vGfA for demo video.

Description: If I have more than one built-in keyboard and some 3rd-party keyboard enabled, IME switching through the "En"-like button may break after I switch to the 3rd-party kb and switch back.

STR:
1. Enable some built-in keyboards. For example, English, Spanish, and France.
2. Enable some 3rd-party kb. In my demo video I use the Demo Keyboard's English, but you may also download Gaia's LOL keyboard from marketplace.
3. Open some app that you may type text.
4. Cycle through the keyboards with the "En" / "Es" / "Fr" or globe button.
5. When you cycle back onto built-in keyboard from the demo keyboard with the globe button, you should be at the built-in English keyboard (which is correct).
6. Press the "En" button again.

Actual result:  you find that you're still at "En" keyboard, with no switching to Es kb having taken effect. Note, at this time, the Utility Tray's notification indicates you're still using the built-in *English* keyboard.

Expected result: you're brought to Spanish keyboard. And you can cycle onto Fr keyboard with the "Es" key.

Additional note: at "Actual result", if you press "En" another two times, you would be brought to the Demo English keyboard, just as though you'd pressed "Es" and "fr" that would have displayed if things were not broken.

Reproducibility is not always 100%, so maybe some timing issue could also be at fault.

Comment 1

[John Lu [:mnjul][Please NI or I'll miss]]

Update: This may actually be a keyboard-app bug, not a system-app (input management) bug. I added code to print out hash on keyboard app's hashchange event during https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard.js#L58 and I see hashchange event being dispatched with a correct hash to keyboard app when the bug happens.

Comment 2

[John Lu [:mnjul][Please NI or I'll miss]]

[Blocking Requested - why for this release]: Can hinder normal usage of input methods.

Comment 3

[John Lu [:mnjul][Please NI or I'll miss]]

Dear QA folks, could you kindly check if the bug is present in 2.0 and 1.4 branches? Thanks. Please note that it can take a few "switching iterations" for the bug to manifest itself.

Comment 4

[John Lu [:mnjul][Please NI or I'll miss]]

For the record: we don't have this issue on 2.0 and 1.4. This issue is only on master.

I did a bisect and it seems this issue is a regression caused by bug 1016283. For the record, commit c0d1670ba57666210a7e7fd021ca4d9b47ab990c (bug 1016283 patch) is the first commit to have this issue, while the one commit before it, 56812bb7f5234088d6802361190411cdab796868, does not.

Comment 5

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #4)
> For the record: we don't have this issue on 2.0 and 1.4. This issue is only
> on master.
> 
> I did a bisect and it seems this issue is a regression caused by bug
> 1016283. For the record, commit c0d1670ba57666210a7e7fd021ca4d9b47ab990c
> (bug 1016283 patch) is the first commit to have this issue, while the one
> commit before it, 56812bb7f5234088d6802361190411cdab796868, does not.

Scrap it ^^^^ 56812bb has this issue. c0d1670 only made it more apparent to discover. To see this issue even in 56812bb, I removed console.log/warn() calls from the code [1], as in: https://github.com/mnjul/gaia/commit/f9ac7a3d77408c0fbe9f766b6f9165cb59b182ca .

Please check the YouTube video of the demo of the code: https://www.youtube.com/watch?v=9QI2TvJTXdo .
From 0'05, we can see that Fr keyboard shows, instead of En keyboard [2]. And the height of the keyboard frame seems wrong. This seems because inputContext is never properly transferred to the keyboard app; as we can see, when this happens, navigator.mozInputMethod.inputcontext is null, and typing on the keyboard has no effect at all.

(again, this bug does not always reproduce -- you might need several cycling to see it)

[1] I discovered that the existence of console.log/warn()s would affect the manifestation of the issue very much, which could suggest this might be a racing/timing issue.

[2] The "correct" behavior should be a flash of switching from the Fr keyboard to the En keyboard, which is not recorded in the video. My built-in keyboards are En - Pt - Es - Fr.

Comment 6

[John Lu [:mnjul][Please NI or I'll miss]]

Here is some analysis with current master.

** TL;DR: We might have some Gecko-side InputMethod-related API issue. **

Root cause of the bug: the Promise returned by getText() of |navigator.mozInputMethod.inputcontext| doesn’t get resolved, and would block the activation of an IME. Again, this scenario does not always happen. And this bug happens iff the getText() Promise doesn’t get resolved.

Code path of the root cause:
|imEngine.activate()| is blocked by a Promise (``dataPromise`` or ``InputMethodManager._inputContextData``) ( https://github.com/mnjul/gaia/blob/f740793dc3f7e59df800c6392209ab0a2486d4c9/apps/keyboard/js/keyboard/input_method_manager.js#L421 ), and that Promise is blocked by the getText() Promise ( https://github.com/mnjul/gaia/blob/f740793dc3f7e59df800c6392209ab0a2486d4c9/apps/keyboard/js/keyboard/input_method_manager.js#L341 )

Observation & Concept:
1. When a KBApp frame is |setInputMethodActive(true)|’ed after it was |setInputMethodActive(false)|’ed, by System app (keyboard_manager.js), the KBApp frame receives |inputcontextchange| event. Supposedly, when this happens, |navigator.mozInputMethod.inputcontext| should be a valid InputContext and its getText() should be resolvable.
2. However, sometimes, the |getText()| doesn’t get resolved. It actually gets rejected when later the KBApp frame is |setInputMethodActive(false)|’ed again.
3. The non-resolving of the getText() Promise blocks IME activation as described above.

Proof of Concept:
Please use code at https://github.com/mnjul/gaia/commit/e085c995bb3a748ec9ebd77810a76bb463887587 .
I added a |flipFramesActive()| method in KeyboardManager, which switches from the current built-in English keyboard into demo English keyboard, and switches back.
STR to observe the non-resolving of getText() Promise (please refer to https://www.youtube.com/watch?v=iYqM6lnu2wk for some quick glimpses of the STR [1]):
1. In Settings, enable built-in English keyboard and demo English keyboard (no other keyboards beside these two and Number keyboard).
2. Launch Messages (SMS) app, and start a new message.
3. Keep the input focus at the recipient textarea. The built-in English keyboard would be brought up. Optionally, type something.
4. Click the “En”, such that the demo English keyboard frame would be loaded/started.
5. Click the globe icon of the demo keyboard to switch back to built-in English keyboard.
6. Launch App Manager on Nightly, debug into System app and Built-in Keyboard app’s consoles.
7. On System app’s console, run `` setInterval(function(){KeyboardManager.flipFramesActive();}, 2000);  ``.
8. In Built-in KBApp’s console, KBApp should receive |inputcontextchange| events each time flipFramesActive() runs [2]. In my code, KBApp would call getText() of |navigator.mozInputMethod.inputcontext| immediately in the event handler, if the inputcontext is valid.
9. Following step 8, for most of the time, the getText() Promise resolves. So in KBApp’s console, you see “success” log (and what you optionally typed in step 3 would be retrieved). *However*, for some of the time, the getText() Promise doesn’t get resolved, and you see a “error” log when [Observation & Concept 2] happens.

So: even as KBApp receives a |inputcontextchange| event and |navigator.mozInputMethod.inputcontext| is some valid object, sometimes the Promise of getText() doesn’t get resolved, and this is the very root cause of this bug.

[1] P.S. flipFramesActive() has a MANIPULATE_VISIBILITY variable. If you set it to true, the switching of different keyboards would be visible. This is the case with the video. Additionally, it appears that when this variable is set true, the non-resolving of getText() Promise is more observable. 

[2] with each flipFramesActive() run, the KBApp should receive two occurrences of inputcontextchange, one when switching out (and |navigator.mozInputMethod.inputcontext| would be null), and one when switching in (and |navigator.mozInputMethod.inputcontext| would be a valid object).

Comment 7

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Xulei,

I was told you might be able to offer some insight on the issue we're facing: [ sometimes the Promise returned by navigator.mozInputMethod.inputcontext.getText() doesn't resolve at all ]. Could you check into this? 
Please check comment 0 for the description of the bug, and comment 6 for some analysis & observation.

Many thanks!

Comment 8

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

I'm looking into it now and what's the error getText() rejects?

Comment 9

[John Lu [:mnjul][Please NI or I'll miss]]

When the Promise returned by getText() is rejected, its error is "inputContext got destroyed".

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I have another STR that trigger the same symptom but the cause might not be the same:

1. Enable only built-in keyboard layouts for En, Es, Pinyin, Zhuyin. Switch to En.
2. Kill the keyboard app and focus on a input field to force it to relaunch.
3. Quickly tap the switch button for 3 times, try to switch from En to Zhuyin (-> Es -> Pinyin -> Zhuyin)

Expected:

Switch to Zhuyin successfully

Actual:

a. Visually, the keyboard remain in Pinyin, so the rendering is not switched yet.
b. Using app manager to inspect |app.layoutManager.currentModifiedLayout.layoutName|, you will find layoutManager already reached zh-Hant-Zhuyin, but |app.inputMethodManager.currentIMEngine| stays at Pinyin.

I don't think this is caused by getText() rejects because |app.inputMethodManager._inputContextData| is null; so for this case it must be due to some racing in the stateManager, previously keyboard.js.

That said, putting console.log() in handleEvent() will kept this from reproducing, as :mnjul have previously discovered.

:mnjul, if fixing the case above will help you find the API issue, please clone another bug and fix it. It mighty be worthy to put an end-to-end protection to the async operations in state manager, in _updateActiveState.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

With the patch below, I caught an error at |window.e2|: "Database not ready!", which should come from here
https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/imes/jspinyin/jspinyin.js#L153

So the case in comment 10 should be a race in JSPinyin, not the keyboard app itself. That said, we should have better error handling to defend against IMEngine (so to save us time debugging). The best approach is to implement bug 1040611, but that is not possible in the short term.

=============

diff --git a/apps/keyboard/js/keyboard/state_manager.js b/apps/keyboard/js/keyboard/state_manager.js
index 1ab9fc8..62bc3b3 100644
--- a/apps/keyboard/js/keyboard/state_manager.js
+++ b/apps/keyboard/js/keyboard/state_manager.js
@@ -79,11 +79,11 @@ StateManager.prototype._updateActiveState = function(active) {
     // Switch the layout,
     this.app.layoutManager.switchCurrentLayout(this._layoutName)
       // ... switch the IMEngine,
-      .then(this._switchCurrentIMEngine.bind(this))
+      .then(this._switchCurrentIMEngine.bind(this), function(e) { window.e1 = e; return e; })
       // ... load the layout rendering,
-      .then(this._updateLayoutRendering.bind(this))
+      .then(this._updateLayoutRendering.bind(this), function(e) { window.e2 = e; return e; })
       // ... load l10n.js (if it's not loaded yet.)
-      .then(this.app.l10nLoader.load.bind(this.app.l10nLoader));
+      .then(this.app.l10nLoader.load.bind(this.app.l10nLoader), function(e) { window.e3 = e; return e; });
   } else {
     // Do nothing if we are already hidden.
     if (active === this._isActive) {

Comment 12

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Gecko patch that adds logging

Some more analysis that might be concluding.

** TL;DR: Since registration and unregistration of keyboards in Gecko (in MozKeyboard.js and Keyboard.jsm) is asynchronous, it is possible that unregistration of the old keyboard takes effect after the registration of the new keyboard does; the specific "effect" results in that MozInputContext (of MozKeyboard.js) cannot find the active keyboard to resolve getText() into. **

Observation:
1. I traced getText()'s call path and found that, when this bug occurs, Keyboard._keyboardMM is null during sendToKeyboard() call (as a result of |GetText:Result:OK| message), in http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.jsm#55-59 .
2. The null-ness is set during the handling of keyboard registration/unregistration messages, in http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.jsm#214,217 .
3. Because keyboard registration/unregistration messages are asynchronous: http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/MozKeyboard.js#259-273 , it is possible that the unregistration handling of the old (switched-out) keyboard takes place (at Keyboard.jsm) *after* the registration message of the new (switched-in) keyboard does. When this happens, the unregistration message will override the registration by means of setting Keyboard._keyboardMM to null, and the bug would occur.

You may apply the patch attached in this comment to see some logs. When you switch from demo keyboard to built-in keyboard, if the last related log from logcat is 'KB:Register', then this bug won't happen. If it's 'KB:Unregister', then this bug will happen; in this case you'll also see 'sendToKeyboard error: TypeError: this._keyboardMM is null' log, indicating that the Promise will not be resolved.

I'm going to propose a (somewhat band-aid) fix, where unregistration is ignored if the MessageManager stored in Keyboard._keyboardMM has already been associated with the new (switched-in) keyboard.

Comment 13

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

Comment on attachment 8466955 [details] [diff] [review]
Gecko patch that adds logging

Thanks for digging into this and finding the cause.

We need a fix to check the messageManager of each message from MozKeyboard.js to Keyboard.jsm. Besides "Keyboard:Unregister", any other messages from previously registered keyboard app should be ignored.

Comment 14

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to Yuan Xulei [:yxl] from comment #13)
> Comment on attachment 8466955 [details] [diff] [review]
> Gecko patch that adds logging
> 
> Thanks for digging into this and finding the cause.
> 
> We need a fix to check the messageManager of each message from
> MozKeyboard.js to Keyboard.jsm. Besides "Keyboard:Unregister", any other
> messages from previously registered keyboard app should be ignored.

Yep, I'm working on that right now.

Comment 15

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Proposed patch

So, I actually spent a lot of time writing the mochitest and making sure it reflects the bug. The major trouble with it is that when under mochitest, it appears that in Keyboard.jsm the MessageManagers for different keyboard frames actually equal each other (which is *not* the case in real device, where built-in KB frame and 3rd-party KB Frame have different MessageManagers in Keyboard.jsm); this results in that my draft patch that solely depends on MM-equality checking fails the test.

So I modified KB messages flying from mozInputMethod/mozInputContext to Keyboard.jsm to include KB frame URL (sans the hash part, since URLs only differing in hash should be of the same KB app). In Keyboard.jsm, if MessageManagers equal, the URLs will be checked to see if the messages are from a registered keyboard.

Tim, could you help push my patch to TBPL and see if things are good?
And Xulei, would you kindly review this patch?

Thanks so much to both of you.

Comment 16

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

Comment on attachment 8467657 [details] [diff] [review]
Proposed patch

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

I didn't review the mochitest test part, which I'm going to do tomorrow :-)

::: dom/inputmethod/Keyboard.jsm
@@ +160,5 @@
> +      kbURL = msg.data.kbURL;
> +      if (kbURL.indexOf('#') !== -1) {
> +        kbURL = kbURL.substr(0, kbURL.indexOf('#'));
> +      }
> +    }

The built-in keyboards are differentiated by their fragment URL, so if we remove the fragment parts, we cannot tell if the message is from an activated keyboard.

@@ +167,5 @@
> +        msg.name !== 'Keyboard:Register'){
> +      if (this._keyboardMM !== mm || this._keyboardURL !== kbURL) {
> +        return;
> +      }
> +    }

You could merge the above two if statements into one :)

::: dom/inputmethod/MozKeyboard.js
@@ +70,5 @@
>      if (!WindowMap.isActive(this._window)) {
>        return;
>      }
> +    cpmm.sendAsyncMessage('Keyboard:ShowInputMethodPicker', {
> +      kbURL: this._window.location.href

I think we need a constant ID for each keyboard app, where
window URL may not be a good candidate, since its value may vary during keyboard app life time. Keyboard app is allowed to change window's fragment URL, for example, changing the URL of "apps://xxx/index.html" to "apps://xxx/index.html#hello".

We might generate one instead.

Could you make a wrapper function for ccpmm.sendAsyncMessage so that the kbURL 
parameter will be set automatically.

Comment 17

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

push the patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=f753c0af5717

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f753c0af5717

Comment 18

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to Yuan Xulei [:yxl] from comment #16)
> ::: dom/inputmethod/Keyboard.jsm
> @@ +167,5 @@
> > +        msg.name !== 'Keyboard:Register'){
> > +      if (this._keyboardMM !== mm || this._keyboardURL !== kbURL) {
> > +        return;
> > +      }
> > +    }
> 
> You could merge the above two if statements into one :)

Yep, will do.

> Could you make a wrapper function for ccpmm.sendAsyncMessage so that the
> kbURL parameter will be set automatically.

Yep, I’m gonna do it.

I see in TBPL that all the M-3 tests are failing. I need to check into that.

Comment 19

[John Lu [:mnjul][Please NI or I'll miss]]

Regarding identifying a keyboard app with frame URL:
I originally wanted to use a keyboard frame’s dataset’s “manifestURL” (as set by Gaia, https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/keyboard_manager.js#L384 ) to identify a keyboard. However, as I didn’t know XPCOM very well, I couldn’t figure out a way to retrieve the frame reference (from msg.target and/or from messagemanager), and the dataset with it. Do you think you might be able to offer some insight on this?

Also, I think keyboards differing only from #hash in their URLs should not be considered different “keyboards” (in Keyboard.jsm’s sense) since they’re supposed to hold the same keyboard (only different layouts) with the same frame: In https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/keyboard_manager.js#L367-L373, when launching a new keyboard layout, Gaia always uses the same iframe from some old keyboard layout if the old layout has the same URL (up to “.html”, but without #hash) as the new one. Actually, register & unregistering don’t happen if Gaia’s Input Management knows the keyboards are from the same app/frame: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/keyboard_manager.js#L615-L618 . So the current strategy of differentiating keyboards should work. How do you think about this?

Comment 20

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

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #18)
...
> 
> Yep, I’m gonna do it.
> 
> I see in TBPL that all the M-3 tests are failing. I need to check into that.
That is caused by bug 1048423, not by your patch. And bug 1048423 has been fixed, so you don't need to check it now. :-)

Comment 21

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to Yuan Xulei [:yxl] from comment #20)
> (In reply to John Lu [:mnjul] [MoCoTPE] from comment #18)
> ...
> > 
> > Yep, I’m gonna do it.
> > 
> > I see in TBPL that all the M-3 tests are failing. I need to check into that.
> That is caused by bug 1048423, not by your patch. And bug 1048423 has been
> fixed, so you don't need to check it now. :-)

Unfortunately (some of) the test failures actually were caused by my patch :( I'm specifically talking about these failures:

429 INFO TEST-UNEXPECTED-FAIL | /tests/dom/inputmethod/mochitest/test_bug978918.html | Test timed out.
433 INFO TEST-UNEXPECTED-FAIL | /tests/dom/inputmethod/mochitest/test_bug978918.html | setSelectionRange failed:undefined
462 INFO TEST-UNEXPECTED-FAIL | /tests/dom/inputmethod/mochitest/test_sendkey_cancel.html | Test timed out.

Take test_bug978918 as example. This test would *not* fail if run alone. It fails if it is run after test_bug960946 (as in http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/mochitest/mochitest.ini#15-16 ). I reproduced the test failures by disabling other tests than the two.

I investigated and found out that test_bug978918 hung because it was waiting for setSelectionRange Promise to return. And the Promise could not be resolved because Keyboard.jsm actually thought the active keyboard was still test_bug960946. This was because MozKeyboard.js thought the window was active in http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/MozKeyboard.js#253-255, and further because test_bug960946 never did |SpecialPowers.wrap(im).setActive(false);| when it finished.

I tried to add |SpecialPowers.wrap(im).setActive(false);| in test_bug960946 when it finished, but that brought up quite a few more errors within the test -- presumably the test kinda didn't take those into account when Keyboard.jsm had not checked the origin of kb messages.

Anyway, I'm afraid that modifying all those tests would require quite some undertaking and is out of the scope of this bug, so I'm proposing that:

In this bug, let's only disregard 'Unregister' message according to the original investigations. I've just experimented this and tests seemed supper happy. Other messages would proceed as normal. This would allow this bug to be resolved. And let's open a follow-up bug that should deal with all other messages, and modify tests accordingly.

How does this proposal sound?

Comment 22

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

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #21)
> In this bug, let's only disregard 'Unregister' message according to the
> original investigations. I've just experimented this and tests seemed supper
> happy. Other messages would proceed as normal. This would allow this bug to
> be resolved. And let's open a follow-up bug that should deal with all other
> messages, and modify tests accordingly.
> 
> How does this proposal sound?

It is a way to solve this bug. We could make some improvement:

1. Let Keyboard.jsm to generate and assign ID for each MozInputMethod instance. 
The ID could be an auto increment integer.

2. For a new MozInputMethod instance, it will get its ID by sending "Register" 
message to Keyboard.jsm.

3. When MozInputMethod sending "Unregister" message to Keyboard.jsm, it should 
include its ID in the message.

4. Deactivating MozInputMethod won't clear its ID.

Comment 23

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to Yuan Xulei [:yxl] from comment #22)
> 2. For a new MozInputMethod instance, it will get its ID by sending
> "Register" message to Keyboard.jsm.

Hmm... I'm not sure how that's supposed to work -- that 'Register' is an async message from MozInputMethod to Keyboard.jsm, and thus to transmit the Keyboard.jsm-generated ID to MozInputMethod, we need to fire back an async message from Keyboard.jsm to MozInputMethod like Register:OK, right? (which might result in another asynchronicity-induced issue).

Comment 24

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

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #23)
> (In reply to Yuan Xulei [:yxl] from comment #22)
> > 2. For a new MozInputMethod instance, it will get its ID by sending
> > "Register" message to Keyboard.jsm.
> 
> Hmm... I'm not sure how that's supposed to work -- that 'Register' is an
> async message from MozInputMethod to Keyboard.jsm, and thus to transmit the
> Keyboard.jsm-generated ID to MozInputMethod, we need to fire back an async
> message from Keyboard.jsm to MozInputMethod like Register:OK, right? (which
> might result in another asynchronicity-induced issue).

Yes, if the "Register" message is async, we need to fire back an async "Register:OK" 
message and handle the case that "Unregister" needs to be sent before "Register:OK". 

Or we can send sync message for 'Register' as following (I prefer this way):

--- MozKeyboard.js  ---

// Change the cpmm to allow sending sync message.
XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
  "@mozilla.org/childprocessmessagemanager;1", "nsISyncMessageSender");

// Send sync message to Keyboard.jsm to get ID.
let id = cpmm.sendSyncMessage('Keyboard:Register', {})[0];


--- Keyboard.jsm ---

receiveMessage: function keyboardReceiveMessage(msg) {
  ...
  if (msg.name === 'Keyboard:Register') {
     let id = ...
     return id;
  }
  ...
}

Comment 25

[John Lu [:mnjul][Please NI or I'll miss]]

Thanks for your reply! I think I'll go with the sync message route. Hope that doesn't have much performance impact.

Comment 26

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

Comment on attachment 8467657 [details] [diff] [review]
Proposed patch

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

It is really a difficult job to write keyboard api test. Thanks for your work:)

setTimeout is one of the main causes of intermittent test failures, so please be careful when using setTimeout and do not use it unless necessary.

::: dom/inputmethod/mochitest/test_bug1043828.html
@@ +56,5 @@
> +   * 5. Allow frame b to use getText() with inputcontext to get the content from the text field
> +   *    iframe. Wait 200ms.
> +   * [Test would succeed if the Promise returned by getText() resolves correctly.
> +   *  Test would fail if otherwise]
> +   * 6. Set keyboard frame B as inactive. The Promise returned by getText() should be rejected, or

Step 6 is not necessary as step 5 can tell if the test is successful or not already. If step 5 fails, we will do nothing until we reach the mochitest 300 seconds time out.

@@ +134,5 @@
> +    keyboardA.setInputMethodActive(true);
> +
> +    setTimeout(function(){
> +      step3();
> +    }, WAIT_TIME);

The try server sometime would be quite slow, the time we wait here may be not sufficient. To avoid set long timeout, I suggest that we wait until keyboardA.setInputMethodActive succeeds before starting step 3. The code could be change to something like this:

  let req = keyboardA.setInputMethodActive(false);
  req.onsuccess = function() {
    ok(true, 'setInputMethodActive before loading succeeded.');
    setTimeout(function(){
      step3();
    }, WAIT_TIME);
  };

  req.onerror = function() {
    ok(false, 'setInputMethodActive before loading failed: ' + this.error.name);
    inputmethod_cleanup();
  };

Step 3 and 4 have the same problem.

@@ +168,5 @@
> +  // STEP 6: Set keyboard B inactive
> +  function step6() {
> +    keyboardB.setInputMethodActive(false);
> +
> +    lastTimeout = setTimeout(function(){

Typically we leave the mochitest framework to deal with the last timeout issue, so we can remove this timer.

Comment 27

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

cc Fabrice as he is experienced with ipc message with keyboard api.

Comment 28

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Proposed Patch 2

Comment 29

[John Lu [:mnjul][Please NI or I'll miss]]

(In reply to John Lu [:mnjul] [MoCoTPE] from comment #28)
> Created attachment 8468398 [details] [diff] [review]
> Proposed Patch 2

So here is the modified patch using auto-incrementing IDs generated by Keyboard.jsm, and synchronous message for registration. Only during unregistration is the ID being checked.

The test has been modified to address issues in comment 26. It still reflects the bug correctly.

Xulei, would you mind review the patch again for me? And please push it to TBPL for testing. Much appreciated !

Comment 30

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

Comment on attachment 8468398 [details] [diff] [review]
Proposed Patch 2

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

We're almost done.

Push to try:
https://tbpl.mozilla.org/?tree=Try&rev=c38af8327088
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c38af8327088

::: dom/inputmethod/Keyboard.jsm
@@ +21,5 @@
>  
>  this.Keyboard = {
> +  _formMM: null,      // The current web page message manager.
> +  _keyboardMM: null,  // The keyboard app message manager.
> +  _keyboardID: 0,     // The keyboard app's ID number.

If there is no active keyboard, we should set it to an invalid ID, such as -1.

@@ +64,5 @@
>      Services.obs.addObserver(this, 'inprocess-browser-shown', false);
>      Services.obs.addObserver(this, 'remote-browser-shown', false);
>      Services.obs.addObserver(this, 'oop-frameloader-crashed', false);
>  
> +    this._nextKeyboardID = 0;

We don't need to re-initialize it.

@@ +160,5 @@
> +    if ('kbID' in msg.data) {
> +      kbID = msg.data.kbID;
> +    }
> +
> +    if (msg.name === "Keyboard:Unregister" && this._keyboardID !== kbID) {

We should not allow any Keyboard:XXX message (except "Keyboard:Register") whose ID doesn't match to pass. It will be better to have a condition  statement as:
 if (msg.name.indexOf("Keyboard:") === 0 &&
     msg.name !== 'Keyboard:Register' &&
     this._keyboardID !== kbID) ...

@@ +236,3 @@
>          break;
>        case 'Keyboard:Unregister':
>          this._keyboardMM = null;

Set the _keyboardID to an invalid ID after unregistered.

::: dom/inputmethod/MozKeyboard.js
@@ +15,5 @@
>  XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
>    "@mozilla.org/childprocessmessagemanager;1", "nsIMessageSender");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "cpmms",
> +  "@mozilla.org/childprocessmessagemanager;1", "nsISyncMessageSender");

This could also be used to send async message, so you just need one message sender to send both sync and async messages.

@@ +306,5 @@
>        // a GetContext:Result:OK event, and we can initialize ourselves.
>        // Otherwise silently ignored.
> +
> +      // get keyboard ID from Keyboard.jsm
> +      let res = cpmms.sendSyncMessage('Keyboard:Register', {});

Check WindowMap to see if we already have an ID. If so, send an async 'Keyboard:Register' without require a new ID.

::: dom/inputmethod/mochitest/test_bug1043828.html
@@ +82,5 @@
> +    document.body.appendChild(keyboardB);
> +
> +    // simulate two different keyboard apps
> +    let imeUrl = basePath + '/file_inputmethod_1043828.html';
> +    let imeUrl2 = basePath + '/file_inputmethod_1043828_2.html';

Since we don't depend on the URL to identify keyboard apps, we can use the same URL for the two apps and cut one file_inputmethod_XXX.html.

Comment 31

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Proposed Patch 3

Here is the revised patch according to comment 30.

It appears that when we're using IDs generated by Keyboard.jsm, the test failures observed in comment 21 no longer occur even when all messages (except |Keyboard:Register|) are checked for matching kbID.

So we don't need to restrict the check for only |Keyboard:Unregister| message and don't need to open a follow-up bug like I'd said in comment 21 -- I hope TBPL won't disagree, of course ;)

Anyway, Xulei, thanks for your time and patience with me. Please review this latest patch and push to TBPL for me. Thanks a lot!

Comment 32

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Diff between Patch 3 and 2

Here is a diff between Patch 3 (attachment 8469076 [details] [diff] [review]) and Patch 2 (attachment 8468398 [details] [diff] [review]) for reviewer's reference.

Comment 33

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

Comment on attachment 8469076 [details] [diff] [review]
Proposed Patch 3

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

Thanks for providing interdiff to help review.
Let's wait for tbpl's rusult:

https://tbpl.mozilla.org/?tree=Try&rev=51e9dbea82cb

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=51e9dbea82cb

Comment 34

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Xulei,

Again, thanks for your assistance on this bug. I'm not familiar with Gecko's landing process so I'm asking -- it seems that the patch is testing well on TBPL. Although Mulet M-2 and M-4 are failing but we have M-2 failed tests tracked by another bug, and those pushes before mine have same failed tests in M-4. Shall we land this bug by adding "checkin-needed" here?

Thanks a lotttttt !

Comment 35

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

Mulet M-2 and M-4 are unlikely caused by this patch. So I think its OK to ask sheriff to check in :-)

Comment 36

[John Lu [:mnjul][Please NI or I'll miss]]

Here we go!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/64cf467276b4

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64cf467276b4

Comment 40

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

[Blocking Requested - why for this release]:
Nominate 2.0+ because we've got Bug 1069189 (v2.0+'ed) reported, and it has the same symptom as this one.

Comment 41

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

I've already seen conflict for this patch to go into b2g32.
Yuan, could you help guide us on how to uplift this patch to b2g-32 if this got 2.0+ (I assume it will be)?

Thanks in advance.

Comment 42

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

Attachment: Patch for v2.0

patch for v2.0.

Comment 43

[howie [:howie]]

Triage: blocking. Hi Rudy, please request uplift to 2.0, thanks.

Comment 44

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

Comment on attachment 8495820 [details] [diff] [review]
Patch for v2.0

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Cannot type after switching between the built-in keyboard and 3rd-party keyboard.
Testing completed: Yes, mochitest and manual testing.
Risk to taking this patch (and alternatives if risky): Should be low, stayed on m-c for about 2 months. 
String or UUID changes made by this patch: N/A.

Comment 45

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

The try run for this b2g-32 specific patch - attachment 8495820 [details] [diff] [review],
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a3a8a1bb1f84

Comment 46

[bhavana bajaj [:bajaj]]

Unless this is a regression we caused in 2.0 its late to land this there, I see the reasoning on this being uplifted for https://bugzilla.mozilla.org/show_bug.cgi?id=1069189. If woodduck, is not ready to ship without this, probably take this on 2.0M only.

Comment 47

[bhavana bajaj [:bajaj]]

Comment on attachment 8495820 [details] [diff] [review]
Patch for v2.0

Clearing the b2g32 request given this is landing on 2.0M

Comment 48

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

Set checkin-needed for attachment 8495820 [details] [diff] [review].

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/ac0060f0ea13

Comment 50

[Keven Kuo [:kkuo]]

(In reply to bhavana bajaj [:bajaj] from comment #46)
> Unless this is a regression we caused in 2.0 its late to land this there, I
> see the reasoning on this being uplifted for
> https://bugzilla.mozilla.org/show_bug.cgi?id=1069189. If woodduck, is not
> ready to ship without this, probably take this on 2.0M only.

It doesn't make sense to me, "Unless this is a regression we caused in 2.0 its late to land this there".
This is a generic bug and a partner blocker. It should be 2.0+. 

--
Keven

Comment 51

[bhavana bajaj [:bajaj]]

(In reply to Keven Kuo [:kkuo] from comment #50)
> (In reply to bhavana bajaj [:bajaj] from comment #46)
> > Unless this is a regression we caused in 2.0 its late to land this there, I
> > see the reasoning on this being uplifted for
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1069189. If woodduck, is not
> > ready to ship without this, probably take this on 2.0M only.
> 
> It doesn't make sense to me, "Unless this is a regression we caused in 2.0
> its late to land this there".
> This is a generic bug and a partner blocker. It should be 2.0+. 
> 
> --
> Keven

My question is is this a new regression in 2.0 or did the past releases have this bug in which case it will not be a sever enough bug to land it on 2.0 at this point. It looks like we've shipped with this in 1.4 already and its not a new regression at least from the bug comments.

For [partner-blocker], looks like it already landed on woodduck.

Comment 52

[Josh Cheng [:josh]]

Norry,
PLease verify the fix on Woodduck 2.0M. Thanks!

Comment 53

[Norry.L.F(Leave from Mozilla)]

Hi Josh，

The bug can't be reproed on Woodduck 2.0M.

Gaia-Rev        ff24db33f8ebb275b200030035b851701b617b80
Gecko-Rev       e7df4dde2d9dbedee942333d34eaea2afe32bebc
Build-ID        20141023050313
Version         32.0
Device-Name     soul35
FW-Release      4.4.2
FW-Incremental  1414011955
FW-Date         Thu Oct 23 05:06:18 CST 2014

Comment 54

[Norry.L.F(Leave from Mozilla)]

(In reply to Norry.L.F from comment #53)
> Hi Josh，
> 
> The bug can't be reproed on Woodduck 2.0M.
> 
> Gaia-Rev        ff24db33f8ebb275b200030035b851701b617b80
> Gecko-Rev       e7df4dde2d9dbedee942333d34eaea2afe32bebc
> Build-ID        20141023050313
> Version         32.0
> Device-Name     soul35
> FW-Release      4.4.2
> FW-Incremental  1414011955
> FW-Date         Thu Oct 23 05:06:18 CST 2014

Repro times: 20, can't repro

Comment 55

[Lance（leave from Mozilla）]

This issue has been verified successfully on Flame 2.1. For the 3rd-party keyboards can't be displayed in IME, we can't verify this issue on Flame 2.2.
See attachment: 1658.MP4
Reproducing rate: 0/5

Step:
1.Launch Marketplace.
2.Search "Greek Alphabet Keyboard" to install.
3.Launch Settings -> "Keyboards" -> "Select Keyboards".
4.Check "Number", "English", "France", "拼音" and "Greek Alphabet".
5.Back to Home, launch Message, create new message.
6.Tap content box to invoke IME.
7.Cycle through the keyboards with the "En" / "Fr" / "拼" or globe button.

Actual result:
You can cycle all keyboards. 

Flame 2.1 version:
Gaia-Rev        5655269098c7e82254e56933f1af05b4abe2a2f3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-2g34_v2_1/rev/86608c9389b5
Build-ID        20141204001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141204.034958
FW-Date         Thu Dec  4 03:50:09 EST 2014
Bootloader      L1TC00011880

Comment 56

[Lance（leave from Mozilla）]

Attachment: 1648.MP4