1050660 - setInputMethodActive doesn't sometimes work when switching IME imminently after boot

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Makoto Kato [:m_kato]]

This is reported from 3rd party developer that develops IME.

- Env
Flame with Firefox OS v2.0 or v2.1

- Step
1. enable the following IME

   - Built-in Keyborad: English
   - Built-in Keyborad: Number
   - Demo Keyboard: English

2. Reboot Firefox OS device
3. Tap [Search or enter address] textbox on home screen.
4. Tap [En...] button to switch IME.  Then it switches to Demo Keyboard.
5. Tap key button

- Result
Any key doesn't work.  Then you tap any key, you can find this following log.

E/GeckoConsole( 1134): [JavaScript Error: "TypeError: this._context is undefined" {file: "app://demo-keyboard.gaiamobile.org/js/input_field.js" line: 204}]


This is a race condition of _sendDOMRequest and this._mm.addMessageListener().  Before "hello" message, setInputMethodActive is called, then sendAsyncMessage will return NS_ERROR_NOT_INITIALIZED due to no registered callback.

So we should wait sending all API call before "hello" is received.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: don't allow sendAsyncMessage until hello is received

Comment 2

[Makoto Kato [:m_kato]]

Comment on attachment 8470640 [details] [diff] [review]
don't allow sendAsyncMessage until hello is received

There is a race condition of _sendDOMRequest and this._mm.addMessageListener().  Before "hello" message, setInputMethodActive is called, then sendAsyncMessage will return NS_ERROR_NOT_INITIALIZED due to no registered callback.

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8470640 [details] [diff] [review]
don't allow sendAsyncMessage until hello is received

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

lgtme, but can you push to try before landing? thanks!

::: dom/browser-element/BrowserElementParent.jsm
@@ +1,1 @@
> +/* vim: set ts=2 et sw=2 tw=80 ft=javascript: */

nit: remove this change.

Comment 4

[Makoto Kato [:m_kato]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Comment on attachment 8470640 [details] [diff] [review]
> don't allow sendAsyncMessage until hello is received
> 
> Review of attachment 8470640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtme, but can you push to try before landing? thanks!

I already test on try server before review.

> 
> ::: dom/browser-element/BrowserElementParent.jsm
> @@ +1,1 @@
> > +/* vim: set ts=2 et sw=2 tw=80 ft=javascript: */
> 
> nit: remove this change.

Thanks

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Makoto Kato (:m_kato) from comment #4)
> (In reply to Fabrice Desré [:fabrice] from comment #3)
> > Comment on attachment 8470640 [details] [diff] [review]
> > don't allow sendAsyncMessage until hello is received
> > 
> > Review of attachment 8470640 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > lgtme, but can you push to try before landing? thanks!
> 
> I already test on try server before review.

In this case, please post the link to the try build when you ask for review.

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> (In reply to Makoto Kato (:m_kato) from comment #4)
> > (In reply to Fabrice Desré [:fabrice] from comment #3)
> > > Comment on attachment 8470640 [details] [diff] [review]
> > > don't allow sendAsyncMessage until hello is received
> > > 
> > > Review of attachment 8470640 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > lgtme, but can you push to try before landing? thanks!
> > 
> > I already test on try server before review.
> 
> In this case, please post the link to the try build when you ask for review.

OK.  I will do it next.

try server url was the following.  https://tbpl.mozilla.org/?tree=Try&rev=d323b8f4ef49

Comment 7

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5b8f52a76065

Comment 8

[Makoto Kato [:m_kato]]

backout.  strange mochitest-2 failure on Linux only.  I am investigating....
https://hg.mozilla.org/integration/b2g-inbound/rev/f56231d7ed71

Comment 9

[Makoto Kato [:m_kato]]

failure test has bug.  setVisible is async on e10s, so we should not call removeChild before setVisible is completed.

Comment 10

[Makoto Kato [:m_kato]]

Attachment: fix testcase

Don't call removeChild immediately after calling setVisible.  setVisible on remote browser is async method, so we should wait until it sends child process.

Comment 11

[Makoto Kato [:m_kato]]

passed
https://tbpl.mozilla.org/?tree=Try&rev=d5f63a58c248

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8473435 [details] [diff] [review]
fix testcase

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

::: dom/browser-element/mochitest/priority/test_BackgroundLRU.html
@@ +56,5 @@
>  
> +  }).then(function() {
> +    // Don't call removeChild immediately after calling setVisible.
> +    // setVisible on remote browser is async method, so we should wait
> +    // until it sends child process. 

nit: whitespace at EOL

"sends to the child process"

Comment 13

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c42a34184e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e9920fe407

also, try: https://tbpl.mozilla.org/?tree=Try&rev=c79d5c80f279

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c42a34184e0
https://hg.mozilla.org/mozilla-central/rev/f7e9920fe407