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

RESOLVED FIXED in Firefox 55

Status

()

Core
Widget
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod, regression})

Trunk
mozilla56
inputmethod, regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(thunderbird_esr52 unaffected, firefox-esr52 unaffected, firefox54 unaffected, firefox55 fixed, firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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).
(Assignee)

Updated

6 months ago
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
status-thunderbird_esr52: --- → unaffected
(Assignee)

Updated

6 months ago
Keywords: regression
https://treeherder.mozilla.org/#/jobs?repo=try&revision=850bb30a4e465d1618b34d3e64f229e0a8c67727
(Assignee)

Updated

6 months ago
Blocks: 1376417
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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+

Comment 6

6 months ago
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

Comment 7

6 months ago
backoutbugherder
landed on m-c
https://hg.mozilla.org/mozilla-central/rev/fa41e128cb5c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 8

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/111ba549dd0a
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+

Comment 13

5 months ago
backoutbugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/3d31624e0a5b
status-firefox55: affected → fixed

Comment 14

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/0119ca3a5757
(Assignee)

Updated

5 months ago
Depends on: 1379448
(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-
(Assignee)

Updated

5 months ago
Depends on: 1380652
You need to log in before you can comment on or make changes to this bug.