1251063 - Opening new windows through SendCreateWindow sends two sync IME messages

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

Here's a profile I gathered while investigating the e10s tpaint regression (bug 1174770):

http://people.mozilla.org/~bgirard/cleopatra/?zippedProfile=http://mozilla-releng-blobs.s3.amazonaws.com/blobs/Try/sha512/6c9ae00721db9ecad047d736bb9484422b3ac8ab6a42ab0ddf8aba98c90f53d506a27d624a8cec55e3387f61ff2c77424eab58eb58f8a585131ab30831fdabf3#report=de6ea8e7ef2b36b67bb0821b7e3cd355f69289c5&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A4391,%22end%22%3A4992%7D%5D&selection=0,6937,6,7,6938,6939,4,5,6,7,6938,8,9,10,1831,1832,1833,1834,1835,1836,1837,897,26,27,28,6945,29,30,27,2044,6946,6947,6948,6949,6950,51,52,6951,6952,6957,6958,1589,1590,1591,1592,1593,1594,1595,1596,46

In the segment that's being viewed, towards the end, there are two sync IPC messages, both having to do with IME. Specifically, they're for the GetInputContext message:

https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/dom/ipc/PBrowser.ipdl#305

Both of these messages are sent due to the nsFocusManager having ::Focus called on it. I understand we cannot avoid the sync message (see bug 1193454), but I feel we can probably try to avoid sending the message twice here.

This accounts for about 10ms of the ~60ms regression that we're seeing in bug 1174770.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hello :masayuki! Do you know if it's possible to avoid sending the sync message twice?

Comment 2

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

Yeah, it must be possible.

IMEStateManager::SetIMEState() always calls nsIWidget::GetInputContext() and its callers also (sometimes or always) call it too. So, SetIMEState() should take the result of GetInputContext() via its argument.

Comment 3

[Jim Mathies [:jimm]]

depends on what we decide in bug 1251032 .

Comment 4

[Mike Conley (:mconley) (:⚙️)]

I think we can deprioritize this. The IME messages are only coming up because of the focus being switched to the remote browser, and that focus only occurs after _delayedStartup. I think I can get around this regression by waiting until both content and the parent have painted before running _delayedStartup.

Comment 5

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

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

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 4a - Make WebRTC PeerConnection use the public Telemetry API

This should keep the previous behavior as far as i can tell, as there is only one global instance of mConnectionCounter.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8739405 [details] [diff] [review]
Part 4a - Make WebRTC PeerConnection use the public Telemetry API

Bug number typo, sorry about the noise :)

Comment 9

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

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

Comment 10

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

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

Comment 11

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

Attachment: MozReview Request: Bug 1251063 PuppetWidget should cache InputContext which is set with SetInputContext() and use it in GetInputContext() only when it is the widget which has active input context in the process r?smaug

PuppetWidget::GetInputContext() needs to communicate with its parent process with synchronous IPC. This is very expensive for focus move.

Currently, IMEStateManager uses nsIWidget::GetInputContext() only for checking the IME enabled state.  Therefore, it's enough to cache input context when nsIWidget::SetInputContext() is called.  Then, we can avoid to communicate with synchronous IPC with PuppetWidget::GetInputContext() in most cases.

Fortunately, our IME enabled state management across process boundary is simple.  Only focused process manages InputContext and when new remote process gets focus, its parent disables IME temporarily.  Therefore, PuppetWidget should cache only while the process's IMEStateManager is managing InputContext.  Otherwise, it should use synchronous IPC.

This patch gives up to support retrieving IME open state from child process. However, perhaps, this is not necessary for everybody including add-on developers because the only user of IME open state in child process is nsIDOMWindowUtils. So, add-ons can send IME open state from chrome process instead.  If this decision is wrong, unfortunately, we should support it again in another bug.  It's easy to support with creating another nsIWidget::GetInputContext() or adding additional argument to it.

Review commit: https://reviewboard.mozilla.org/r/55782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55782/

Comment 12

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

Comment on attachment 8757244 [details]
MozReview Request: Bug 1251063 PuppetWidget should cache InputContext which is set with SetInputContext() and use it in GetInputContext() only when it is the widget which has active input context in the process r?smaug

https://reviewboard.mozilla.org/r/55782/#review52640

r+ if some explanation is given.

::: dom/events/IMEStateManager.cpp:243
(Diff revision 1)
>      NotifyIME(REQUEST_TO_COMMIT_COMPOSITION, sPresContext);
>    }
> +  if (sPresContext) {
> +    nsCOMPtr<nsIWidget> widget = sPresContext->GetRootWidget();
> +    if (widget) {
> +      widget->OnStopManagingInputContext();

Do we actually need this call here. Please explain.

::: widget/PuppetWidget.h:180
(Diff revision 1)
>  
>    NS_IMETHOD_(void) SetInputContext(const InputContext& aContext,
>                                      const InputContextAction& aAction) override;
>    NS_IMETHOD_(InputContext) GetInputContext() override;
> +  virtual void OnStopManagingInputContext() override;
> +  void WillStartToManageInputContext();

Perhaps call this StartToManageInputContext() ?
Or just ManageInputContext();

::: widget/PuppetWidget.cpp:709
(Diff revision 1)
> +  NS_WARN_IF(mInputContext.mIMEState.mEnabled == IMEState::UNKNOWN);
> +
>    mInputContext = aContext;
> +  // Cannot cache IME open state because it can be changed without any
> +  // notifications on some platforms.
> +  mInputContext.mIMEState.mOpen = IMEState::OPEN_STATE_NOT_SUPPORTED;

I don't understand this change. Please explain.

Comment 13

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

https://reviewboard.mozilla.org/r/55782/#review52640

> Do we actually need this call here. Please explain.

Yes, we need to do it. When IMEStateManager::StopIMEStateManagement() is called by its parent process, cached InputContext in current PuppetWidget may be different from actual input context of chrome's widget. Therefore, IMEStateManager needs to notify PuppetWidget of its InputContext cache becoming outdated. Then, PuppetWidget needs to mark its cache invalid.

But I realized that this is not enough because a content process can have multiple PuppetWidgets. So, when a PuppetWidget instance becomes to be not managed by IMEStateManager, IMEStateManager makes PuppetWidget stop caching its InputContext. I'll update this patch.

> Perhaps call this StartToManageInputContext() ?
> Or just ManageInputContext();

Okay, as I answered to the first question, OnStopManagingInputContext() may not be proper name. So, I'll try to think this method's name too.

> I don't understand this change. Please explain.

Content can change IME open state when new editable element gets focus (with specifying CSS ime-mode). However, the open state can be changed by the user and we cannot know the state change in some platforms. Therefore, we cannot cache it in child process. Therefore, we needs to change the cache's IME open state to "not supported". It might be better to do this in GetInputContext(). But anyway, the open state may not be corrent. So, I think that it does make sense always to mark the cache's open state as unsupported.

Comment 14

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

Comment on attachment 8757244 [details]
MozReview Request: Bug 1251063 PuppetWidget should cache InputContext which is set with SetInputContext() and use it in GetInputContext() only when it is the widget which has active input context in the process r?smaug

So, this patch has a bug.

Comment 15

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

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

Comment 16

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

Comment on attachment 8757244 [details]
MozReview Request: Bug 1251063 PuppetWidget should cache InputContext which is set with SetInputContext() and use it in GetInputContext() only when it is the widget which has active input context in the process r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55782/diff/1-2/

Comment 17

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

Comment on attachment 8757244 [details]
MozReview Request: Bug 1251063 PuppetWidget should cache InputContext which is set with SetInputContext() and use it in GetInputContext() only when it is the widget which has active input context in the process r?smaug

See above message (commit message).

FYI: This is simpler fix than the old approach. But first call of nsIWidget::GetInputContext() from IMEStateManager::SetInputContext() causes synchronous IPC since PuppetWidget doesn't initialize its InputContext cache with "Disabled" state when starting to manage InputContext.

However, the call in IMEStateManager::SetInputContext() is for optimization and it's not worthwhile to do it instead of using synchronous IPC. Therefore, I just removed it for now. Fortunately, looks like that nobody is using "ime-enabled-state-changed" in both m-c and gaia. Therefore, I filed bug 1276406 to remove it.

Comment 18

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

https://reviewboard.mozilla.org/r/55782/#review52640

> Yes, we need to do it. When IMEStateManager::StopIMEStateManagement() is called by its parent process, cached InputContext in current PuppetWidget may be different from actual input context of chrome's widget. Therefore, IMEStateManager needs to notify PuppetWidget of its InputContext cache becoming outdated. Then, PuppetWidget needs to mark its cache invalid.
> 
> But I realized that this is not enough because a content process can have multiple PuppetWidgets. So, when a PuppetWidget instance becomes to be not managed by IMEStateManager, IMEStateManager makes PuppetWidget stop caching its InputContext. I'll update this patch.

Now, this call is dropped from the new patch.

> Okay, as I answered to the first question, OnStopManagingInputContext() may not be proper name. So, I'll try to think this method's name too.

And also this method is removed from the new patch.

Comment 19

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

Comment on attachment 8757244 [details]
MozReview Request: Bug 1251063 PuppetWidget should cache InputContext which is set with SetInputContext() and use it in GetInputContext() only when it is the widget which has active input context in the process r?smaug

https://reviewboard.mozilla.org/r/55782/#review52974

The question answered, r+.

::: widget/PuppetWidget.cpp:725
(Diff revision 2)
> +
> +  // When this widget caches input context and currently managed by
> +  // IMEStateManager, the cache is valid.  Only in this case, we can
> +  // avoid to use synchronous IPC.
> +  if (mInputContext.mIMEState.mEnabled != IMEState::UNKNOWN &&
> +      IMEStateManager::GetWidgetForActiveInputContext() == this) {

Is it ever possible that we quickly switch active input context to another widget and then back to the original and then just end up having this expression true here? And if yes, is mInputContext still valid in such case?

Comment 20

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

https://reviewboard.mozilla.org/r/55782/#review52974

> Is it ever possible that we quickly switch active input context to another widget and then back to the original and then just end up having this expression true here? And if yes, is mInputContext still valid in such case?

Unfortunately, no. mInputContext is invalid in such case. However, fortunately, IMEStateManager doesn't use nsIWidget::GetInputContext() when focus is actually changing (in this case, from nothing to someone) because always need to set other informations like editor type. So, at least with current code, PuppetWidget::GetInputContext() is never called before a call of SetInputContext().

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/370c3f499098

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/370c3f499098

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

this has a perf win for winxp tps talos test in e10s mode:
https://treeherder.mozilla.org/perf.html#/alerts?id=1376

thanks for the patch!

Comment 24

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

(In reply to Joel Maher (:jmaher) from comment #23)
> this has a perf win for winxp tps talos test in e10s mode:
> https://treeherder.mozilla.org/perf.html#/alerts?id=1376
> 
> thanks for the patch!

no problem, and sorry for the delay to fix this.