ASSERTION: Don't call Send/RecvNotifyIMETextChange without NOTIFY_TEXT_CHANGE: 'updatePreference.WantTextChange()', triggered in e10s content processeditor

RESOLVED WORKSFORME

Status

()

Core
DOM: Events
RESOLVED WORKSFORME
4 years ago
3 years ago

People

(Reporter: ally, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
x86_64
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 obsolete attachment)

(Reporter)

Description

4 years ago
probable regression from bug 960877.

When in content-editables, replacement or extending an existing work triggers a slew of these assertions.

stack trace: http://pastebin.mozilla.org/5858958
(Reporter)

Updated

4 years ago
Blocks: 960877
Oh, interesting. I'm not sure the reason why the assertion occurs. However, query content event dispatch from PuppetWidget::NotifyIMEOfTextChange() causes a loop of renotification of it. I guess that suppressing it fixes this bug.

And this must be a regression of bug 496360 if it regressed about a week ago.
Assignee: nobody → masayuki
Blocks: 496360
Status: NEW → ASSIGNED
The for the assertion I don't see anything in the parent which would set WantTextChange flag, 
but on child side we set it always, in case of desktop-e10s?
(In reply to Olli Pettay [:smaug] from comment #2)
> but on child side we set it always, in case of desktop-e10s?

Yes. Currently, e10s doesn't support sync communication from parent to child. However, at querying content from IME, it's necessary. I believe that it should be allowed only for query content event in the future. But for now, PuppetWidget caches the latest plain text content for avoiding across process boundary at querying content. For maintaining the cached content, puppet widget needs text and selection change notifications itself.
Hmm, I cannot see the assertion in twitter.com.

Could you give me STR?
Flags: needinfo?(ally)
Created attachment 8469854 [details] [diff] [review]
IMEContentObserver shouldn't flush pending notifications again during flushing notifications

This patch should fix this bug but I'd like to check with STR to see the assertion.
(Reporter)

Comment 6

4 years ago
You need this patch, which makes the suggestions/replace options work in e10s https://bug1030451.bugzilla.mozilla.org/attachment.cgi?id=8469709

It includes ipdl changes so no incremental builds :/

Apply that to a debug build, focus on a content-editable box, like a bugzilla comment field, misspell a word, right click on it, pick a word to replace it with, watch the assertions fly
Flags: needinfo?(ally)
Hmm, the patch cannot fix this bug.

The reason of this bug is, opening context menu causes disabling IME on the widget which owns the focused editor. Then, nsTextStore is destroyed. However, PuppetWidget still stores IME update preference when IME is enabled.
Attachment #8469854 - Attachment is obsolete: true
I see the cause but I'm not sure what is the best way to fix this.

When context menu is open, parent process's nsXULPopupManager calls  	IMEStateManager::OnInstalledMenuKeyboardListener() for disabling IME temporarily. However, all of them are handled in parent process. Therefore, PuppetWidget doesn't know when IME state is changed by opening or closing menu. So, somebody needs to notify PuppetWidget of IME state change and PuppetWidget should update its stored IME update preference then.
Assignee: masayuki → nobody
Status: ASSIGNED → NEW
tracking-e10s: --- → ?
tracking-e10s: ? → +

Updated

4 years ago
Blocks: 1041185
Masayuki, are there any user visible issues here?
Flags: needinfo?(masayuki)
I don't test this again because I forgot what's the problem here (the stacktrace has gone).

However, according to my comments above, bug 1179632 and/or bug 1203381 should have fixed this bug completely because I changed the design of IMEContentObserver and IME state management across process boundary. So, the original report is already outdated.

Marking this as WFM for now, if there were some remaining bugs, please file new bugs for each symptom.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(masayuki)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.