Closed Bug 1181714 Opened 9 years ago Closed 9 years ago

[TSF] crash in _invalid_parameter_noinfo with Microsoft Office Korean IME

Categories

(Core :: Widget: Win32, defect)

40 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED INCOMPLETE
mozilla40
Tracking Status
firefox39 --- unaffected
firefox40 + disabled
firefox41 + unaffected
firefox42 + unaffected

People

(Reporter: away, Assigned: masayuki)

References

Details

(Keywords: crash, inputmethod)

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-564dc4c2-b7fe-47f5-bacb-c41292150705.
=============================================================

This is a top crash on 40 beta. About 80% of crashes have Korean locale, and most URLs are on http://map.daum.net or http://search.daum.net.

0040e638 7296ccd5 msvcr90!_invalid_parameter_noinfo+0xc
0040e734 768566de msvcr90!_invalid_parameter_noinfo+0xc
[possibly some frames in imetip.dll]
0040e760 7686dfd1 msctf!CThreadInputMgr::_NotifyCallbacks+0xfa
0040e7a8 76871bce msctf!CThreadInputMgr::_SetFocus+0x183
0040e7c0 5879d6bf msctf!CThreadInputMgr::SetFocus+0x2e
0040e7e8 587a23ca xul!nsTextStore::CreateAndSetFocus+0xf6
0040e864 57c2932e xul!nsTextStore::OnFocusChange+0x117
0040e8e8 587bdcb0 xul!mozilla::widget::IMEHandler::NotifyIME+0x31dd60
0040e8ec 58786b86 xul!nsWindow::NotifyIMEInternal+0x9
0040e8fc 5851da0b xul!nsBaseWidget::NotifyIME+0x54

:masayuki, can you think of any IME changes in version 40 that may have triggered this?
Flags: needinfo?(masayuki)
Adding a tracking flag for FF41 and FF42. We should also try to fix this FF40 as this is where we see the issue predominantly.
I think so. This must be a bug of TIP or TSF.

FYI: TSF will be disabled in 40 next week (bug 1181042).
Flags: needinfo?(masayuki)
Hmm, I cannot reproduce this with Korean IME which is included in Win8.1... I suspect that this is a bug of third party's IME. I need exact STR for this.
Um, but according to the loaded dlls, it seems that MS-IME 2010 is used...
Odd.. I cannot reproduce this bug on Windows7-KR (just installed).
Component: General → Widget: Win32
channy:

Do you reproduce this bug? Or do you see some complain about this bug in Korean user community? I want more detail how to reproduce this bug.
Flags: needinfo?(channy)
IMKR14.IME is 14.0.4734.1000.  This IME is on Office 2010?
Perhaps. But I cannot install it into my Win8.1 environment because of the IME's limitation. And also I cannot install it to Win7 on VMware due to license issue...
Summary: crash in _invalid_parameter_noinfo with Korean IME → crash in _invalid_parameter_noinfo with Microsoft Office Korean IME
Keywords: inputmethod
Summary: crash in _invalid_parameter_noinfo with Microsoft Office Korean IME → [TSF] crash in _invalid_parameter_noinfo with Microsoft Office Korean IME
Attached patch Patch (obsolete) — Splinter Review
Looks like that the crash occurs during releasing old focused document. I hope that this fixes the bug. But who can test this?? Should we uplift this to Beta immediately for testing?

FYI: If e10s is enabled, TSF is currently disabled. So, only Beta users hit this with default settings.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(dmajor)
Comment on attachment 8631455 [details] [diff] [review]
Patch

Anyway, I'd like to ask Kato-san.

# Although, the comment should be added later if this patch actually fixes this bug.
Attachment #8631455 - Flags: review?(m_kato)
Comment on attachment 8631455 [details] [diff] [review]
Patch

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

This crash may be into ITfThreadMgrEventSink::OnSetFocus(), so ITfDocumentMgr of OnSetFocus() seems to be incorrect.

I am not sure that this can fix by this, but if imetip doesn't hold refcount of previous ITfDocumentMgr, it can fix.
Attachment #8631455 - Flags: review?(m_kato) → review+
Microsoft Office Pinyin IME 2010 can also make firefox crash.
https://crash-stats.mozilla.com/report/index/186dc832-5e68-46d5-8646-ee1822150709

Tried the test build, and it didn't crash anymore
Kato-san:

Thanks, let's try.

(In reply to Xu Zhen from comment #14)
> Microsoft Office Pinyin IME 2010 can also make firefox crash.
> https://crash-stats.mozilla.com/report/index/186dc832-5e68-46d5-8646-
> ee1822150709
> 
> Tried the test build, and it didn't crash anymore

Oh, thank you for the feedback!!! Looks like exactly same bug!
Whiteboard: [leave open]
Comment on attachment 8631455 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Making TSF enabled in release builds (bug 478029)
[User impact if declined]: This is a topcrash
[Describe test coverage new/current, TreeHerder]: Just landed into mc-inbound. But comment 14 claims that this patch fixes this bug.
[Risks and why]: Very low risk since just holding previous focused object after we finish to set focus to new object.
[String/UUID change made/needed]: nothing
Flags: needinfo?(dmajor)
Attachment #8631455 - Flags: approval-mozilla-beta?
Attachment #8631455 - Flags: approval-mozilla-aurora?
After the patch actually fixes this bug on beta, I'll land the comment and close this bug.
Attached patch Patch (r=m_kato)Splinter Review
Approval Request Comment: See comment 16.

This patch was actually landed into the m-i.
Attachment #8631455 - Attachment is obsolete: true
Attachment #8631455 - Flags: approval-mozilla-beta?
Attachment #8631455 - Flags: approval-mozilla-aurora?
Attachment #8631523 - Flags: approval-mozilla-beta?
Attachment #8631523 - Flags: approval-mozilla-aurora?
Comment on attachment 8631523 [details] [diff] [review]
Patch (r=m_kato)

This is a straightforward fix. Let's take this in beta3 to prove that we've fixed the crash. Beta+ Aurora+
Attachment #8631523 - Flags: approval-mozilla-beta?
Attachment #8631523 - Flags: approval-mozilla-beta+
Attachment #8631523 - Flags: approval-mozilla-aurora?
Attachment #8631523 - Flags: approval-mozilla-aurora+
Whiteboard: [leave open]
Target Milestone: --- → mozilla42
Xu Zhen:

Do you reproduce this bug with Developer Edition (Aurora 41)?
# Note that this is reproducible when (intl.tsf.enable is true and e10s is disabled) or (intl.tsf.force_enable is true).

https://www.mozilla.org/en-US/firefox/channel/#developer
Flags: needinfo?(xuzhen)
If this isn't reproducible 41 or later, bug 1167022, bug 1172466 or bug 1175392 may have fixed this bug already.
No
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> Xu Zhen:
> 
> Do you reproduce this bug with Developer Edition (Aurora 41)?
> # Note that this is reproducible when (intl.tsf.enable is true and e10s is
> disabled) or (intl.tsf.force_enable is true).
> 
> https://www.mozilla.org/en-US/firefox/channel/#developer

Tried many times, firefox 41.0a2(2015-07-11) hasn't crashed yet.
Flags: needinfo?(xuzhen)
(In reply to Xu Zhen from comment #28)
> No
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> > Xu Zhen:
> > 
> > Do you reproduce this bug with Developer Edition (Aurora 41)?
> > # Note that this is reproducible when (intl.tsf.enable is true and e10s is
> > disabled) or (intl.tsf.force_enable is true).
> > 
> > https://www.mozilla.org/en-US/firefox/channel/#developer
> 
> Tried many times, firefox 41.0a2(2015-07-11) hasn't crashed yet.

Thank you. Then, this bug must have been fixed on Aurora. This bug should be only in Beta (and Release). Beta's TSF will be disabled in Beta5. So, we don't need to work anymore in this bug.
Depends on: 1181042
Flags: needinfo?(channy)
Target Milestone: mozilla42 → mozilla40
Resolving this as incomplete only because I can't think of a better status for this based on the last comment in the bug.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → INCOMPLETE
(In reply to Xu Zhen from comment #32)
> crash again with 41.0 beta 3
> https://crash-stats.mozilla.com/report/index/58d2dd19-be14-437c-a9e7-
> c545d2150823

That's wired. You said 41 doesn't have this bug (comment 28). How often do you reproduce this?
Flags: needinfo?(xuzhen)
It happened only once since upgrading to 41 beta, and I can't reproduce it again.
Flags: needinfo?(xuzhen)
Okay, then, we should keep current plan (releasing TSF mode in 41). We should try to fix this when we get a report with STR.
You need to log in before you can comment on or make changes to this bug.