1052230 - IMEContentObserver shouldn't flush pending notifications again during flushing notifications

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I think the initial FlushMergeableNotifications could happen sync, but possible nested ones async.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d44580bc427

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d44580bc427