993714 - [e10s] Cache native key bindings in tests

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: native-patch

I ran into a problem in bug 989563. In that bug, we send every synthetic key event to the parent, where we compute the native key bindings, and then send it down to the child. As long as we only execute the native key bindings while processing the event, that works fine. To be careful, the patch asserts that we only access native key bindings while processing an event from the parent.

Unfortunately, the assertion is firing in this test:

http://mxr.mozilla.org/mozilla-central/source/dom/events/test/test_bug493251.html?force=1#116

It looks suppressEventHandling causes us to delay processing the key events until later. The delay only happens in the child since that's where suppressEventHandling runs. When we finally run the event, the cached key bindings are no longer valid.

I was curious why we even have native key bindings attached to events that are synthetically generated. I found the reason here:

http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#1646

To fix the problem, I added some similar code to PuppetWidget that retrieves the native key bindings for synthetic events using IPC. I think this is the best way to solve this unfortunate problem.

Comment 1

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

Comment on attachment 8403596 [details] [diff] [review]
native-patch

>diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
>--- a/dom/base/nsDOMWindowUtils.cpp
>+++ b/dom/base/nsDOMWindowUtils.cpp
>@@ -1230,17 +1230,17 @@ nsDOMWindowUtils::SendKeyEvent(const nsA
>     event.mFlags.mDefaultPrevented = true;
>   }
> 
>   nsEventStatus status;
>   nsresult rv = widget->DispatchEvent(&event, status);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   *aDefaultActionTaken = (status != nsEventStatus_eConsumeNoDefault);
>-  
>+
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsDOMWindowUtils::SendNativeKeyEvent(int32_t aNativeKeyboardLayout,
>                                      int32_t aNativeKeyCode,
>                                      int32_t aModifiers,
>                                      const nsAString& aCharacters,

Could you remove this change since you don't change around here?

> bool
> TabChild::RecvRealKeyEvent(const WidgetKeyboardEvent& event,
>                            const MaybeNativeKeyBinding& aBindings)
>- {
>+{
>+  PuppetWidget* widget = static_cast<PuppetWidget*>(mWidget.get());
>+  PuppetWidget::AutoInvalidateNativeKeyCommands autoInvalidate;

Looks like you assume that puppet widget doesn't already cache native key commands. However, isn't is possible if DOM key event handler synthesizes key event?

>+bool
>+TabParent::RecvRequestNativeKeyBindings(const WidgetKeyboardEvent& aEvent,
>+                                        MaybeNativeKeyBinding* aBindings)
>+{
>+  AutoInfallibleTArray<mozilla::CommandInt, 4> singleLine;
>+  AutoInfallibleTArray<mozilla::CommandInt, 4> multiLine;
>+  AutoInfallibleTArray<mozilla::CommandInt, 4> richText;
>+
>+  *aBindings = mozilla::void_t();
>+
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget) {
>+    return true;
>+  }
>+
>+  WidgetKeyboardEvent localEvent(aEvent);
>+
>+  if (!NS_SUCCEEDED(widget->AttachNativeKeyEvent(localEvent))) {
>+    return true;
>+  }

Use NS_FAILED().

>diff --git a/dom/ipc/TabParent.h b/dom/ipc/TabParent.h
>--- a/dom/ipc/TabParent.h
>+++ b/dom/ipc/TabParent.h
>@@ -216,16 +216,19 @@ public:
>     void NotifyTransformEnd(ViewID aViewId);
>     void Activate();
>     void Deactivate();
> 
>     bool MapEventCoordinatesForChildProcess(mozilla::WidgetEvent* aEvent);
>     void MapEventCoordinatesForChildProcess(const LayoutDeviceIntPoint& aOffset,
>                                             mozilla::WidgetEvent* aEvent);
> 
>+    virtual bool RecvRequestNativeKeyBindings(const mozilla::WidgetKeyboardEvent& aEvent,
>+                                                MaybeNativeKeyBinding* aBindings) MOZ_OVERRIDE;
>+

Strange indent.

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -8906,16 +8906,17 @@ PresShell::DelayedMouseEvent::DelayedMou
> PresShell::DelayedKeyEvent::DelayedKeyEvent(WidgetKeyboardEvent* aEvent) :
>   DelayedInputEvent()
> {
>   WidgetKeyboardEvent* keyEvent =
>     new WidgetKeyboardEvent(aEvent->mFlags.mIsTrusted,
>                             aEvent->message,
>                             aEvent->widget);
>   keyEvent->AssignKeyEventData(*aEvent, false);
>+  keyEvent->mFlags.mIsSynthesizedForTests = aEvent->mFlags.mIsSynthesizedForTests;
>   mEvent = keyEvent;
> }

I guess that some other flags should be copied too. But it's okay for now...

>diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm
>--- a/widget/cocoa/nsChildView.mm
>+++ b/widget/cocoa/nsChildView.mm
>@@ -1937,16 +1937,22 @@ nsChildView::GetInputContext()
>   // instead of nullptr since nullptr means that the platform has only one
>   // context per process.
>   if (!mInputContext.mNativeIMEContext) {
>     mInputContext.mNativeIMEContext = this;
>   }
>   return mInputContext;
> }
> 
>+NS_IMETHODIMP
>+nsChildView::AttachNativeKeyEvent(mozilla::WidgetKeyboardEvent& aEvent)
>+{
>+  return mTextInputHandler->AttachNativeKeyEvent(aEvent);
>+}

Hmm, I'm afraid this... Could you insert NS_ENSURE_TRUE(mTextInputHandler, NS_ERROR_NOT_AVAILABLE)?

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -1806,16 +1806,18 @@ public:
>     NS_IMETHOD_(void) SetInputContext(const InputContext& aContext,
>                                       const InputContextAction& aAction) = 0;
> 
>     /*
>      * Get current input context.
>      */
>     NS_IMETHOD_(InputContext) GetInputContext() = 0;
> 
>+    NS_IMETHOD AttachNativeKeyEvent(mozilla::WidgetKeyboardEvent& aEvent) = 0;

You need to change UUID of nsIWidget and you need sr too.

>@@ -270,16 +271,25 @@ PuppetWidget::DispatchEvent(WidgetGUIEve
> #ifdef DEBUG
>   debug_DumpEvent(stdout, event->widget, event,
>                   nsAutoCString("PuppetWidget"), 0);
> #endif
> 
>   NS_ABORT_IF_FALSE(!mChild || mChild->mWindowType == eWindowType_popup,
>                     "Unexpected event dispatch!");
> 
>+  AutoInvalidateNativeKeyCommands autoInvalidate;
>+  if (event->mFlags.mIsSynthesizedForTests) {
>+    WidgetKeyboardEvent* keyEvent = event->AsKeyboardEvent();
>+    if (keyEvent) {
>+      mTabChild->RequestNativeKeyBindings(keyEvent);
>+      mNativeKeyCommandsValid = true;
>+    }
>+  }

Hmm, I guess that mNativeKeyCommandsValid can be broken with any event dispatch from the dispatching key event handler and/or related events such as input event handler.

Maybe, you need to back up all cached values and restore them by the destructor of AutoInvalidateNativeKeyCommands?

>@@ -314,16 +324,21 @@ PuppetWidget::DispatchEvent(WidgetGUIEve
> 
> 
> NS_IMETHODIMP_(bool)
> PuppetWidget::ExecuteNativeKeyBinding(NativeKeyBindingsType aType,
>                                       const mozilla::WidgetKeyboardEvent& aEvent,
>                                       DoCommandCallback aCallback,
>                                       void* aCallbackData)
> {
>+  // Don't bother asserting on b2g since it doesn't have native key bindings.
>+#ifndef MOZ_B2G
>+  MOZ_ASSERT(mNativeKeyCommandsValid);
>+#endif
>+

Why don't you use:

#ifdef MOZ_B2G
  return false;
#else // #ifdef MOZ_B2G

...

#endif // #ifdef MOZ_B2G #else

?

Then, B2G doesn't need to pay redundant cost at run time.

>+  struct AutoInvalidateNativeKeyCommands
>+  {
>+    ~AutoInvalidateNativeKeyCommands()
>+    {
>+      mNativeKeyCommandsValid = false;
>+    }
>+  };

Oh, I've not known that nested class/struct can access its parent's member without pointer to the instance...

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: native-patch v2

This patch fixes the review issues.

> Looks like you assume that puppet widget doesn't already cache native key
> commands. However, isn't is possible if DOM key event handler synthesizes key
> event?

I'm not sure what you mean by this. Are you worried that we'll get a synthesized key event from the parent that include native key binding information, and we'll also request the native key binding data in PuppetWidget? I added a !mNativeKeyCommandsValid test to check for that. If that's not the problem you're worried about, can you please explain more?

Comment 3

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

(In reply to Bill McCloskey (:billm) from comment #2)
> > Looks like you assume that puppet widget doesn't already cache native key
> > commands. However, isn't is possible if DOM key event handler synthesizes key
> > event?
> 
> I'm not sure what you mean by this. Are you worried that we'll get a
> synthesized key event from the parent that include native key binding
> information, and we'll also request the native key binding data in
> PuppetWidget? I added a !mNativeKeyCommandsValid test to check for that. If
> that's not the problem you're worried about, can you please explain more?

Imagine like this code:

document.addEventListener("keydown", function (e) {
    domWindowUtils.sendKeyEvent(...);
  }, false);

If the "keydown" event is caused by nsIDOMWindowUtils.sendKeyEvent(), your methods may be called as nested.

Comment 4

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

Sorry, I couldn't review this today. I'd like to do it this weekend if it's possible. However, perhaps, I can do it on next Monday.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, I think the new patch handles nesting correctly.

Comment 6

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

Comment on attachment 8405093 [details] [diff] [review]
native-patch v2

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>+void
>+TabChild::RequestNativeKeyBindings(AutoCacheNativeKeyCommands *aAutoCache,

AutoCacheNativeKeyCommands* aAutoCache,

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h
>@@ -382,16 +386,18 @@ public:
>     void GetDefaultScale(double *aScale);
> 
>     ScreenOrientation GetOrientation() { return mOrientation; }
> 
>     void SetBackgroundColor(const nscolor& aColor);
> 
>     void NotifyPainted();
> 
>+    void RequestNativeKeyBindings(mozilla::widget::AutoCacheNativeKeyCommands *aAutoCache,
>+                                  WidgetKeyboardEvent* aEvent);

Same. The * should be immediately after the type name, instead of before argument.

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -95,18 +95,18 @@ typedef void* nsNativeWidget;
> #ifdef XP_WIN
> #define NS_NATIVE_TSF_THREAD_MGR       100
> #define NS_NATIVE_TSF_CATEGORY_MGR     101
> #define NS_NATIVE_TSF_DISPLAY_ATTR_MGR 102
> #define NS_NATIVE_ICOREWINDOW          103 // winrt specific
> #endif
> 
> #define NS_IWIDGET_IID \
>-{ 0x8e081187, 0xf123, 0x4572, \
>-  { 0x82, 0xc6, 0x4c, 0xcd, 0xc2, 0x0e, 0xbd, 0xf9 } }
>+{ 0x87d80888, 0x9917, 0x4bfb,                                           \
>+  { 0x81, 0xa9, 0x1c, 0x5e, 0x30, 0x9c, 0x78, 0xb4 } }

Could you remove the unnecessary white spaces before "\".

>@@ -1806,16 +1806,18 @@ public:
>     NS_IMETHOD_(void) SetInputContext(const InputContext& aContext,
>                                       const InputContextAction& aAction) = 0;
> 
>     /*
>      * Get current input context.
>      */
>     NS_IMETHOD_(InputContext) GetInputContext() = 0;
> 
>+    NS_IMETHOD AttachNativeKeyEvent(mozilla::WidgetKeyboardEvent& aEvent) = 0;

Please explain what the method does.

> NS_IMETHODIMP_(bool)
> PuppetWidget::ExecuteNativeKeyBinding(NativeKeyBindingsType aType,
>                                       const mozilla::WidgetKeyboardEvent& aEvent,
>                                       DoCommandCallback aCallback,
>                                       void* aCallbackData)
> {
>+  // Don't bother asserting on b2g since it doesn't have native key bindings.

Isn't enough just "b2g doesn't have native key bindings"?

>+#ifdef MOZ_B2G
>+  return false;
>+#else // #ifdef MOZ_B2G
>+  MOZ_ASSERT(mNativeKeyCommandsValid);
>+

>diff --git a/widget/xpwidgets/PuppetWidget.h b/widget/xpwidgets/PuppetWidget.h
>+struct AutoCacheNativeKeyCommands
>+{
>+  AutoCacheNativeKeyCommands(PuppetWidget* aWidget)
>+   : mWidget(aWidget)

Needs one more " " before ":".

>+  {
>+    mSavedValid = mWidget->mNativeKeyCommandsValid;
>+    mSavedSingleLine = mWidget->mSingleLineCommands;
>+    mSavedMultiLine = mWidget->mMultiLineCommands;
>+    mSavedRichText = mWidget->mRichTextCommands;
>+  }
>+
>+  void Clear()
>+  {
>+    mWidget->mNativeKeyCommandsValid = true;

"Clear" should strange to me since you set this flag true. Perhaps, CacheNoCommand() or something?

Please request sr for next patch, thanks.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm not familiar with the super-review policy. Who should do that?

Comment 8

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

(In reply to Bill McCloskey (:billm) from comment #7)
> I'm not familiar with the super-review policy. Who should do that?

Changing interface (nsI*) needs sr. I think roc is a good person for nsIWidget.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: native-patch v3

Comments addressed.

Comment 0 explains the reason for this patch. To put it more briefly, we have some Mac-specific code [1] that attaches a native key event to key events that are synthesized by tests. We need an additional workaround to do the same thing in e10s.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#1646

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a47bd7d0df

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2a47bd7d0df