Closed Bug 1052230 Opened 6 years ago Closed 6 years ago

IMEContentObserver shouldn't flush pending notifications again during flushing notifications

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(1 file, 1 obsolete file)

Currently, IMEContentObserver dispatches runnable events which notify IME of something *before* clearing "pending to dispatch" flag.

Therefore, if the method is called recursively, same runnable event (i.e., it has been being dispatched) may be dispatched again.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8471355 - Flags: review?(bugs)
Comment on attachment 8471355 [details] [diff] [review]
Patch

>+  // If we're in handling an edit action, this method will be called later.
>+  // If this is already detached from the widget, this don't need to notify
s/don't/doesn't/

>+  // Notifying something may cause nested call of this method.  For example,
>+  // when somebody notified one of the notifications may dispatch query content
>+  // event. Then, it causes flushing layout which may cause another layout
>+  // change notification.
>+
>+  if (mIsFlushingPendingNotifications) {
>+    // So, if this is already called, this should do nothing.
>+    return;
>+  }
>+
>+  mIsFlushingPendingNotifications = true;
Perhaps use mozilla::AutoRestore here.

>+  // If notifications may cause new change, we should notify them now.
>+  if (mTextChangeData.mStored ||
>+      mIsSelectionChangeEventPending ||
>+      mIsPositionChangeEventPending) {
>+    FlushMergeableNotifications();
>   }
Hmm, isn't it possible that we end up to endless recursion here
Explain why recursion couldn't be an issue and ask re-review, or fix the possible issue.
(I could imagine there might be a case where we just need to call FlushMergeableNotifications asynchronously to break the
recursion.)
Attachment #8471355 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #2)
> >+  // If notifications may cause new change, we should notify them now.
> >+  if (mTextChangeData.mStored ||
> >+      mIsSelectionChangeEventPending ||
> >+      mIsPositionChangeEventPending) {
> >+    FlushMergeableNotifications();
> >   }
> Hmm, isn't it possible that we end up to endless recursion here
> Explain why recursion couldn't be an issue and ask re-review, or fix the
> possible issue.
> (I could imagine there might be a case where we just need to call
> FlushMergeableNotifications asynchronously to break the
> recursion.)

Ah, could be. But I think that TextChange and SelectionChange should be notified synchronously as far as possible... On the other hand, I don't have idea to make infinite recursion. I'll create a patch which dispatches the events asynchronously.
I think the initial FlushMergeableNotifications could happen sync, but possible nested ones async.
Attached patch PatchSplinter Review
Attachment #8471355 - Attachment is obsolete: true
Attachment #8472151 - Flags: review?(bugs)
Attachment #8472151 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/3d44580bc427
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.