Closed Bug 1420849 Opened 2 years ago Closed 2 years ago

Crash in mozilla::ContentCacheInParent::OnEventNeedingAckHandled #2

Categories

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

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: crash, inputmethod, regression)

Crash Data

Attachments

(2 files)

+++ 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
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 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
Attachment #8932282 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
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
https://hg.mozilla.org/mozilla-central/rev/b4995dead417
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please request Beta approval on this when you get a chance.
Flags: needinfo?(masayuki)
Hmm, due to bug 1402519, we need a patch for Beta. I'll attach it soon.
Flags: needinfo?(masayuki)
Attached patch Patch for BetaSplinter Review
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.
Attachment #8934112 - Flags: approval-mozilla-beta?
Hi :masayuki,
From the crash report, there are still crashes in nightly 59 after the patch was landed.
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(masayuki)
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.
Summary: Crash in mozilla::ContentCacheInParent::OnEventNeedingAckHandled → Crash in mozilla::ContentCacheInParent::OnEventNeedingAckHandled #2
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?
Flags: needinfo?(masayuki)
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.
Attachment #8934112 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: needinfo?(masayuki)
Thank you!  The patches should be in beta 12, next week's build.
You need to log in before you can comment on or make changes to this bug.