Closed Bug 1376424 Opened 7 years ago Closed 7 years ago

[e10s] ContentCacheInParent may set different composition's start offset to active TextComposition instance

Categories

(Core :: Widget, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
thunderbird_esr52 --- unaffected
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(2 files)

This is regression of bug 1368554.  When a remote process is too slow, it may have older composition than the user is actually typing.  However, TextComposition instance in the parent process is destroyed when eCompositionCommit(AsIs) is being sent to the remote process (i.e., while the remote process still has the composition).
Comment on attachment 8881655 [details]
Bug 1376424 - part0: Backout the patch for bug 1368554

https://reviewboard.mozilla.org/r/152824/#review157962
Attachment #8881655 - Flags: review?(m_kato) → review+
Comment on attachment 8881656 [details]
Bug 1376424 - part1: TabChild should notify TabParent of "request to commit composition" handled

https://reviewboard.mozilla.org/r/152826/#review157968
Attachment #8881656 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/fa41e128cb5c
part0: Backout the patch for bug 1368554 r=m_kato
https://hg.mozilla.org/integration/autoland/rev/111ba549dd0a
part1: TabChild should notify TabParent of "request to commit composition" handled r=m_kato
landed on m-c
https://hg.mozilla.org/mozilla-central/rev/fa41e128cb5c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8881655 [details]
Bug 1376424 - part0: Backout the patch for bug 1368554

Approval Request Comment
[Feature/Bug causing the regression]:
Backing out the patch of bug 1368554.

[User impact if declined]:
Not sure. User may see unreported bugs because the approach to fix bug 1368554 was really wrong. The fix made TabParent not handle pending composition events correctly.

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

[Has the fix been verified in Nightly?]:
56.

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This is just back out the patch for bug 1368554. So, the reported crash bug will be reproduced if we won't uplift the following patch.

[String changes made/needed]:
No.
Attachment #8881655 - Flags: approval-mozilla-beta?
Comment on attachment 8881656 [details]
Bug 1376424 - part1: TabChild should notify TabParent of "request to commit composition" handled

Approval Request Comment
[Feature/Bug causing the regression]:
Fixing bug 1368554 with different approach (probably this is the right way).

[User impact if declined]:
User may meet the crash reported in bug1368554 if we'll take only the previous patch.

If we won't take both patches for this bug, user may see odd behavior when remote process is too busy since the patch for bug 1368554 stopped counting pending composition in the remote process.

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

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
Not so risky.

[Why is the change risky/not risky?]:
This patch adds internal notification which is sent from a remote process having IME composition to the main process when the remote process finishes handling request to IME of committing or canceling composition. (No such notification caused not decreasing the pending composition count in the main process. And when it reaches 255 and starts new composition, the main process crashed.)

[String changes made/needed]:
No.
Attachment #8881656 - Flags: approval-mozilla-beta?
So, at least only "part 0" should be uplifted. And if it's acceptable, "part 1" should be uplifted together for fixing the original crash report.

Note that the crash depends on the user's behavior whether users see the crash. If a user meets the crash, they may see the crash every several hours or almost everyday. Otherwise, users won't meet this bug. So, the number of crash reports does NOT represent the users' actual inconvenience.
Comment on attachment 8881655 [details]
Bug 1376424 - part0: Backout the patch for bug 1368554

seems worth taking both of those...

should be in 55.0b8
Attachment #8881655 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8881656 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10)
> [Is this code covered by automated tests?]:
> No.
> 
> [Has the fix been verified in Nightly?]:
> Yes.
> 
> [Needs manual test from QE? If yes, steps to reproduce]:
> No.

Setting qe-verify- based on Masayuki Nakano's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.