Closed
Bug 1050660
Opened 11 years ago
Closed 11 years ago
setInputMethodActive doesn't sometimes work when switching IME imminently after boot
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: m_kato, Assigned: m_kato)
Details
Attachments
(2 files)
5.72 KB,
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•11 years ago
|
Assignee: nobody → m_kato
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
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 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•11 years ago
|
||
(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•11 years ago
|
||
(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.
Assignee | ||
Comment 6•11 years ago
|
||
(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
Assignee | ||
Comment 7•11 years ago
|
||
OS: Windows 8.1 → Gonk (Firefox OS)
Hardware: x86 → All
Target Milestone: --- → mozilla34
Assignee | ||
Comment 8•11 years ago
|
||
backout. strange mochitest-2 failure on Linux only. I am investigating....
https://hg.mozilla.org/integration/b2g-inbound/rev/f56231d7ed71
Assignee | ||
Comment 9•11 years ago
|
||
failure test has bug. setVisible is async on e10s, so we should not call removeChild before setVisible is completed.
Assignee | ||
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c42a34184e0
https://hg.mozilla.org/mozilla-central/rev/f7e9920fe407
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•