PBrowser::Msg_RequestNativeKeyBindings is too slow

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Events
P1
normal
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: Ehsan, Assigned: masayuki)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

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)

Comment 1

4 months ago
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
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a91e5311b89dcdfc4d37449a36ef7dc744505fd
https://treeherder.mozilla.org/#/jobs?repo=try&revision=045dfe58d442b62f86ba543f5cdf916cb328f493
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fae85bb6796dc59ea6472615a2edd9fa9f2ff122
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b28935e16a1488ecb241637fe045867d03472739
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1d6cae7ecd803708690494c20eff0fccd42f97d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c21f95feafa3531c6c8d8b7d7d8cc6096c90a88
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0288bb0adc889fb265466a2f997896068200a8f2
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8890b8cdd3a0928898944ef6fd95d5e8b261e839
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d03478c11f6217b3a8a27650c22943e6ea86092
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f48d7bee2970a9acf6479456b65bb6c3d3fa90b2
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46519d402d501d5406c9a170a7b02d24054cd4a0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=614eca6fb39cba7d2013de9565ab7b05dafa4fd8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5952d0d29d1acbc61d22bcc3dcee28bea23d9b35
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

a month ago
mozreview-review
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 29

a month ago
mozreview-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

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

a month ago
mozreview-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 31

a month ago
mozreview-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 32

a month ago
mozreview-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 33

a month ago
mozreview-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+
(Assignee)

Comment 34

a month ago
mozreview-review-reply
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.
(Assignee)

Comment 35

a month ago
mozreview-review-reply
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().
(Assignee)

Comment 36

a month ago
mozreview-review-reply
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().
(Assignee)

Comment 37

a month ago
mozreview-review-reply
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.
(Assignee)

Comment 38

a month ago
mozreview-review-reply
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.
(Assignee)

Comment 39

a month ago
mozreview-review-reply
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.
(Assignee)

Comment 40

a month ago
mozreview-review-reply
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).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 47

a month ago
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

Comment 48

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/68c474271ba5
https://hg.mozilla.org/mozilla-central/rev/198e58f7052f
https://hg.mozilla.org/mozilla-central/rev/a950fbfa6d8c
https://hg.mozilla.org/mozilla-central/rev/b75c111837a8
https://hg.mozilla.org/mozilla-central/rev/8d32e150bf7f
https://hg.mozilla.org/mozilla-central/rev/bf71ece03a29
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.