1176954 - [e10s][TSF][GTK?] TabParent shouldn't notify IME until all sending events are handled

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

If content process is busy, TabParent may send two or more events before the first sending event is handled.

Then, if TabParent notifies IME of text change or something at the first event handled, native IME expected that all sending events are handled. So, if IME tries to query content, TabParent still has old content which only handles the first event.

For preventing this issue, TabParent should store sending events and when it's handled by editor, it should be notified that and remove the handled event from the queue. Then, all events are handled, start notifying IME.

Comment 1

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

Attachment: part.1 Child process should notify its parent process when it dispatches composition or selection event into the DOM tree

For managing pending event count in TabParent, TabChild should notify TabParent of receiving an event. (Although, it could be synchronous.)

Comment 2

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

Attachment: part.2 ContentCache in parent process should manage how many events are sent but not yet received

ContentCache in TabParent should count the number of pending event which are sent but not received yet.

Comment 3

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

Attachment: part.3 Don't send selection change, text change nor composition update notification to IME from TabParent until all events sent to the child process is received by it

When there is no pending event, ContentCache should notify IME of pending notifications. I think that only selection change, text change and composition update is enough. Even if it's not enough, we can append other notifications such as position change easy.

Note that this patch doesn't merge multiple text change ranges before sending a text change notification. It will be fixed by following patches.

Comment 4

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

Attachment: part.4 Define IMENotification::TextChangeDataBase and whose member names should be same as IMEContentObserver::TextChangeData's

Currently, we have the code of merging text change ranges in IMEContentObserver. But we should move it into IMENotification.

This patch names the struct of mTextChangeData as TextChangeDataBase. We need constructor for that later, but structs which have constructor cannot be a member of union. Therefore, its name has "Base" postfix.

And its members should be same name as IMEContentObserver::TextChangeData which were named by you ;-)

The comment of each member are just copied from IMEContentObserver::TextChangeData definition.

Comment 5

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

Attachment: part.5 IMENotification::TextChangeDataBase should have a state which indicates that it's not initialized

Instead of |bool mStored|, we can use mStartOffset, mRemovedEndOffset and mAddedEndOffset for indicating if the members are initialized. mStartOffset must be same or less than the others. So, setting mStartOffset UINT32_MAX and m*EndOffset 0 can indicate "uninitialized".

This state is necessary when merging with another instance.

Comment 6

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

Attachment: part.6 IMEContentObserver should use IMENotification::TextChangeData

Now, we can drop IMEContentObserver::TextChangeData and make it use IMENotification::TextChangeData(Base).

Comment 7

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

Attachment: part.7 IMENotification::TextChangeData should have a method to merge with another instance and IMENotification::MergeWith() should use it

Finally, we can move merge method and test method of that into IMENotification::TextChangeDataBase. Then, we can fix the pending issue of part.3.

Comment 8

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

Comment on attachment 8631569 [details] [diff] [review]
part.1 Child process should notify its parent process when it dispatches composition or selection event into the DOM tree

(it is not clear to me why any of IME stuff needs to use prio(urgent), but that is a separate bug)


OnEventReceived() is way too generic name. What event? DOM Event, something else?

I think the cleanest API would be to explicitly have
OnSeclectionEventReceived()/OnCompositionEventReceived() in PBrowser. Sure, it is a bit redundant, but makes code easier to understand.
Or otherwise hint in the name that it is just about some particular events.

Comment 9

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

Comment on attachment 8631570 [details] [diff] [review]
part.2 ContentCache in parent process should manage how many events are sent but not yet received

So I would add the OnFooEventReceived also to ContentCacheInParent, and then
both implementations would just --mPendingEvents;

And perhaps mPendingEvents could be called mPendingCompositionAndSelectionEvents;
A bit long, but clear.

With those, r+

Comment 10

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

Comment on attachment 8631572 [details] [diff] [review]
part.3 Don't send selection change, text change nor composition update notification to IME from TabParent until all events sent to the child process is received by it

> TabParent::RecvOnEventReceived()
> {
>-  mContentCache.OnEventReceived();
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget) {
>+    return true;
>+  }
>+
>+  // While calling OnEventReceived(), TabParent *might* be destroyed since
>+  // it may send notifications to IME.
>+  nsRefPtr<TabParent> kungFuDeathGrip(this);
>+  mContentCache.OnEventReceived(widget);
>   return true;
> }
So this could be in those RecvOnFooEventReceived() methods, or in some helper



>+ContentCacheInParent::OnEventReceived(nsIWidget* aWidget)
> {
>   MOZ_LOG(sContentCacheLog, LogLevel::Info,
>-    ("ContentCacheInParent: 0x%p OnEventReceived(), mPendingEvents=%u",
>-     this, mPendingEvents));
>+    ("ContentCacheInParent: 0x%p OnEventReceived(aWidget=0x%p), "
>+     "mPendingEvents=%u",
>+     this, aWidget, mPendingEvents));
> 
>   MOZ_RELEASE_ASSERT(mPendingEvents > 0);
>-  mPendingEvents--;
>+  if (--mPendingEvents) {
>+    return;
>+  }
>+
>+  FlushPendingNotifications(aWidget);
> }
And this too... 
Hmm, I'm not quite sure what would be cleanest setup for this all, and least error prone, so not code duplication.
OnEventReceived is too generic. Perhaps add some enum
EventTypeNeedingAck and then pass that to SendOnEventReceived and so on.
It wouldn't be useful for anything else bug debugging, and making code more readable.
Or do you have better ideas?

>+ContentCacheInParent::FlushPendingNotifications(nsIWidget* aWidget)
>+{
>+  MOZ_ASSERT(!mPendingEvents);
>+
>+  // New notifications which are notified during flushing pending notifications
>+  // should be merged again.
>+  mPendingEvents++;
>+
>+  nsCOMPtr<nsIWidget> kungFuDeathGrip(aWidget);
>+
>+  if (mPendingTextChange.HasNotification()) {
>+    IMENotification notification(mPendingTextChange);
>+    if (!aWidget->Destroyed()) {
>+      mPendingTextChange.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  if (mPendingSelectionChange.HasNotification()) {
>+    IMENotification notification(mPendingSelectionChange);
>+    if (!aWidget->Destroyed()) {
>+      mPendingSelectionChange.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  if (mPendingCompositionUpdate.HasNotification()) {
>+    IMENotification notification(mPendingCompositionUpdate);
>+    if (!aWidget->Destroyed()) {
>+      mPendingCompositionUpdate.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }

Why do we do these in this order? Shouldn't we keep the right order?

Comment 11

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

Comment on attachment 8631576 [details] [diff] [review]
part.5 IMENotification::TextChangeDataBase should have a state which indicates that it's not initialized

IsStored sounds a bit odd to my non-native-English speaker's ears ;)
Perhaps IsValid()

Comment 12

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

Comment on attachment 8631578 [details] [diff] [review]
part.6 IMEContentObserver should use IMENotification::TextChangeData

Hmm,  mCausedOnlyByComposition changes to mCausedByComposition.
I guess that is ok.

s/IsStored()/IsValid()/ here too.
I don't feel too strongly about that, but IMO IsValid() is more clear.


Why do we need struct TextChangeData? It doesn't seem to add anything useful to
TextChangeDataBase.
Maybe the next patch explains. tentative r+, assuming TextChangeData is actually useful.

Comment 13

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

> EventTypeNeedingAck and then pass that to SendOnEventReceived and so on.
> It wouldn't be useful for anything else bug debugging, and making code more readable.
> Or do you have better ideas?

How about to send WidgetGUIEvent or just its message?

Comment 14

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

Comment on attachment 8631578 [details] [diff] [review]
part.6 IMEContentObserver should use IMENotification::TextChangeData

no, I don't understand struct TextChangeData. The next patch adds something to
*Base, but not to TextChangeData. Why we need TextChangeData?
Why not just TextChangeDataBase everywhere (and rename it to TextChangeData)

Comment 15

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

>>+ContentCacheInParent::FlushPendingNotifications(nsIWidget* aWidget)
>>+{
>>+  MOZ_ASSERT(!mPendingEvents);
>>+
>>+  // New notifications which are notified during flushing pending notifications
>>+  // should be merged again.
>>+  mPendingEvents++;
>>+
>>+  nsCOMPtr<nsIWidget> kungFuDeathGrip(aWidget);
>>+
>>+  if (mPendingTextChange.HasNotification()) {
>>+    IMENotification notification(mPendingTextChange);
>>+    if (!aWidget->Destroyed()) {
>>+      mPendingTextChange.Clear();
>>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>>+    }
>>+  }
>>+
>>+  if (mPendingSelectionChange.HasNotification()) {
>>+    IMENotification notification(mPendingSelectionChange);
>>+    if (!aWidget->Destroyed()) {
>>+      mPendingSelectionChange.Clear();
>>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>>+    }
>>+  }
>>+
>>+  if (mPendingCompositionUpdate.HasNotification()) {
>>+    IMENotification notification(mPendingCompositionUpdate);
>>+    if (!aWidget->Destroyed()) {
>>+      mPendingCompositionUpdate.Clear();
>>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>>+    }
>>+  }
> 
> Why do we do these in this order? Shouldn't we keep the right order?

Yes. New selection range which is notified by selection change notification is in the latest text content. Therefore, text change notification must be notified before that.

Composition update notification indicates that the dispatched events are completely handled including sending notifications. So, this must be the last. Therefore, only this order is right.

Comment 16

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

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8631578 [details] [diff] [review]
> part.6 IMEContentObserver should use IMENotification::TextChangeData
> 
> no, I don't understand struct TextChangeData. The next patch adds something
> to
> *Base, but not to TextChangeData. Why we need TextChangeData?
> Why not just TextChangeDataBase everywhere (and rename it to TextChangeData)

Because constructor guarantees to clear the data if it's used as a member of a class. And also it's used by creating temporary instance with specific ranges in TextChangeDataBase::Test().

Comment 17

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

Why can't TextChangeDataBase ctor guarantee to clear the data? And same with specific ranges.
Why can't all the functionality, which isn't much, moved to the *Base?

Comment 18

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

(In reply to Olli Pettay [:smaug] from comment #17)
> Why can't TextChangeDataBase ctor guarantee to clear the data? And same with
> specific ranges.
> Why can't all the functionality, which isn't much, moved to the *Base?

Because it's used in union of IMENotification. Any structs/classes cannot have constructor in union.

Comment 19

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

Comment on attachment 8631578 [details] [diff] [review]
part.6 IMEContentObserver should use IMENotification::TextChangeData

I see. Ok, add a comment why we need TextChangeData..

Comment 20

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> > EventTypeNeedingAck and then pass that to SendOnEventReceived and so on.
> > It wouldn't be useful for anything else bug debugging, and making code more readable.
> > Or do you have better ideas?
> 
> How about to send WidgetGUIEvent or just its message?
Well, if it is used just for these two types of events, it feels odd if some random message would be sent. Whoever reads the code has hard time to understand which events need OnEventReceived.
That is the concern I have - how to make the code easier to read.
If we're pretty sure it will be needed only for Composition or Selection events, 
perhaps OnCompositionOrSelectionEventReceived() or some such?

Comment 21

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

(In reply to Olli Pettay [:smaug] from comment #20)
> perhaps OnCompositionOrSelectionEventReceived() or some such?

Yeah, my first patch used such name, but I worry about that in the future, if we need to send the notification for the other events too, the name becomes odd (or needs to change longer).

So, I think that if the method takes WidgetGUIEvent or its message, we can do check the message in RecvOnEventReceived(). Then, I believe that it's enough easy to read.

E.g.,

TabParent::RecvOnEventReceived(const WidgetGUIEvent& aEvent)
{
  switch (aEvent.message) {
    case NS_COMPOSITION_START:
    case NS_COMPOSITION_CHANGE:
    case NS_COMPOSITION_COMMIT:
    case NS_COMPOSITION_COMMIT_AS_IS:
    case NS_SELECTION_SET:
      mContentCache.OnEventReceived();
      break;
    default:
      break;
  }
}

# Or, this switch statement should be in mContentCache.OnEventReceived().

Comment 22

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

is OnEventNeedingAckReceived too long?

Comment 23

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

(In reply to Olli Pettay [:smaug] from comment #22)
> is OnEventNeedingAckReceived too long?

No, I don't think so, but is it clear to the other developers? Isn't it still unclear what events are notified by the method? Or is it enough?

Comment 24

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

Well, then some comment somewhere. The "*NeedingAck" is IMO rather clear that it isn't some generic
method which should be called for all the events, but only for some special ones.

Comment 25

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

Okay, then, I'll use the name without additional argument.

Comment 26

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

Attachment: part.1 Child process should notify its parent process when it dispatches composition or selection event into the DOM tree

Comment 27

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

Attachment: part.2 ContentCache in parent process should manage how many events are sent but not yet received (r=smaug)

Comment 28

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

Attachment: part.3 Don't send selection change, text change nor composition update notification to IME from TabParent until all events sent to the child process is received by it

Comment 29

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

Attachment: part.4 Define IMENotification::TextChangeDataBase and whose member names should be same as IMEContentObserver::TextChangeData's (r=smaug)

Comment 30

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

Attachment: part.5 IMENotification::TextChangeDataBase should have a state which indicates that it's not initialized (r=smaug)

Comment 31

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

Attachment: part.6 IMEContentObserver should use IMENotification::TextChangeData (r=smaug)

Comment 32

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

Attachment: part.7 IMENotification::TextChangeData should have a method to merge with another instance and IMENotification::MergeWith() should use it (r=smaug)

Comment 33

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

Attachment: part.2 ContentCache in parent process should manage how many events are sent but not yet received (r=smaug)

Oops, I forget to call ContentCacheInParent::OnSelectionEvent() from TabParent::SendSelectionEvent().

Just a mistake, carrying over the r+.

Comment 34

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

Attachment: part.8 ContentCacheInParent::OnEventNeedingAckReceived() should log the event message

Anyway, this makes us debug easier. I hope you agree with this patch.

Comment 35

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

I'm investigating bug 1176955 now. Perhaps, it needs the part.8.

Comment 36

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

Comment on attachment 8631937 [details] [diff] [review]
part.3 Don't send selection change, text change nor composition update notification to IME from TabParent until all events sent to the child process is received by it

>+ContentCacheInParent::MaybeNotifyIME(nsIWidget* aWidget,
>+                                     IMENotification& aNotification)
> {
>-  if (NS_WARN_IF(aNotification.mMessage != NOTIFY_IME_OF_SELECTION_CHANGE)) {
>+  if (aNotification.mMessage == NOTIFY_IME_OF_SELECTION_CHANGE) {
>+    aNotification.mSelectionChangeData.mOffset = mSelection.StartOffset();
>+    aNotification.mSelectionChangeData.mLength = mSelection.Length();
>+    aNotification.mSelectionChangeData.mReversed = mSelection.Reversed();
>+    aNotification.mSelectionChangeData.SetWritingMode(mSelection.mWritingMode);
>+  }
This is a bit odd that one mMessage type is initialized here but not others.
But I guess it is fine, given that InitNotification does that without the patch.


>+ContentCacheInParent::FlushPendingNotifications(nsIWidget* aWidget)
>+{
>+  MOZ_ASSERT(!mPendingEventsNeedingAck);
>+
>+  // New notifications which are notified during flushing pending notifications
>+  // should be merged again.
>+  mPendingEventsNeedingAck++;
>+
>+  nsCOMPtr<nsIWidget> kungFuDeathGrip(aWidget);
>+
>+  if (mPendingTextChange.HasNotification()) {
>+    IMENotification notification(mPendingTextChange);
>+    if (!aWidget->Destroyed()) {
>+      mPendingTextChange.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  // First, text change notification should be sent because selection change
>+  // notification notifies IME of current selection range in the latest content.
>+  // So, IME may need the latest content before that.
Shouldn't this comment be above the previous 'if', since this talks mostly about textchange

>+  if (mPendingSelectionChange.HasNotification()) {
>+    IMENotification notification(mPendingSelectionChange);
>+    if (!aWidget->Destroyed()) {
>+      mPendingSelectionChange.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  if (mPendingCompositionUpdate.HasNotification()) {
>+    IMENotification notification(mPendingCompositionUpdate);
>+    if (!aWidget->Destroyed()) {
>+      mPendingCompositionUpdate.Clear();
>+      IMEStateManager::NotifyIME(notification, aWidget, true);
>+    }
>+  }
>+
>+  // Finally, send composition update notification because it notifies IME of
>+  // finishing handling whole sending events.
And this calks about compositionupdate, which is done above, so perhaps move also this to be above the
previous 'if'

Comment 37

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

(In reply to Olli Pettay [:smaug] from comment #36)
> Comment on attachment 8631937 [details] [diff] [review]
> part.3 Don't send selection change, text change nor composition update
> notification to IME from TabParent until all events sent to the child
> process is received by it
> 
> >+ContentCacheInParent::MaybeNotifyIME(nsIWidget* aWidget,
> >+                                     IMENotification& aNotification)
> > {
> >-  if (NS_WARN_IF(aNotification.mMessage != NOTIFY_IME_OF_SELECTION_CHANGE)) {
> >+  if (aNotification.mMessage == NOTIFY_IME_OF_SELECTION_CHANGE) {
> >+    aNotification.mSelectionChangeData.mOffset = mSelection.StartOffset();
> >+    aNotification.mSelectionChangeData.mLength = mSelection.Length();
> >+    aNotification.mSelectionChangeData.mReversed = mSelection.Reversed();
> >+    aNotification.mSelectionChangeData.SetWritingMode(mSelection.mWritingMode);
> >+  }
> This is a bit odd that one mMessage type is initialized here but not others.
> But I guess it is fine, given that InitNotification does that without the
> patch.

Yeah. But only the selection data is in ContentCacheInParent. I wonder why IMENotification isn't used at IPC.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/300b08b3eca6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b226ef6b9ca3
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0af72de7c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/890bb7cdc21a
https://hg.mozilla.org/integration/mozilla-inbound/rev/10807ee6ca50
https://hg.mozilla.org/integration/mozilla-inbound/rev/63e298231e56
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc21c056682
https://hg.mozilla.org/integration/mozilla-inbound/rev/8370d245d8aa

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/300b08b3eca6
https://hg.mozilla.org/mozilla-central/rev/b226ef6b9ca3
https://hg.mozilla.org/mozilla-central/rev/9d0af72de7c0
https://hg.mozilla.org/mozilla-central/rev/890bb7cdc21a
https://hg.mozilla.org/mozilla-central/rev/10807ee6ca50
https://hg.mozilla.org/mozilla-central/rev/63e298231e56
https://hg.mozilla.org/mozilla-central/rev/2bc21c056682
https://hg.mozilla.org/mozilla-central/rev/8370d245d8aa

Comment 40

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

Comment on attachment 8631923 [details] [diff] [review]
part.1 Child process should notify its parent process when it dispatches composition or selection event into the DOM tree

>  bool
>  TabChild::RecvCompositionEvent(const WidgetCompositionEvent& event)
>  {
> +  unused << SendOnEventNeedingAckReceived();
>    WidgetCompositionEvent localEvent(event);
>    localEvent.widget = mPuppetWidget;
>    APZCCallbackHelper::DispatchWidgetEvent(localEvent);
>    return true;
>  }
>  
>  bool
>  TabChild::RecvSelectionEvent(const WidgetSelectionEvent& event)
>  {
> +  unused << SendOnEventNeedingAckReceived();
>    WidgetSelectionEvent localEvent(event);
>    localEvent.widget = mPuppetWidget;
>    APZCCallbackHelper::DispatchWidgetEvent(localEvent);
>    return true;
>  }

Oh, I realized this now. Sending notification before dispatching the event into the tree may cause TabParent notifying IME of the previous state of the sending event. So, they should be notified after dispatching the event...