Crash in mozilla::ContentCacheInParent::OnEventNeedingAckHandled #3

RESOLVED FIXED in Firefox 58

Status

()

defect
P2
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({crash, inputmethod, regression})

56 Branch
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 wontfix, firefox58 fixed, firefox59 fixed)

Details

(crash signature)

Attachments

(2 attachments)

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

+++ 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

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.
Comment on attachment 8934870 [details]
Bug 1423456 - ContentCacheInParent::OnEventNeedingAckHandled() shouldn't decrement mPendingCompositionCount when it receives eCompositionCommit(AsIs) from the remote process but the composition has already been committed by a request to commit composition

https://reviewboard.mozilla.org/r/205746/#review211764
Attachment #8934870 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d9b9a40f8d94
ContentCacheInParent::OnEventNeedingAckHandled() shouldn't decrement mPendingCompositionCount when it receives eCompositionCommit(AsIs) from the remote process but the composition has already been committed by a request to commit composition r=m_kato
https://hg.mozilla.org/mozilla-central/rev/d9b9a40f8d94
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Do we need to uplift this along with #1 and #2?
Flags: needinfo?(masayuki)
(In reply to Julien Cristau [:jcristau] from comment #6)
> Do we need to uplift this along with #1 and #2?

Although, even without this patch, release channel won't crash if we uplift the patches for both #1 and #2. However, now, we have a patch for this. So, yes, if we can uplift the patch for this with them, that must be the best.
Flags: needinfo?(masayuki)
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 bug 1405832 and bug 1420849 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 fixes a bug of counting composition which is not yet committed in the composition. Even if this patch has some bugs, the counter will be adjusted not to subtracted from 0 in release channel and beta channel except developer edition.

[String changes made/needed]:
No.
Comment on attachment 8936205 [details] [diff] [review]
Patch for beta

Uplift to fix a crash, along with patches in bug 1405832 and bug 1420849.

We should follow up to check if there are any "leftover" crashes once these land on beta (either in 58.0b11 or 58.0b12)
Attachment #8936205 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.