Closed Bug 1339543 Opened 7 years ago Closed 7 years ago

PBrowser::Msg_RequestNativeKeyBindings is too slow

Categories

(Core :: DOM: Events, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Performance Impact high
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Per <https://docs.google.com/spreadsheets/d/1x_BWVlnQPg0DHbsrvPFX7g89lnFGa3lAIHWD_pLa_dE/edit#gid=738048355> this is our 5th worst sync IPC.

smaug, can we do something about this?
Flags: needinfo?(bugs)
It was added in bug 993714
Blocks: 993714
Flags: needinfo?(bugs) → needinfo?(wmccloskey)
Ehsan, I don't see that message in your spreadsheet at all. Is there a mistake?

Originally this message was only sent during tests. The RequestNativeKeyBindings call in PuppetWidget.cpp used to be gated on mIsSynthesizedForTests. Now it seems to be running in other cases. That happened either in bug 1271125 or bug 1321245, I think. Also, I don't understand this stuff very well. Masayuki, maybe you know what is going on here?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(masayuki)
Flags: needinfo?(ehsan)
We're using native key bindings (e.g., Emacs like shortcut keys on macOS) as far as possible. For user convenience, we retrieve native key binding information at runtime instead of declaring with XUL.

When the keyboard event is synthesized for tests, we need to to this via IPC because keyboard event is synthesized in the content process.

On the other hand, I'm not sure about mIsSuppressedOrDelayed case. I think that the original event of delayed event comes from parent process. Then, we must have native key binding information in the widget because TabParent::SendRealKeyEvent() sends it.
https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/ipc/TabParent.cpp#1456,1465,1472-1480

So, perhaps, DelayedKeyEvent should cache native key bindings stored in the widget and set it to widget when PresShell dispatches it and the process is not a main process. (Or WidgetKeyboardEvent should have the information instead of widget?)
Flags: needinfo?(masayuki)
(In reply to Bill McCloskey (:billm) from comment #2)
> Ehsan, I don't see that message in your spreadsheet at all. Is there a
> mistake?

The spreadsheet was edited by Harald while I was commenting on these bugs, and honestly I can't really trust it any more, not sure why this probe doesn't even show up.  The raw data is still in the "auto-import" sheet, but I cannot figure out how to reproduce the view I was looking at yesterday.  :/  I guess for now you should look at telemetry.m.o raw data.
Flags: needinfo?(ehsan)
Priority: -- → P1
Whiteboard: [qf:p1]
Do we have a plan here?
Flags: needinfo?(masayuki)
Should be not so hard to fix.

* WidgetKeyboardEvent should have commands.
* TextEventDispatcher should set them before dispatching keypress events.
* WidgetKeyboardEvent should have a method to execute the stored command.
* nsIWidget::ExecuteNativeKeyBinding() shouldn't be used from EditorEventListener::KeyPress() nor nsTextInputListener::HandleEvent().
* Some additional hack for delayed keyboard events, perhaps.
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #6)
> Should be not so hard to fix.
> 
> * WidgetKeyboardEvent should have commands.
> * TextEventDispatcher should set them before dispatching keypress events.
> * WidgetKeyboardEvent should have a method to execute the stored command.
> * nsIWidget::ExecuteNativeKeyBinding() shouldn't be used from
> EditorEventListener::KeyPress() nor nsTextInputListener::HandleEvent().
> * Some additional hack for delayed keyboard events, perhaps.

Oops, I misunderstood because forgot that this is not a problem when actual user input. So, those approaches are wrong.

About the issue for delayed events, we need to improve it. However, do we need to work on for synthesized events? I think that for solving it, child process should cache key bindings, however, I think that it just wants memory in real use.

For fixing the former issue, taking this.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Comment on attachment 8868921 [details]
Bug 1339543 part 3 PuppetWidget should stop getting all edit commands before dispatching keyboard events which are synthesized for tests

https://reviewboard.mozilla.org/r/140558/#review144138

::: commit-message-12e9b:9
(Diff revision 1)
> +
> +So, PuppetWidget should stop calling TabChild::RequestNativeKeyBindings() before dispatching keyboard events.
> +
> +This patch changes browser_audioTabIcon.js which becomes permanent orange with this change.
> +
> +The test has odd asyc method, wait_for_tab_playing_event(), which never returns Promise but is used with await. Additionally, this runs immediately after |await ContentTask.spawn()| in pause(), i.e., before |await awaitTabPausedAttrModified;|, and the last check of pause() causes orange.  Therefore, this patch makes wait_for_tab_playing_event() may return Promise when it needs to wait TabAttrModified event.  Additionally, makes pause() use |await wait_for_tab_playing_event(tab, false);| directly before checking if tab has soundplaying attribute.

This kind of comments shouldn't IMO go to commit message, but be just a comment in bugzilla or something
Attachment #8868921 - Flags: review?(bugs) → review+
ok, this all is complicated. Takes some time to figure out what is changed and why... but review is coming.
Comment on attachment 8868919 [details]
Bug 1339543 part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it

https://reviewboard.mozilla.org/r/140554/#review144140

::: commit-message-7cc61:11
(Diff revision 1)
> +
> +We can make this simpler if we make WidgetKeyboardEvent store edit commands for
> +the key combination.  Then, child process can handle it even if it's delayed
> +event or it's a nested event.
> +
> +This patch adds WidgetKeyboardEvent::ExecuteEditCommands() for the wrapper and

for the wrapper? What wrapper is this comment talking about?

::: dom/html/nsTextEditorState.cpp:1004
(Diff revision 1)
>    if (!widget) {
>      widget = mFrame->GetNearestWidget();
>      NS_ENSURE_TRUE(widget, NS_OK);
>    }
> -                                         
> -  if (widget->ExecuteNativeKeyBinding(nativeKeyBindingsType,
> +
> +  AutoRestore<nsCOMPtr<nsIWidget>> saveWidget(keyEvent->mWidget);

I don't understand this change.
This definitely needs a comment why we need to change the widget.

I guess the reason is that ExecuteEditCommands wants some widget. But in which case do we not have a widget? When using events for testing? Should we make them have widget?

::: editor/libeditor/EditorEventListener.cpp:630
(Diff revision 1)
>      widget = pc ? pc->GetNearestWidget() : nullptr;
>      NS_ENSURE_TRUE(widget, NS_OK);
>    }
>  
>    nsCOMPtr<nsIDocument> doc = editorBase->GetDocument();
> -  bool handled = widget->ExecuteNativeKeyBinding(
> +  AutoRestore<nsCOMPtr<nsIWidget>> saveWidget(aKeyboardEvent->mWidget);

ditto

::: widget/TextEvents.h:469
(Diff revision 1)
>    static CodeNameIndexHashtable* sCodeNameIndexHashtable;
> +
> +  // mEditCommandsFor*Editor store edit commands.  This should be initialized
> +  // with InitEditCommandsFor().
> +  // XXX Ideally, this should be array of Command rather than CommandInt.
> +  //     However, ParamTraits isn't aware with enum array.

aware of

::: widget/TextEvents.h:470
(Diff revision 1)
> +
> +  // mEditCommandsFor*Editor store edit commands.  This should be initialized
> +  // with InitEditCommandsFor().
> +  // XXX Ideally, this should be array of Command rather than CommandInt.
> +  //     However, ParamTraits isn't aware with enum array.
> +  InfallibleTArray<CommandInt> mEditCommandsForSingleLineEditor;

Just nsTArray
Attachment #8868919 - Flags: review?(bugs) → review+
Comment on attachment 8868923 [details]
Bug 1339543 part 5 Remove unnecessary stuff from PuppetWidget

https://reviewboard.mozilla.org/r/140562/#review144148
Attachment #8868923 - Flags: review?(bugs) → review+
Comment on attachment 8868920 [details]
Bug 1339543 part 2 eKeyPress event should have edit commands for all editor types when it's dispatched to a remote process

https://reviewboard.mozilla.org/r/140556/#review144162

::: widget/TextEvents.h:299
(Diff revision 1)
> +   * process.
> +   */
> +  void InitAllEditCommands();
> +
> +  /**
> +   * PreventNativeKeyBindings() makes the instance doesn't cause any edit

makes the instance to not cause ...

::: widget/TextEvents.h:328
(Diff revision 1)
> +
> +  /**
> +   * IsAllEditCommandsInitialized() returns true if edit commands for all types
> +   * were already initialized.  Otherwise, false.
> +   */
> +  bool IsAllEditCommandsInitialized() const

I think this should be
AreAllEditCommandsInitialized

::: widget/WidgetEventImpl.cpp:608
(Diff revision 1)
>  WidgetKeyboardEvent::CodeNameIndexHashtable*
>    WidgetKeyboardEvent::sCodeNameIndexHashtable = nullptr;
>  
> +void
> +WidgetKeyboardEvent::InitAllEditCommands()
> +{

Is it ok to re-initialize? If not, should there be an assertion for that.
Attachment #8868920 - Flags: review?(bugs) → review+
Comment on attachment 8868922 [details]
Bug 1339543 part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieves edit commands for the type

https://reviewboard.mozilla.org/r/140560/#review144168

::: commit-message-60e4f:1
(Diff revision 1)
> +Bug 1339543 part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieve edit commands for the type r?smaug

s/retrieve/retrieves/

::: dom/ipc/TabChild.h:516
(Diff revision 1)
>  
>    void NotifyPainted();
>  
> -  void RequestNativeKeyBindings(mozilla::widget::AutoCacheNativeKeyCommands* aAutoCache,
> -                                const WidgetKeyboardEvent* aEvent);
> +  void RequestNativeKeyBindings(nsIWidget::NativeKeyBindingsType aType,
> +                                const WidgetKeyboardEvent& aEvent,
> +                                InfallibleTArray<CommandInt>& aCommands);

I think I'd still prefer use of nsTArray everywhere, but up to you.
Attachment #8868922 - Flags: review?(bugs) → review+
Comment on attachment 8868924 [details]
Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type

https://reviewboard.mozilla.org/r/140564/#review144184

::: commit-message-5203c:3
(Diff revision 1)
> +Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type r?smaug
> +
> +PBrowser::RequestNativeKeyBindings() is used only when somebody tries to execute native key bindings for synthesized keyboard events.  Therefore, it doesn't need to retrieve edit commands for all editor types.  Instead, it should take the editor type and just return the edit commands for it.

Given that this is testing only, why do we need to optimize here at all? Why not just send all the commands?

But since you already wrote the patch, I guess it is fine :)

::: dom/ipc/TabChild.cpp:1881
(Diff revision 1)
>    }
>    return IPC_OK();
>  }
>  
>  void
> -TabChild::RequestNativeKeyBindings(nsIWidget::NativeKeyBindingsType aType,
> +TabChild::RequestEditCommands(nsIWidget::NativeKeyBindingsType aType,

is there any way we could assert here that this method is called only when doing testing?

::: dom/ipc/TabParent.cpp:1233
(Diff revision 1)
> +    case nsIWidget::NativeKeyBindingsForSingleLineEditor:
> +    case nsIWidget::NativeKeyBindingsForMultiLineEditor:
> +    case nsIWidget::NativeKeyBindingsForRichTextEditor:
> +      break;
> +    default:
> +      return IPC_OK();

Shouldn't this be IPC_FAIL or some such
Attachment #8868924 - Flags: review?(bugs) → review+
Comment on attachment 8868919 [details]
Bug 1339543 part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it

https://reviewboard.mozilla.org/r/140554/#review144140

> for the wrapper? What wrapper is this comment talking about?

Ah, I meant "as a wrapper of nsIWidget::ExecuteNativeKeyBinding()".

I'll update the commit message.

> I don't understand this change.
> This definitely needs a comment why we need to change the widget.
> 
> I guess the reason is that ExecuteEditCommands wants some widget. But in which case do we not have a widget? When using events for testing? Should we make them have widget?

If event is created by chrome script, it doesn't have widget even it returns true when calling IsTrusted().

I'll add comment to there.
Comment on attachment 8868919 [details]
Bug 1339543 part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it

https://reviewboard.mozilla.org/r/140554/#review144140

> Ah, I meant "as a wrapper of nsIWidget::ExecuteNativeKeyBinding()".
> 
> I'll update the commit message.

Modified the last paragraph to:

> This patch adds arrays to WidgetKeyboardEvent to store edit commands which are
> initialized with nsIWidget::ExecuteNativeKeyBinding() and adds
> WidgetKeyboardEvent::ExecuteEditCommands() to execute stored edit commands as
> same as nsIWidget::ExecutenativeKeyBinding().
Comment on attachment 8868920 [details]
Bug 1339543 part 2 eKeyPress event should have edit commands for all editor types when it's dispatched to a remote process

https://reviewboard.mozilla.org/r/140556/#review144162

> Is it ok to re-initialize? If not, should there be an assertion for that.

Although, it's checked in InitEditCommandsFor(), I added MOZ_ASSERT() with !AreAllEditCommandsInitialized().
Comment on attachment 8868921 [details]
Bug 1339543 part 3 PuppetWidget should stop getting all edit commands before dispatching keyboard events which are synthesized for tests

https://reviewboard.mozilla.org/r/140558/#review144138

> This kind of comments shouldn't IMO go to commit message, but be just a comment in bugzilla or something

Okay, I dropped this paragraph:

> The test has odd asyc method, wait_for_tab_playing_event(), which never returns
> Promise but is used with await. Additionally, this runs immediately after
> |await ContentTask.spawn()| in pause(), i.e., before |await awaitTabPausedAttrModified;|,
> and the last check of pause() causes orange.  Therefore, this patch makes
> wait_for_tab_playing_event() may return Promise when it needs to wait TabAttrModified event.
> Additionally, makes pause() use |await wait_for_tab_playing_event(tab, false);| directly
> before checking if tab has soundplaying attribute.
Comment on attachment 8868922 [details]
Bug 1339543 part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieves edit commands for the type

https://reviewboard.mozilla.org/r/140560/#review144168

> I think I'd still prefer use of nsTArray everywhere, but up to you.

I like nsTArray better (because of really shorter name), but the old code used InfallibleTArray explicitly. Therefore, I tried to keep it.

But I change them to nsTArray.
Comment on attachment 8868924 [details]
Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type

https://reviewboard.mozilla.org/r/140564/#review144184

> Given that this is testing only, why do we need to optimize here at all? Why not just send all the commands?
> 
> But since you already wrote the patch, I guess it is fine :)

If there were some add-ons which is like IME or software keyboard, it can synthesize keyboard events not for tests (mIsSynthesizedForTests becomes false).
https://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/widget/TextEventDispatcher.cpp#161,165
https://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/widget/TextEventDispatcher.h#374,378,385,390,392-393

Although, I guess there are no such add-ons.

> is there any way we could assert here that this method is called only when doing testing?

Sure. Like PuppetWidget::DispatchEvent(), I'll add MOZ_ASSERT() here.

> Shouldn't this be IPC_FAIL or some such

Okay. But I usually see IPC_OK() even if it obviously unexpected case.
Comment on attachment 8868924 [details]
Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type

https://reviewboard.mozilla.org/r/140564/#review144184

> Sure. Like PuppetWidget::DispatchEvent(), I'll add MOZ_ASSERT() here.

Hmm, unfortunately, it's impossible for now because we cannot check if it's created by chrome script (bug 1345374).
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/68c474271ba5
part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it r=smaug
https://hg.mozilla.org/integration/autoland/rev/198e58f7052f
part 2 eKeyPress event should have edit commands for all editor types when it's dispatched to a remote process r=smaug
https://hg.mozilla.org/integration/autoland/rev/a950fbfa6d8c
part 3 PuppetWidget should stop getting all edit commands before dispatching keyboard events which are synthesized for tests r=smaug
https://hg.mozilla.org/integration/autoland/rev/b75c111837a8
part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieves edit commands for the type r=smaug
https://hg.mozilla.org/integration/autoland/rev/8d32e150bf7f
part 5 Remove unnecessary stuff from PuppetWidget r=smaug
https://hg.mozilla.org/integration/autoland/rev/bf71ece03a29
part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type r=smaug
Depends on: 1374862
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.