Closed Bug 1208969 Opened 4 years ago Closed 4 years ago

[TSF] When the active IME is Google Japanese Input, sometimes crashes at clicking middle and right buttons on tabbar

Categories

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

41 Branch
x86
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 - wontfix
firefox42 + fixed
firefox43 + fixed
firefox44 + fixed
firefox-esr38 --- unaffected

People

(Reporter: masayuki, Assigned: masayuki)

Details

(Keywords: crash, regression)

Attachments

(1 file)

Unfortunately, this crash never causes launching crash reporter. However, when I reproduce this bug on Aurora, the stack indicates that mSink->OnLayoutChange(TS_LC_DESTROY) causes new document lock infinitely. I'm not sure if the stack is trustable, but actually this tells our bug. I think that we should not grant to lock during destroying the context.
We're notifying TSF of destroying layout from TSFTextStore::Destroy():

http://mxr.mozilla.org/mozilla-central/source/widget/windows/TSFTextStore.cpp?rev=dec41eaf2fed&mark=1426-1426#1399

At sending this, Google Japanese Input tries to query content without checking the notification reason:

https://github.com/google/mozc/blob/fd0f5b347d35fe287f4cc9be49f650c87f82ba03/src/win32/tip/tip_text_service.cc#L921

This must be the bug of both Google Japanese Input and Gecko. Gecko shouldn't grant to lock the destroying document if the context has started to be destroyed.

I asked to users who can reproduce this bug high frequently. They said that the patched build fixes this bug.
https://twitter.com/inukai192/status/648520672843444224
https://twitter.com/raxnxt/status/648669112281923584


# This patch is enough simple and safe. So, I think that we can uplift at least Beta. If we have a change, I'd like to uplift to release too.
Attachment #8667068 - Flags: review?(VYV03354)
The patch also fixes a crash bug when user opens TweetDeck. Anyway, we don't have much information about the importance since this crash has never reported with breakpad.
Keywords: regression
[Tracking Requested - why for this release]:
New regression crash bug of 41 due to TSF mode. This is caused by two bugs one is Google Japanese Input not checking layout reason, the other is Gecko not banning requests of document lock even after TSFTextStore started to be destroyed. The proposal patch fixes the latter issue.

Unfortunately, this crash isn't detected by breakpad. Therefore, we cannot know actual number of victims. So, at least, I'd like to uplift the patch to Beta. But if it's possible, I'd like to fix this bug on 41.0.x.
Attachment #8667068 - Flags: review?(VYV03354) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> At sending this, Google Japanese Input tries to query content without
> checking the notification reason:
> 
> https://github.com/google/mozc/blob/fd0f5b347d35fe287f4cc9be49f650c87f82ba03/
> src/win32/tip/tip_text_service.cc#L921

Just for the record, I'm not yet sure whether calling ITfContext::RequestEditSession from ITextStoreACPSink::OnLayoutChange handler when layout_code == TS_LC_DESTROY is a bug or not.  IIUC, the sequence would be as follows:

 1.  Firefox enters TSFTextStore::Destroy.
 2.    Firefox calls ITextStoreACPSink::OnLayoutChange with layout_code == TS_LC_DESTROY
 3.      Google Japanese Input enters TipTextServiceImpl::OnLayoutChange with layout_code == TS_LC_DESTROY
 4.        Google Japanese Input calls OnLayoutChangedAsyncImpl.
 5.          Google Japanese Input calls ITfContext::RequestEditSession with TF_ES_ASYNCDONTCARE.
 6.            Firefox enters TSFTextStore::RequestLock.
 7.              Firefox calls ITextStoreACPSink::OnLockGranted or schedule the task with TS_S_ASYNC.
 8.            Firefox exits TSFTextStore::RequestLock.
 9.          Google Japanese Input returns from ITfContext::RequestEditSession.
10.       Google Japanese Input returns from OnLayoutChangedAsyncImpl.
11.     Google Japanese Input exits TipTextServiceImpl::OnLayoutChange.
12.   Firefox returns from ITextStoreACPSink::OnLayoutChange.
13. Firefox executes the rest of TSFTextStore::Destroy.

Correct me if I'm wrong, but I think the ITfContext (not view) here should still be valid between between step 2 and step 12 since TS_LC_DESTROY means that "The view is about to be destroyed" according to the document.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms629200.aspx
I don't think it implies that ITfContext is already destroyed and TIP is no longer able to access to ITfContext anymore.

Note that basically TIPs can know when the active document is just destroyed with ITfThreadMgrEventSink::OnUninitDocumentMgr, which should be triggered here.
http://mxr.mozilla.org/mozilla-central/source/widget/windows/TSFTextStore.cpp?rev=dec41eaf2fed&mark=1426-1426#1434

Thus probably the following argument would make sense.  I'm fine with above change.

  Google Japanese Input should make sure that its UI elements are updated
  ITfThreadMgrEventSink::OnUninitDocumentMgr no matter if it receives
  ITextStoreACPSink::OnLayoutChange with TS_LC_DESTROY or not.

See "Remarks" in the following document.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms538899.aspx
(In reply to Yohei Yukawa from comment #5)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> > At sending this, Google Japanese Input tries to query content without
> > checking the notification reason:
> > 
> > https://github.com/google/mozc/blob/fd0f5b347d35fe287f4cc9be49f650c87f82ba03/
> > src/win32/tip/tip_text_service.cc#L921
> 
> Just for the record, I'm not yet sure whether calling
> ITfContext::RequestEditSession from ITextStoreACPSink::OnLayoutChange
> handler when layout_code == TS_LC_DESTROY is a bug or not.  IIUC, the
> sequence would be as follows:
> 
>  1.  Firefox enters TSFTextStore::Destroy.
>  2.    Firefox calls ITextStoreACPSink::OnLayoutChange with layout_code ==
> TS_LC_DESTROY
>  3.      Google Japanese Input enters TipTextServiceImpl::OnLayoutChange
> with layout_code == TS_LC_DESTROY
>  4.        Google Japanese Input calls OnLayoutChangedAsyncImpl.
>  5.          Google Japanese Input calls ITfContext::RequestEditSession with
> TF_ES_ASYNCDONTCARE.
>  6.            Firefox enters TSFTextStore::RequestLock.
>  7.              Firefox calls ITextStoreACPSink::OnLockGranted or schedule
> the task with TS_S_ASYNC.
>  8.            Firefox exits TSFTextStore::RequestLock.
>  9.          Google Japanese Input returns from
> ITfContext::RequestEditSession.
> 10.       Google Japanese Input returns from OnLayoutChangedAsyncImpl.
> 11.     Google Japanese Input exits TipTextServiceImpl::OnLayoutChange.
> 12.   Firefox returns from ITextStoreACPSink::OnLayoutChange.
> 13. Firefox executes the rest of TSFTextStore::Destroy.
> 
> Correct me if I'm wrong, but I think the ITfContext (not view) here should
> still be valid between between step 2 and step 12 since TS_LC_DESTROY means
> that "The view is about to be destroyed" according to the document.
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms629200.aspx

Perhaps, you're right. Thinking from ITfContext, only the view is destroyed but ITfContext is still available.

> I don't think it implies that ITfContext is already destroyed and TIP is no
> longer able to access to ITfContext anymore.

Sure.

> Note that basically TIPs can know when the active document is just destroyed
> with ITfThreadMgrEventSink::OnUninitDocumentMgr, which should be triggered
> here.
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/TSFTextStore.
> cpp?rev=dec41eaf2fed&mark=1426-1426#1434
> 
> Thus probably the following argument would make sense.  I'm fine with above
> change.
> 
>   Google Japanese Input should make sure that its UI elements are updated
>   ITfThreadMgrEventSink::OnUninitDocumentMgr no matter if it receives
>   ITextStoreACPSink::OnLayoutChange with TS_LC_DESTROY or not.
> 
> See "Remarks" in the following document.
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms538899.aspx

I think that, in strictly speaking, TSFTextStore::GetActiveView() should return invalid value or an error code (not documented in https://msdn.microsoft.com/en-us/library/windows/desktop/ms538420.aspx) and other methods which take TsViewCookie should return error for TEXTSTORE_DEFAULT_VIEW or the invalid value. Then, anyway, even if TIP can lock the document, each method will fail (but the change is too risky for uplift). Therefore, I still think that after TS_LC_DESTROY, TIP should do nothing with the TextStore.
I do not believe this bug meets the bar for 41, too late, it will be a wontfix.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> I think that, in strictly speaking, TSFTextStore::GetActiveView() should
> return invalid value or an error code (not documented in
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms538420.aspx) and
> other methods which take TsViewCookie should return error for
> TEXTSTORE_DEFAULT_VIEW or the invalid value. Then, anyway, even if TIP can
> lock the document, each method will fail (but the change is too risky for
> uplift). Therefore, I still think that after TS_LC_DESTROY, TIP should do
> nothing with the TextStore.

Keep in mind that there are so many things that can do without
ITfContextView/TsViewCookie.  Here are examples:
- Request document lock.
- Read data from TextStore.
- Write date to TextStore.

What we are not able to do without TsViewCookie/ITfContextView are:
- Get the character bounds with ITfContextView::GetRangeFromPoint.
- Get the document bounds with ITfContextView::GetScreenExt.

Suppose there is an application that is not showing any text at
all but still accepting text input, e.g. for a voice input session
in a hands-free mode where showing composing text does not make
much sense.  In such a situation, TIPs cannot assume that the text
store is not accepting text simply because
ITfContext::GetActiveView() fails.  If TIP developers started
checking ITfContext::GetActiveView() every time before the TIP
requests document lock, it would cause various compatibility
issues.

In short, unavailability of TsViewCookie/ITfContextView means
only that there is no active UI for the text.  Unavailability
of text store is a different concept and should be notified
with a different mechanism, ITfThreadMgrEventSink::Pop(),
as I explained in comment #5.
https://hg.mozilla.org/mozilla-central/rev/5766724701d5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
> Suppose there is an application that is not showing any text at
> all but still accepting text input, e.g. for a voice input session
> in a hands-free mode where showing composing text does not make
> much sense.  In such a situation, TIPs cannot assume that the text
> store is not accepting text simply because
> ITfContext::GetActiveView() fails.

Good point.

> If TIP developers started
> checking ITfContext::GetActiveView() every time before the TIP
> requests document lock, it would cause various compatibility
> issues.

Sure. I'm afraid that TSF or TIP would stop working until restart the process due to unexpected result (we've met such situation a lot...).

FYI: TSFTextStore's log shows a lot of access to GetActiveView() with any IME. I guess that the caller is TSF.

> In short, unavailability of TsViewCookie/ITfContextView means
> only that there is no active UI for the text.  Unavailability
> of text store is a different concept and should be notified
> with a different mechanism, ITfThreadMgrEventSink::Pop(),
> as I explained in comment #5.

Understood. However, I don't think current *order* of doing jobs at destroying TextStore is wrong.

1. If there is ITextStoreACPSink, calls ITextStoreACPSink::OnLayoutChange(TS_LC_DESTROY, TEXTSTORE_DEFAULT_VIEW).
2. Releasing ITfContext.
3. Calling ITfDocumentMgr::Pop(TF_POPF_ALL).
4. Releasing ITfDocumentMgr.
5. Releasing ITextStoreACPSink.

I guess that if we do #3, #4 and #5 before #1, some TIP could access deleted object. Although, I forgot the bug#, we met such bug.

It might be that we don't need to do #1. But it's still not clear to me.
Comment on attachment 8667068 [details] [diff] [review]
TSFTextStore shouldn't grant document lock after starting to destroy the context

Approval Request Comment
[Feature/regressing bug #]: New regression of enabling TSF mode in 41.
[User impact if declined]: Some Google Japanese Input users complains about this crash bug. I see that on twitter everyday. Breakpad fails to catch this crash, therefore, we cannot know the how many users meet this crash.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Not sure but not so high. This is reported by only Google Japanese Users. So, other IMEs must not do nothing at receiving our text view destroying notification. This patch refuses IME to access the content after blur.
[String/UUID change made/needed]: No.
Attachment #8667068 - Flags: approval-mozilla-beta?
Attachment #8667068 - Flags: approval-mozilla-aurora?
Comment on attachment 8667068 [details] [diff] [review]
TSFTextStore shouldn't grant document lock after starting to destroy the context

fix a crash, taking it. should be in 42 beta 3
Attachment #8667068 - Flags: approval-mozilla-beta?
Attachment #8667068 - Flags: approval-mozilla-beta+
Attachment #8667068 - Flags: approval-mozilla-aurora?
Attachment #8667068 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.