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.

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

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

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

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.

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

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

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.

> 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 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?

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.

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.

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

s/bool preference/bool member

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.

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.

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

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

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 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 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 on attachment 8661238 [details] [diff] [review] part.5 IMENotificationSender shouldn't send notification recursively Couldn't we use AutoRestore for mSendingNotification? Either way.

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

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

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

>>+ // 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.

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

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

Shouldn't this be uplifted to Beta 42?

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

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.

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