Closed Bug 1395876 Opened 2 years ago Closed 2 years ago

[TSF] If QQ input is default IME and Firefox launched with -no-remote, cannot use IME unless selecting another IME/keyboard layout

Categories

(Core :: Widget: Win32, defect, major)

All
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(1 file)

QQ input is a Chinese IME which is a TIP of TSF:
http://qq.pinyin.cn/

As far as I tested on Win7 with QQ input, I see a bug similar to bug 1390097.

When Firefox is launched with -no-remote, "intl.tsf.enable" is true and QQ input is the default IME, QQ input isn't "enabled" even after fixing bug 1390097.

With mozregressions, I reached bug 1354020 as regression point. So, the patch for bug 1354020 still has some problems.

When I switch active IME from QQ input to another keyboard layout or IME and switch it back to QQ input, it's "enabled correctly.

I guess that QQ input has a bug since other TIPs don't have this bug as far as we know. I wonder, can we avoid this issue in TSFTextStore or something?
Severity: normal → major
Also reproduced on Win10.
Does it fix the issue to disable PerMonitorV2?
(In reply to Masatoshi Kimura [:emk] from comment #2)
> Does it fix the issue to disable PerMonitorV2?

Did you mean backing out the patch? Looks like there is no pref to disable it. And I can reproduce this bug on Win7. IIRC, Win7 doesn't support PerMonitorV2.
Why is this bug blocking bug 1354020, then?
Because of reaching the changeset when I use mozregressions.
That's really odd... Could you confirm that reverting bug 1354020 fixes the issue?
Even if removing PerMonitorV2 from manifest, this still occur.  By bug 1390097, some changes are already reverted, so I think that this occurs by another changes...
regression by bug 1341915?
Hmm, still not sure. I reproduced this bug with the trybuild. So, I failed to test with mozregressions.

FYI: When I tested this with the trybuild first time, I couldn't reproduce this bug. I don't understand why this is not 100% reproducible... The security dialog which blocks to run untrustable exe file could affect this bug?
No longer blocks: 1354020
This seems to be fixed by backing out bug 1341915.
Blocks: 1341915
This is regression by bug 1341915 as long as I test on my environment.  Do you have another idea without backed out?
Flags: needinfo?(aklotz)
Okay, perhaps, I got the reason. When we initialize TSF modules, we've not created "normal" window yet, there is perhaps only the message window after bug 1341915. We initialize TSF modules after creating first top level window, it works fine. I keep investigating and testing this more.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(aklotz)
FYI: However, this is really not related to bug 1390097. Even with the hack, I still see IMM-IME issue if I backed out the backout patch of bug 1390097.
I believe that the fix is really risky. So, it must be too late for Beta.
Comment on attachment 8905848 [details]
Bug 1395876 - Initialize TSF modules after we create first normal window

https://reviewboard.mozilla.org/r/177658/#review182970

::: widget/windows/nsWindow.cpp:946
(Diff revision 1)
>      }
> +    // Some TIP and IMM may want a non-message window to initialize itself.
> +    // So, don't call this in the constructor of nsWindow.
> +    IMEHandler::Initialize();
>    }
> +

We might have tgo add assersion when IMEHandler::Initialize isn't called.
Attachment #8905848 - Flags: review?(m_kato) → review+
Comment on attachment 8905848 [details]
Bug 1395876 - Initialize TSF modules after we create first normal window

https://reviewboard.mozilla.org/r/177658/#review182970

> We might have tgo add assersion when IMEHandler::Initialize isn't called.

What did you mean? IMEHandler::Initialize() just calls TSFTextStore::Initialize() and IMMHandler::Initialize(). So, it's stateless. Should IMMHandler has a bool flag if it's already been initialized? However, the local static variable, sFirstTopLevelWindowCreated must be enough to guarantee that it's called only once.

What do you worry about and want to avoid with an assertion?
Comment on attachment 8905848 [details]
Bug 1395876 - Initialize TSF modules after we create first normal window

https://reviewboard.mozilla.org/r/177658/#review182970

> What did you mean? IMEHandler::Initialize() just calls TSFTextStore::Initialize() and IMMHandler::Initialize(). So, it's stateless. Should IMMHandler has a bool flag if it's already been initialized? However, the local static variable, sFirstTopLevelWindowCreated must be enough to guarantee that it's called only once.
> 
> What do you worry about and want to avoid with an assertion?

It is possible that IMEHandler::InitInputContext is called witout IMEHandler::Initialize.  (But I think it may be no situation now , because first window will be top level window).  If this situation occurs, does IME work well?
Comment on attachment 8905848 [details]
Bug 1395876 - Initialize TSF modules after we create first normal window

https://reviewboard.mozilla.org/r/177658/#review182970

> It is possible that IMEHandler::InitInputContext is called witout IMEHandler::Initialize.  (But I think it may be no situation now , because first window will be top level window).  If this situation occurs, does IME work well?

Ah, I understand. Okay, perhaps, I should stop changing this block. Instead, IMEHandler::InitInputContext() should initialize IMEHadler::Initialize() only when the first time with local static bool variable because IMEHandler::InitInputContext() requires an HWND of an nsWindow instance. So, it's guaranteed that there is at least one normal window when it's called. How do you think?
Comment on attachment 8905848 [details]
Bug 1395876 - Initialize TSF modules after we create first normal window

https://reviewboard.mozilla.org/r/177658/#review182970

> Ah, I understand. Okay, perhaps, I should stop changing this block. Instead, IMEHandler::InitInputContext() should initialize IMEHadler::Initialize() only when the first time with local static bool variable because IMEHandler::InitInputContext() requires an HWND of an nsWindow instance. So, it's guaranteed that there is at least one normal window when it's called. How do you think?

good, no problem.
Attachment #8905848 - Flags: review+ → review?(m_kato)
Attachment #8905848 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/86465f8cfaf3
Initialize TSF modules after we create first normal window r=m_kato
https://hg.mozilla.org/mozilla-central/rev/86465f8cfaf3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.