1420849 - Crash in mozilla::ContentCacheInParent::OnEventNeedingAckHandled #2

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

+++ This bug was initially created as a clone of Bug #1405832 +++

The patches for bug 1405832 are not enough to fix the crash.

Here is the latest crash report after landing them:
https://crash-stats.mozilla.com/report/index/706f8f0f-0b3e-450d-860b-614c60171125

> There is no pending composition but received eCompositionCommitRequestHandled message from the remote child
> 
> Dispatched Event Message Log:
>   eCompositionStart
>   eCompositionChange
>   eCompositionChange
>   eCompositionChange
>   eCompositionChange
>   eCompositionCommit
> 
> Received Event Message Log:
>   eCompositionStart
>   eCompositionChange
>   eCompositionChange
>   eCompositionChange
>   eCompositionCommitRequestHandled
>   eCompositionChange
>   eCompositionCommitRequestHandled
> 
> Result of RequestIMEToCommitComposition():
>   Commit reqeust is handled synchronously
>   Commit request is handled with stored composition string because TabParent has already lost focus

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, perhaps, TextComposition in content process is destroyed by synthesized eCompositionCommitAsIs which is dispatched from here:
https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/widget/PuppetWidget.cpp#635,661,673,677

So, the old TextComposition does NOT manage following requests to commit/cancel composition.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62bde317d4bf2299887a0916b89e0758e08ba7bf

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1420849 - Make PuppetWidget discard composition events after requesting commit composition and synthesizing eCommitComposition event until new eCompositionStart event comes

When ContentCacheInParent::RequestIMEToCommitComposition() returns true,
the remote process will synthesize eCompositionCommit event.  This causes
destroying TextComposition event in the remote process (by
IMEStateManager::DispatchCompositionEvent()).  Then,
IMEStateManager::DispatchCompositionEvent() will recreate new TextComposition
instance when it receives new composition event.  Then, it may request
to commit composition to the main process via PuppetWidget accidentally.

So, after PuppetWidget::RequestIMEToCommitComposition() synthesizes
eCompositionCommit, PuppetWidget should discard following composition events
until it receives eCompositionStart since they are unnecessary for the content
and they are for the old composition, i.e., outdated events.

Review commit: https://reviewboard.mozilla.org/r/203304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203304/

Comment 4

[Makoto Kato [:m_kato]]

Comment on attachment 8932282 [details]
Bug 1420849 - Make PuppetWidget discard composition events after requesting commit composition and synthesizing eCommitComposition event until new eCompositionStart event comes

https://reviewboard.mozilla.org/r/203304/#review209258

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4995dead417
Make PuppetWidget discard composition events after requesting commit composition and synthesizing eCommitComposition event until new eCompositionStart event comes r=m_kato

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b4995dead417

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, due to bug 1402519, we need a patch for Beta. I'll attach it soon.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for Beta

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1377672.

[User impact if declined]:
A lot of IME users keep meeting this crash.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes, after landing the patch, no crash report is recorded.

[Needs manual test from QE? If yes, steps to reproduce]:
No, since we don't have STR.

[List of other uplifts needed for the feature/fix]:
The patches for 1405832 are required because this patch is a follow up patch for them.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This makes PuppetWidget in remote process ignore outdated composition events. Even if this patch had a bug, after canceling/committing current composition, they would use IME normally.

[String changes made/needed]:
No.

Comment 10

[Gerry Chang [:gchang]]

Hi :masayuki,
From the crash report, there are still crashes in nightly 59 after the patch was landed.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, really odd. According to the log in crash report, there still be a simple mistake. I really want STR with testcase, though, but I'll take a look after dinner. But looks like that current crash reports won't be reproducible in release channel and beta channel except developer edition if we land the patches. So, I recommend to uplift them due to the crash rate is too high.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The reason why some people meet this crash is, mPendingCompositionCount is decremented when both commit request handles synchronously when TabParent has already lost focus and commit event is handled (ignored, actually) by the remote process. So, accidentally, mPendingCompositionCount is decremented twice for a composition in this case.

Perhaps, ContentCacheInParent::OnEventNeedingAckHandled() shouldn't decrement it when the main process receives notification  that eCompositionCommit event is handled (ignored) in the remote process because of already committed by preceding commit request.

In other words, this is only the problem of count management of mPendingCompositionCount and this doesn't cause crash in release channel nor beta channel unless developers edition if those patches are uplifted. I'll try to fix this tomorrow, but the patch might not be so small and might be risky to uplift.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Masayuki, any luck? Do you think we should try uplifting the patches here and in bug 1405832?  Would it make the crash rate worse in dev edition, or do think it would stay about the same?

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8934112 [details] [diff] [review]
Patch for Beta

Uplift crash fix for beta 11 or 12,  along with patches from bug 1405832 and bug 1423456.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #13)
> Masayuki, any luck? Do you think we should try uplifting the patches here
> and in bug 1405832?

Of course, yes. The crash rate is too high.

> Would it make the crash rate worse in dev edition, or
> do think it would stay about the same?

Must be fixed by all patches across 3 bugs even on dev edition and Nightly. The latest crash report is 20171206221407 from Nightly. So, I think that we probably succeeded to fix this crash.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1dc54b85de4d

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Thank you!  The patches should be in beta 12, next week's build.