1203381 - IMEContentObserver should not post new notification after it posts some notifications but some of them are not performed yet

Status: RESOLVED FIXED

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

Description

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

Currently, IMEContentObserver posts notifications as runnable. At this time, it clears the posted data. So, if the posted notifications are performed asynchronously, new notifications may post before performing the previous notifications.

This is bad for performance. widget should receive only the last notification because following notifications will cause querying content again. Querying content may be very expensive.

So, IMEContentObserver should know if posted notifications are actually sent to widget.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08f84751e6c0

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4da8b5e3fd1

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b391647623d

Comment 4

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

Attachment: part.1 IMEContentObserver should manage if each notification was posted but not performed

# This is a preparation, doesn't add any new job actually.

IMEContentObserver should be able to know if it already created runnable event for notifying IME.

Comment 5

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

Attachment: part.2 IMEContentObserver should manage if each notification was sent to widget

# This patch is also another preparation to fix this bug.

For detecting nested call of FlushMergeableNotifications(), IMEContentObserver should be able to check if it's sending notification to IME.

Comment 6

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

Attachment: part.3 IMEContentObserver shouldn't clear mTextChangeData until immediately before sending a text change notification

# This patch is also preparation of the fix.

Currently, IMEContentObserver clears mTextChangeData when creating TextChangeEvent instance and the instance stores the change data. However, if new text change occurs before sending notification but after creating the event, the event will notify IME of old text change. This causes that IME tries to query content at receiving a text change notification, the result may be different from the expected content by IME.

So, until TextChangeEvent::Run() notifies IME of text change actually, IMEContentObserver should merge new text change data even after it created a TextChangeEvent instance. Then, expected content by IME and expected content by IMEContentObserver are always same.

Comment 7

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

Attachment: part.4 IMEContentObserver shouldn't create redundant text/selection/position change event

Let's fix this bug actually!

If IMEContentObserver has created runnable events but they have not actually run yet, IMEContentObserver shouldn't create redundant runnable events. Then, we can reduce to send unnecessary notifications. This also reduces redundant query content events from either IME or ContentCacheInChild.

Comment 8

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

Attachment: part.5 IMEContentObserver shouldn't notify IME of position change caused by ContentEventHandler during sending NOTIFY_IME_OF_POSITION_CHANGE due to just redundant

This is additional fix.

When IMEContentObserver uses ContentEventHandler which occurs during sending notification to IME, the result includes the latest layout information. So, even if ContentEventHandler causes flushing pending layout, IMEContentObserver doesn't need to notify position change anymore.

Comment 9

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

Comment on attachment 8660633 [details] [diff] [review]
part.1 IMEContentObserver should manage if each notification was posted but not performed

>+  // mIsPosting*Event indicates that whether the runnable event class's
>+  // Run() method is not still performed yet.
...indicates whether the runnable event class' Run() method hasn't been executed yet.



But I wonder how much all of the patches in this bug will actually help, given that we're
using ScriptRunners here, not NS_DispatchToCurrent/MainThread or such.
ScriptRunners are run rather soon.
So please measure a bit how often we actually save anything with this new setup (which is a bit complicated).

Comment 10

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

Comment on attachment 8660634 [details] [diff] [review]
part.2 IMEContentObserver should manage if each notification was sent to widget

>+  // mIsSent*Notification indicates that the notification is being sent to
>+  // widget from the runnable event's Run().
>+  bool mIsSentFocusSetNotification;
>+  bool mIsSentTextChangeNotification;
>+  bool mIsSentSelectionChangeNotification;
>+  bool mIsSentPositionChangeNotification;

This naming is odd. I need to read the other patches still to understand what these are about, but
perhaps mIsSendingFocusSetNotification; etc.

Comment 11

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

> So please measure a bit how often we actually save anything with this new setup (which is a bit complicated).

Depends on the webpage. If <input> works with autocomplete implemented by the website, Reflow() is called much many times during typing a character. In that case, part.5 works well (the others are necessary for it).

Comment 12

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

Comment on attachment 8660637 [details] [diff] [review]
part.3 IMEContentObserver shouldn't clear mTextChangeData until immediately before sending a text change notification

>+IMEContentObserver::PostTextChangeNotification()
> {
>   MOZ_LOG(sIMECOLog, LogLevel::Debug,
>-    ("IMECO: 0x%p IMEContentObserver::PostTextChangeNotification("
>-     "aTextChangeData=%s)",
>-     this, TextChangeDataToString(aTextChangeData).get()));
>-
>-  mTextChangeData += aTextChangeData;
>-  MOZ_ASSERT(mTextChangeData.IsValid(),
>-             "mTextChangeData must have text change data");
>-
>-  MOZ_LOG(sIMECOLog, LogLevel::Debug,
>     ("IMECO: 0x%p IMEContentObserver::PostTextChangeNotification(), "
>      "mTextChangeData=%s)",
>      this, TextChangeDataToString(mTextChangeData).get()));
>+
>+  MOZ_ASSERT(mTextChangeData.IsValid(),
>+             "mTextChangeData must have text change data");
>+  mIsTextChangeEventPending = true;
> }
IMEContentObserver::PostTextChangeNotification is now rather dummy method, but
I guess it is fine for that logging stuff.


>-  if (mTextChangeData.IsValid()) {
>+  if (mIsTextChangeEventPending) {
>     MOZ_LOG(sIMECOLog, LogLevel::Debug,
>       ("IMECO: 0x%p IMEContentObserver::FlushMergeableNotifications(), "
>        "creating TextChangeEvent...", this));
>+    mIsTextChangeEventPending = false;
>     mIsPostingTextChangeEvent = true;
>-    nsContentUtils::AddScriptRunner(new TextChangeEvent(this, mTextChangeData));
>+    nsContentUtils::AddScriptRunner(new TextChangeEvent(this));


Oh, the naming is like this...
mIsTextChangeEventPending sounds to me like something where we have TextChangeEvent waiting to be processed -
but mIsPostingTextChangeEvent is currently for that.
Could we call mIsTextChangeEventPending mNeedsTextChangeEvent

Comment 13

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

Oh, and I can say the another reason why this is necessary.

See bug 1202080. Current code may cause infinite recursive calls. Although, I cannot test it yet because I cannot reproduce the crash, the bug must be fixed by these patches as far as watching the log of IMEContentObserver.

Comment 14

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

> Could we call mIsTextChangeEventPending mNeedsTextChangeEvent

Sure. I'll add part.6 for that tomorrow. I'll go to bed soon. Thank you very much, I'm really happy you reviewed a lot of patches quickly!!

Comment 15

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

Comment on attachment 8660642 [details] [diff] [review]
part.4 IMEContentObserver shouldn't create redundant text/selection/position change event

I think I need the naming of the variables changed before reviewing this. So many different bool variables. 


Hmm, would it even work if we didn't have bool variables but some enum?
So that states would be
eInitialState (something better would be good for this)
eNeedsEvent
eIsPostingEvent

or something like that, and then use that for different events/notifications?

Comment 16

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

Attachment: part.1 IMEContentObserver shouldn't clear mTextChangeData until immediately before sending a text change notification

This is same as the old part.3 (attachment 8660637 [details] [diff] [review]). I think that we should merge FocusSetEvent, SelectionChangeEvent, TextChangeEvent and PositionChangeEvent. Then, we can reduce the flags and make the code simpler. Additionally, we can make notification order same always.

Comment 17

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

Attachment: part.2 Merge all IME notification sending events of IMEContentObserver to a runnable class

Let's merge the 3 runnable classes. Then, we can reduce the number of time to create runnable events and send all pending notifications synchronously when there is no script blocker.

Comment 18

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

Attachment: part.3 IMEContentObserver::mIsFlushingPendingNotifications should be cleared when all pending notifications are sent to IME

Then, we can manage if a runnable event is performed with only one bool preference which is already existing.

Comment 19

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

s/bool preference/bool member

Comment 20

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

Attachment: part.4 IMENotificationSender should keep the order of notifications even when a notification causes another change

Although, this is different bug, we should keep the notification order (text change -> selection change -> position change) even if one of them causes new pending notification. E.g., without this patch, IME may receive the latest selection while IME caches older text.

Comment 21

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

Attachment: part.5 IMENotificationSender shouldn't send notification recursively

Let's fix the possibility of infinite recursive calls with |IMEMessage mSendingNotification| which stores the sending notification. If it's not NOTIFY_IME_OF_NOTHING, AChangeEvent should think that it's not safe to notify IME of anything.

Comment 22

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

Attachment: part.6 IMEContentObserver shouldn't post position change event if Reflow() is called during handling a query content event and sending NOTIFY_IME_OF_POSITION_CHANGE since the result of query content e

Let's prevent redundant position change notification if it's caused by flushing layout at querying the content and sending position change notification.

Comment 23

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

Attachment: part.7 Rename IMEContentObserver::mIs*ChangeEventPending to IMEContentObserver::mNeedsToNotifyIMEOf*Change

Finally, rename the existing members whose names you don't like.

Comment 24

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

Comment on attachment 8661232 [details] [diff] [review]
part.2 Merge all IME notification sending events of IMEContentObserver to a runnable class

>+IMEContentObserver::IMENotificationSender::Run()
>+{
>+  // NOTE: Reset each pending flag because sending notification may cause
>+  //       another change.
> 
>-NS_IMETHODIMP
>-IMEContentObserver::FocusSetEvent::Run()
>-{
>-  if (!CanNotifyIME()) {
>-    // If IMEContentObserver has already gone, we don't need to notify IME of
>-    // focus.
>-    MOZ_LOG(sIMECOLog, LogLevel::Debug,
>-      ("IMECO: 0x%p IMEContentObserver::FocusSetEvent::Run(), FAILED, due to "
>-       "impossible to notify IME of focus", this));
>+  if (mIMEContentObserver->mIsFocusEventPending) {
>+    mIMEContentObserver->mIsFocusEventPending = false;
>+    SendFocusSet();
>+    // This is the first notification to IME. So, we don't need to notify
>+    // anymore since IME starts to query content after it gets focus.
>     mIMEContentObserver->ClearPendingNotifications();
>     return NS_OK;
>   }
Actually, I don't understand this. Why do we return NS_OK here?
What if there are other pending events, what guarantees those are run?
And then in the next patch there is explicit
mIMEContentObserver->mIsFlushingPendingNotifications = false;
here which makes the code a bit harder to read.

Please explain or fix, and re-ask review.

Comment 25

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

Comment on attachment 8661234 [details] [diff] [review]
part.3 IMEContentObserver::mIsFlushingPendingNotifications should be cleared when all pending notifications are sent to IME

>   if (mIMEContentObserver->mIsFocusEventPending) {
>     mIMEContentObserver->mIsFocusEventPending = false;
>     SendFocusSet();
>     // This is the first notification to IME. So, we don't need to notify
>     // anymore since IME starts to query content after it gets focus.
>     mIMEContentObserver->ClearPendingNotifications();
>+    mIMEContentObserver->mIsFlushingPendingNotifications = false;
>     return NS_OK;
>   }
r+ assuming this change is removed if there are changes to the previous patch (or not removed if it is explained why we need to return here.)


>+  // If notifications caused some new change, we should notify them now.
>+  mIMEContentObserver->mIsFlushingPendingNotifications =
>+    mIMEContentObserver->mIsTextChangeEventPending ||
>+    mIMEContentObserver->mIsSelectionChangeEventPending ||
>+    mIMEContentObserver->mIsPositionChangeEventPending;
In theory, and for consistency, shouldn't we add also || mIMEContentObserver->mIsFocusEventPending
here.

Comment 26

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

Comment on attachment 8661232 [details] [diff] [review]
part.2 Merge all IME notification sending events of IMEContentObserver to a runnable class

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8661232 [details] [diff] [review]
> part.2 Merge all IME notification sending events of IMEContentObserver to a
> runnable class
> 
> >+IMEContentObserver::IMENotificationSender::Run()
> >+{
> >+  // NOTE: Reset each pending flag because sending notification may cause
> >+  //       another change.
> > 
> >-NS_IMETHODIMP
> >-IMEContentObserver::FocusSetEvent::Run()
> >-{
> >-  if (!CanNotifyIME()) {
> >-    // If IMEContentObserver has already gone, we don't need to notify IME of
> >-    // focus.
> >-    MOZ_LOG(sIMECOLog, LogLevel::Debug,
> >-      ("IMECO: 0x%p IMEContentObserver::FocusSetEvent::Run(), FAILED, due to "
> >-       "impossible to notify IME of focus", this));
> >+  if (mIMEContentObserver->mIsFocusEventPending) {
> >+    mIMEContentObserver->mIsFocusEventPending = false;
> >+    SendFocusSet();
> >+    // This is the first notification to IME. So, we don't need to notify
> >+    // anymore since IME starts to query content after it gets focus.
> >     mIMEContentObserver->ClearPendingNotifications();
> >     return NS_OK;
> >   }
> Actually, I don't understand this. Why do we return NS_OK here?
> What if there are other pending events, what guarantees those are run?
> And then in the next patch there is explicit
> mIMEContentObserver->mIsFlushingPendingNotifications = false;
> here which makes the code a bit harder to read.
> 
> Please explain or fix, and re-ask review.

Because when IME receives focus set notification, IME doesn't know the content information. Therefore, it doesn't make sense to notify IME of previous change notifications before sending focus notification since IME doesn't need to query content with the notification (the query result should be same as the result at querying the content when IME gets focus). I explained it in the comment in the patch.

>+    // This is the first notification to IME. So, we don't need to notify
>+    // anymore since IME starts to query content after it gets focus.

Comment 27

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

Comment on attachment 8661238 [details] [diff] [review]
part.5 IMENotificationSender shouldn't send notification recursively

Couldn't we use AutoRestore for mSendingNotification?
Either way.

Comment 28

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> Because when IME receives focus set notification, IME doesn't know the
> content information. Therefore, it doesn't make sense to notify IME of
> previous change notifications before sending focus notification since IME
> doesn't need to query content with the notification (the query result should
> be same as the result at querying the content when IME gets focus). I
> explained it in the comment in the patch.
> 
> >+    // This is the first notification to IME. So, we don't need to notify
> >+    // anymore since IME starts to query content after it gets focus.
But what guarantees we actually notify other pending events?
I would assume we don't in general actually have other pending events if mIsFocusEventPending is true.

Comment 29

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

(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8661238 [details] [diff] [review]
> part.5 IMENotificationSender shouldn't send notification recursively
> 
> Couldn't we use AutoRestore for mSendingNotification?
> Either way.

At least for now, it makes same result. But I'm afraid that future change would break it by adding some code flushing layout or something.

Comment 30

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

Comment on attachment 8661239 [details] [diff] [review]
part.6 IMEContentObserver shouldn't post position change event if Reflow() is called during handling a query content event and sending NOTIFY_IME_OF_POSITION_CHANGE since the result of query content e

Ok, so this is purely an optimization.

Comment 31

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

(In reply to Olli Pettay [:smaug] from comment #28)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> > Because when IME receives focus set notification, IME doesn't know the
> > content information. Therefore, it doesn't make sense to notify IME of
> > previous change notifications before sending focus notification since IME
> > doesn't need to query content with the notification (the query result should
> > be same as the result at querying the content when IME gets focus). I
> > explained it in the comment in the patch.
> > 
> > >+    // This is the first notification to IME. So, we don't need to notify
> > >+    // anymore since IME starts to query content after it gets focus.
> But what guarantees we actually notify other pending events?

So, we don't need to guarantee that because IME doesn't need them. If they would be notified immediately after focus set notification, IME may query content again. But the result may odd. For example, if IME queries text at getting focus, then the result may be "ABC". However, if the content is generated with "ABXC" and removing X at setting focus, text change may be recorded as removing the third character. If text change notification is sent to IME after focus set notification, IME must assume that the content becomes "AB" (the new third character "C" is removed), but the actual result is "ABC". If IME doesn't query content, IME caches wrong content, that may cause odd behaviors.

> I would assume we don't in general actually have other pending events if
> mIsFocusEventPending is true.

Yes. That's the edge case. Basically, focus set should be sent to IME synchronously at an editable element gets focus. However, if it's not safe due to script blocker or reflowing, it may be put off. At this case, this scenario might occur.

Comment 32

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

Comment on attachment 8661232 [details] [diff] [review]
part.2 Merge all IME notification sending events of IMEContentObserver to a runnable class

Ok,  fine. I assume IME does query new information when needed.

Comment 33

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

>>+  // If notifications caused some new change, we should notify them now.
>>+  mIMEContentObserver->mIsFlushingPendingNotifications =
>>+    mIMEContentObserver->mIsTextChangeEventPending ||
>>+    mIMEContentObserver->mIsSelectionChangeEventPending ||
>>+    mIMEContentObserver->mIsPositionChangeEventPending;
> In theory, and for consistency, shouldn't we add also || mIMEContentObserver->mIsFocusEventPending
> here.

Hmm, it's not necessary because focus set notification is sent to only once per instance of IMEContentObserver. So, if it's true, it's never come here since early return code in the case.

However, I think that adding IMEContentObserver::NeedsToNotifyIMEOfSomething() which checks all flags is useful since it can be shared and when we add a new notification, it's easier to do it.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcfee08450fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b69a6cadbe1
https://hg.mozilla.org/integration/mozilla-inbound/rev/11944d579d68
https://hg.mozilla.org/integration/mozilla-inbound/rev/b746528338fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a37db144dcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/46046fcbf40f
https://hg.mozilla.org/integration/mozilla-inbound/rev/34606afcc726

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bcfee08450fc
https://hg.mozilla.org/mozilla-central/rev/6b69a6cadbe1
https://hg.mozilla.org/mozilla-central/rev/11944d579d68
https://hg.mozilla.org/mozilla-central/rev/b746528338fa
https://hg.mozilla.org/mozilla-central/rev/3a37db144dcd
https://hg.mozilla.org/mozilla-central/rev/46046fcbf40f
https://hg.mozilla.org/mozilla-central/rev/34606afcc726

Comment 36

[Vladan Djeric (:vladan)]

Shouldn't this be uplifted to Beta 42?

Comment 37

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

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #36)
> Shouldn't this be uplifted to Beta 42?

Hmm, this is pretty risky. And I don't have much reason to uplift to Beta since e10s mode isn't available on Beta. Why do you think so?

Comment 38

[Vladan Djeric (:vladan)]

There's a chance we'll be doing A/B experiments on Beta 42 where we force a small # of users into using e10s and then compare their Telemetry vs Telemetry from non-e10s users.

It's not a common crash though, so you can keep the fix on Aurora if you think it's risky.

Comment 39

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

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #38)
> There's a chance we'll be doing A/B experiments on Beta 42 where we force a
> small # of users into using e10s and then compare their Telemetry vs
> Telemetry from non-e10s users.
> 
> It's not a common crash though, so you can keep the fix on Aurora if you
> think it's risky.

The crash bug depending on this bug doesn't have security risk and not a topcrash. So, I think that we should take safer way for this bug.