Closed Bug 1050660 Opened 6 years ago Closed 6 years ago

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

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: m_kato, Assigned: m_kato)

Details

Attachments

(2 files)

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.
Assignee: nobody → 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.
Attachment #8470640 - Flags: review?(fabrice)
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.
Attachment #8470640 - Flags: review?(fabrice) → review+
(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
(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.
(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
https://hg.mozilla.org/integration/b2g-inbound/rev/5b8f52a76065
OS: Windows 8.1 → Gonk (Firefox OS)
Hardware: x86 → All
Target Milestone: --- → mozilla34
backout.  strange mochitest-2 failure on Linux only.  I am investigating....
https://hg.mozilla.org/integration/b2g-inbound/rev/f56231d7ed71
failure test has bug.  setVisible is async on e10s, so we should not call removeChild before setVisible is completed.
Attached patch fix testcaseSplinter Review
Don't call removeChild immediately after calling setVisible.  setVisible on remote browser is async method, so we should wait until it sends child process.
Attachment #8473435 - Flags: review?(khuey)
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"
Attachment #8473435 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/8c42a34184e0
https://hg.mozilla.org/mozilla-central/rev/f7e9920fe407
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.