1110888 - [e10s] Plugin IME broken in e10s mode on OS X

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

The patch for bug 1092630 (which just landed on trunk) makes non-IME keyboard input work in plugins on OS X.  But the design that was chosen (to process plugin keyboard events in a background process) makes it impossible to do IME in e10s mode.  Both Chrome and Safari/WebKit process plugin keyboard events in the main process.  We need to do the same.

This is (probably) a large project, which will involve changes both to Mac-specific and to cross-platform code (particularly to IPC code).  I'm probably the person best suited to do the work, but I'll almost certainly never have time for it.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Log of plugin text input in Chromium (as of 10-2014)

A while ago I wrote a debugging patch and interpose library to catch (in logs) how Chromium handles plugin text input.  Here's the log.  I'll post the patch and the interpose library in my next two comments.

I was originally also going to do this for Safari/WebKit, but never found the time for it.

The log was made of an IME input session in my Debug Events Plugin from bug 441880, running in Chromium.

While the Debug Events Plugin had keyboard focus, I performed the following steps:

1) Set the input source to Hiragana (in the Flags menu)
2) Type 'n'
3) Type 'i'
4) Type 'h'
5) Type 'o'
6) Type 'n'
7) Type 'g'
8) Type 'o'
9) Hit Return

Doing this generates the following text: にほんご

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Chromium debug logging patch used to generate log

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Interpose library used to generate log

Comment 4

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #1)

Something else I should have mentioned:

If I remember right, I performed this test on OS X 10.9.5.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> I'm probably the person best suited to do the work, but I'll almost
> certainly never have time for it.

Thinking more about this, it probably won't be as complex as I first
thought.  So I'll at least try to get started on it in January.

If I can make this work, it will supercede bug 1110884, which will no
longer be required.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Feel free to take this, David, if you feel you can make headway on it.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

[Tracking Requested - why for this release]: This will fix bug 1110884, which is tracking firefox 37

Comment 8

[Benjamin Smedberg]

Technically bug 1110884 is blocking 37, and this one doesn't need to, but if the same work fixes both of them then it doesn't matter.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm going to track both bugs in case this fix doesn't fix bug 1110884.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

I've been working on this bug for a couple of days and things are going reasonably well.  I hope to be done by the end of this week (to have a patch that's simple and safe enough to land on the 37 branch shortly after it's landed on the trunk).

Masayuki, I plan to ask you to review the patch.  This is just to give you a heads up :-)

Comment 11

[Steven Michaud [:smichaud] (Retired)]

I'm making good progress on this, but it's going to leak into next week.

I ran into a couple of unexpected problems.  So far I've fixed them all ... but that's taken longer than I expected.  I think I probably need another couple of days of work to finish my patch.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's my patch.

It was more work than I expected, and is a bit more complicated than I hoped.  For example I had to add two IPC calls, one of which (StartPluginIME()) is synchronous.  But in limited testing it works at least as well (doing IME), in both e10s mode and non-e10s mode, as the old code replaced by Josh's patch for bug 1092630 (which is still on the 36 branch and earlier).  I tested on OS X 10.6.8, 10.7.5 and 10.10.2 and didn't see any differences in behavior (or any problems).

I've started a set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5613c91e02e2

Masayuki, please review as much of this as you can.  I do realize I'll probably have to get additional reviews, at least of the dom/html changes.  Please also test my tryserver build -- you know a lot more about IME (and the different IME clients available) than I do.

I don't expect StartPluginIME() to cause trouble as a synchronous call.  If it does, it'd be possible to create an asynchronous version of this call (for use only in e10s mode).  But I really hope I don't have to do this, since this would mean that plugins will receive some events out of order in e10s mode (for example the text event corresponding to a key-down event would be received after the matching key-up event).

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (fix Windows builds)

Here's another set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d070ff4c277

Comment 14

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

Comment on attachment 8566373 [details] [diff] [review]
Fix rev1 (fix Windows builds)

Although, probably I cannot test this today. But I'll do it as soon as possible.

> NS_IMETHODIMP
>+HTMLObjectElement::PostHandleEvent(EventChainPostVisitor& aVisitor)
>+{
>+  switch (aVisitor.mEvent->message) {
>+    case NS_FOCUS_CONTENT: {
>+      nsIWidget* widget = NULL;
>+      nsIFrame* frame = GetPrimaryFrame();
>+      if (frame) {
>+        widget = frame->GetNearestWidget();
>+      }
>+      if (widget) {
>+        bool value = true;
>+        widget->SetPluginFocused(value);
>+      }
>+      break;
>+    }
>+    case NS_BLUR_CONTENT: {
>+      nsIWidget* widget = NULL;
>+      nsIFrame* frame = GetPrimaryFrame();
>+      if (frame) {
>+        widget = frame->GetNearestWidget();
>+      }
>+      if (widget) {
>+        bool value = false;
>+        widget->SetPluginFocused(value);
>+      }
>+      break;
>+    }
>+  }
>+  return NS_OK;
>+}

>+NS_IMETHODIMP
>+HTMLSharedObjectElement::PostHandleEvent(EventChainPostVisitor& aVisitor)
>+{
>+  switch (aVisitor.mEvent->message) {
>+    case NS_FOCUS_CONTENT: {
>+      nsIWidget* widget = NULL;
>+      nsIFrame* frame = GetPrimaryFrame();
>+      if (frame) {
>+        widget = frame->GetNearestWidget();
>+      }
>+      if (widget) {
>+        bool value = true;
>+        widget->SetPluginFocused(value);
>+      }
>+      break;
>+    }
>+    case NS_BLUR_CONTENT: {
>+      nsIWidget* widget = NULL;
>+      nsIFrame* frame = GetPrimaryFrame();
>+      if (frame) {
>+        widget = frame->GetNearestWidget();
>+      }
>+      if (widget) {
>+        bool value = false;
>+        widget->SetPluginFocused(value);
>+      }
>+      break;
>+    }
>+  }
>+  return NS_OK;
>+}

Looks like that these methods are same. Cannot you share then with:

static nsresult MaybeOnPluginFocus(Element* aElement);
static nsresult MaybeOnPluginBlur(Element* aElement);

they could be in HTMLObjectElement?

>     case NS_KEY_DOWN:
>     case NS_KEY_UP:
>     {
>       WidgetKeyboardEvent* keyEvent = anEvent->AsKeyboardEvent();
> 
>+      // That keyEvent->mPluginTextEventString is non-empty is a signal that we should
>+      // create a text event for the plugin, instead of a key event.
>+      if ((anEvent->message == NS_KEY_DOWN) && keyEvent->mPluginTextEventString.Length()) {

nit: Use !keyEvent->mPluginTextEventString.IsEmpty().

>+      } else {
>+        cocoaEvent.data.key.keyCode = keyEvent->mNativeKeyCode;
>+        cocoaEvent.data.key.isARepeat = keyEvent->mIsRepeat;
>+        cocoaEvent.data.key.modifierFlags = keyEvent->mNativeModifierFlags;
>+        const char16_t* nativeChars = keyEvent->mNativeCharacters.get();
>+        cocoaEvent.data.key.characters =
>+          (NPNSString*)::CFStringCreateWithCharacters(NULL,
>+                                                      reinterpret_cast<const UniChar*>(nativeChars),
>+                                                      keyEvent->mNativeCharacters.Length());
>+        const char16_t* nativeCharsIgnoringModifiers = keyEvent->mNativeCharactersIgnoringModifiers.get();
>+        cocoaEvent.data.key.charactersIgnoringModifiers =
>+          (NPNSString*)::CFStringCreateWithCharacters(NULL,
>+                                                      reinterpret_cast<const UniChar*>(nativeCharsIgnoringModifiers),
>+                                                      keyEvent->mNativeCharactersIgnoringModifiers.Length());
>+      }

I assume that these lines are just indented.

>@@ -1914,72 +1925,53 @@ nsEventStatus nsPluginInstanceOwner::Pro
>     mLastContentFocused = (anEvent.message == NS_FOCUS_CONTENT);
>     mShouldBlurOnActivate = false;
>     PerformDelayedBlurs();
>   }
> 
>   NPCocoaEvent cocoaEvent = TranslateToNPCocoaEvent(const_cast<WidgetGUIEvent*>(&anEvent), mPluginFrame);
>   if (cocoaEvent.type == (NPCocoaEventType)0) {
>     return nsEventStatus_eIgnore;
>   }
> 
>-  if (cocoaEvent.type == NPCocoaEventKeyDown) {
>-    ComplexTextInputPanel* ctiPanel = ComplexTextInputPanel::GetSharedComplexTextInputPanel();
>-    if (ctiPanel && ctiPanel->IsInComposition()) {
>+  if (cocoaEvent.type == NPCocoaEventTextInput) {
>+    mInstance->HandleEvent(&cocoaEvent, nullptr);
>+    return nsEventStatus_eConsumeNoDefault;
>+  }
>+
>+  int16_t response = kNPEventNotHandled;
>+  mInstance->HandleEvent(&cocoaEvent,
>+                         &response,
>+                         NS_PLUGIN_CALL_SAFE_TO_REENTER_GECKO);
>+  if ((response == kNPEventStartIME) && (cocoaEvent.type == NPCocoaEventKeyDown)) {
>+    nsIWidget *widget = mPluginFrame->GetNearestWidget();

nit: nsIWidget* widget =

>+    if (widget) {

Cannot you to return earlier if !widget? (if you need to copy some lines from below, keep this style.)

>+      WidgetKeyboardEvent* keyEvent = (WidgetKeyboardEvent*) anEvent.AsKeyboardEvent();

AsKeyboardEvent() should return WidgetKeyboardEvent*. Why do you need this case?

>diff --git a/widget/TextEvents.h b/widget/TextEvents.h
>   // after preventDefault is called on keydown events. It's ok if this wraps
>   // over long periods.
>   uint32_t mUniqueId;
> 
> #ifdef XP_MACOSX
>   // Values given by a native NSEvent, for use with Cocoa NPAPI plugins.
>   uint16_t mNativeKeyCode;
>   uint32_t mNativeModifierFlags;
>   nsString mNativeCharacters;
>   nsString mNativeCharactersIgnoringModifiers;
>+  // If this is non-empty, create a text event for plugins instead of a
>+  // keyboard event.
>+  nsString mPluginTextEventString;
> #endif

I wonder, don't you need to modify AssignKeyboardEventData()? (Could be no, perhaps, though)

>diff --git a/widget/cocoa/TextInputHandler.mm b/widget/cocoa/TextInputHandler.mm
>@@ -1516,21 +1516,39 @@ TextInputHandler::HandleKeyDownEvent(NSE
>      this, aNativeEvent, GetNativeKeyEventType(aNativeEvent),
>      [aNativeEvent keyCode], [aNativeEvent keyCode],
>      [aNativeEvent modifierFlags], GetCharacters([aNativeEvent characters]),
>      GetCharacters([aNativeEvent charactersIgnoringModifiers])));
> 
>   nsRefPtr<nsChildView> kungFuDeathGrip(mWidget);
> 
>   KeyEventState* currentKeyEvent = PushKeyEvent(aNativeEvent);
>   AutoKeyEventStateCleaner remover(this);
> 
>-  if (!IsIMEComposing()) {
>+  ComplexTextInputPanel* ctiPanel = ComplexTextInputPanel::GetSharedComplexTextInputPanel();
>+  if (ctiPanel && ctiPanel->IsInComposition()) {
>+    nsAutoString committed;
>+    ctiPanel->InterpretKeyEvent(aNativeEvent, committed);
>+    if (!committed.IsEmpty()) {
>+      CFStringRef cfString =
>+        CFStringCreateWithCharacters(NULL,
>+                                     reinterpret_cast<const UniChar*>(committed.get()),
>+                                     committed.Length());
>+      WidgetKeyboardEvent imeEvent(true, NS_KEY_DOWN, mWidget);
>+      InitKeyEvent(aNativeEvent, imeEvent);
>+      imeEvent.mPluginTextEventString.Assign(committed);
>+      DispatchEvent(imeEvent);
>+      CFRelease(cfString);

Shouldn't you release cfString before DispatchEvent()?

>@@ -1548,20 +1566,24 @@ TextInputHandler::HandleKeyDownEvent(NSE
>     }
> 
>     if (currentKeyEvent->IsDefaultPrevented()) {
>       PR_LOG(gLog, PR_LOG_ALWAYS,
>         ("%p TextInputHandler::HandleKeyDownEvent, "
>          "keydown event's default is prevented", this));
>       return true;
>     }
>   }
> 
>+  if (mWidget->IsPluginFocused()) {
>+    return true;
>+  }

Could you add comment the reason why when plugin has focus, we don't need to call interpretKeyEvents?

>+NS_IMETHODIMP
>+nsChildView::StartPluginIME(mozilla::WidgetKeyboardEvent& aKeyboardEvent,
>+                            int32_t aPanelX, int32_t aPanelY,
>+                            nsString& aCommitted)
>+{
>+  NS_ENSURE_TRUE(mView, NS_ERROR_NOT_AVAILABLE);
>+
>+  ComplexTextInputPanel* ctiPanel = ComplexTextInputPanel::GetSharedComplexTextInputPanel();

nit: Looks like over 80 characters.

>+
>+  ctiPanel->PlacePanel(aPanelX, aPanelY);
>+  ctiPanel->InterpretKeyEvent([mView lastKeyDownEvent], aCommitted);
>+
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsChildView::SetPluginFocused(bool& aFocused)
>+{
>+  if (aFocused != mPluginFocused) {

Could you use early return style here?

>+// ComplexTextInputPanel's interpretKeyEvent hack won't work without this.
>+// It makes calls to +[NSTextInputContext currentContext], deep in system
>+// code, return the appropriate context.
>+- (NSTextInputContext *)inputContext
>+{
>+  NSTextInputContext* pluginContext = NULL;
>+  if (mGeckoChild && mGeckoChild->IsPluginFocused()) {
>+    ComplexTextInputPanel* ctiPanel =
>+      ComplexTextInputPanel::GetSharedComplexTextInputPanel();
>+    if (ctiPanel) {
>+      pluginContext = (NSTextInputContext*) ctiPanel->GetInputContext();

Why do you need the cast here?



I wonder you cache last keydown event. However, isn't it possible to be nested by IME or something? Cannot you use GetCurrentKeyEvent() of TextInputHanlder?

I'll keep r? until I finish to test this patch.

Comment 15

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

And do you want to land this before next merge?

Comment 16

[Steven Michaud [:smichaud] (Retired)]

> And do you want to land this before next merge?

Yes, I do.

Thanks for your review.  I'll respond to your comments and revise my patch.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2

Here's my revised patch.  I followed most of your suggestions,
Masayuki.  I also put #ifdef XP_MACOSX blocks around the code I added
to dom/html -- it seemed kind of silly to add non-functional code to
other platforms.  And I changed the declarations of
nsIWidget::StartPluginIME() and friends to make aKeyboardEvent const.

> AsKeyboardEvent() should return WidgetKeyboardEvent*. Why do you
> need this case?

It was to force the return value from const to non-const.  But with
the changes to StartPluginIME() I mentioned above, I no longer need
this.

> I wonder, don't you need to modify AssignKeyboardEventData()?

I don't think it's absolutely necessary -- Josh's patch for bug
1092630 also missed this, and there don't seem to have been any ill
effects.  But it's still probably a good idea, and I've done it.

> Shouldn't you release cfString before DispatchEvent()?

Actually that code slipped in from my debug build.  I've removed it.

> I wonder you cache last keydown event. However, isn't it possible to
> be nested by IME or something? Cannot you use GetCurrentKeyEvent()
> of TextInputHanlder?

I tried to do this at first, but then found that it doesn't work in
e10s mode (TextInputHandler::HandleKeyDownEvent() has already returned
by the time nsChildView::StartPluginIME() is called, so
GetCurrentKeyEvent() returns nothing).  I could try to hack around
this.  But at least one additional IPC call would be needed, and in
any case I don't think nsChildView::StartPluginIME() will ever be
called as a result of nested IME -- it's only ever called to start a
new IME session when none currently exists.

I've added a comment to this effect.

What do you think?  Am I right?

The tryservers are down, so I haven't (yet) started another set of
builds.  I'll do so later.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8566707 [details] [diff] [review]
Fix rev2

Smaug, could you take a look at the changes I've made to dom/html?

In particular I wonder whether it's right to use nsIDOMEventTarget::PostHandleEvent(), and whether I've used it correctly.  I found other uses of PostHandleEvent() in dom/html, and I tried to imitate them as closely as possible.  But maybe I should have used nsIDOMEventListener::HandleEvent(nsIDOMEvent*) and nsIDOMEventTarget::AddSystemEventListener().

As best I can tell, I *don't* want to use nsIDOMEventTarget::PreHandleEvent().

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8566707 [details] [diff] [review]
Fix rev2

I've started another set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95151c13ac66

Comment 20

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

Comment on attachment 8566707 [details] [diff] [review]
Fix rev2

>+HTMLObjectElement::PostHandleEvent(EventChainPostVisitor& aVisitor)
>+{
>+  switch (aVisitor.mEvent->message) {
>+    case NS_FOCUS_CONTENT: {
>+      OnPluginFocus(this);
>+      break;
>+    }
>+    case NS_BLUR_CONTENT: {
>+      OnPluginBlur(this);
>+      break;
>+    }
>+  }
I would have some helper method, like FocusBlurPlugin(WidgetEvent*)
which first checks the event is trusted
(event->mFlags.mIsTrusted)
and then does this switch-case.
And call that method both from HTMLObjectElement::PostHandleEvent and 
HTMLSharedObjectElement::PostHandleEvent


Didn't look at the rest of the code.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev3

This revision follows Smaug's suggestion and does some extra cleanup.

Comment 22

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

Comment on attachment 8566804 [details] [diff] [review]
Fix rev3

(In reply to Steven Michaud from comment #17)
> > AsKeyboardEvent() should return WidgetKeyboardEvent*. Why do you
> > need this case?
> 
> It was to force the return value from const to non-const.  But with
> the changes to StartPluginIME() I mentioned above, I no longer need
> this.

I see but anyway, in such cases, please use const_cast for making it clearer.

> > I wonder, don't you need to modify AssignKeyboardEventData()?
> > I wonder you cache last keydown event. However, isn't it possible to
> > be nested by IME or something? Cannot you use GetCurrentKeyEvent()
> > of TextInputHanlder?
> 
> I tried to do this at first, but then found that it doesn't work in
> e10s mode (TextInputHandler::HandleKeyDownEvent() has already returned
> by the time nsChildView::StartPluginIME() is called, so
> GetCurrentKeyEvent() returns nothing).  I could try to hack around
> this.  But at least one additional IPC call would be needed, and in
> any case I don't think nsChildView::StartPluginIME() will ever be
> called as a result of nested IME -- it's only ever called to start a
> new IME session when none currently exists.

Ah, I see. I'll keep this issue in my mind. e10s mode might break the bug which was fixed by the mechanism.


I've tested the tryserver build in comment 19, roughly. (both e10s and non-e10s mode on Mac OS X 10.6 - 10.10) It works fine to me.

Comment 23

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

Anyway, it seems that this is very important. I cannot use IME on flush on Nightly on Mac OS X 10.6 and 10.7 without e10s but this patch fixes the bug.

Comment 24

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

Wait, did we regress plugin handling in non-e10s builds too? Should we backout the regressing patch from FF37 or will this be safe enough?

Comment 25

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

(In reply to Olli Pettay [:smaug] (no new review requests before Feb 22, please) from comment #24)
> Wait, did we regress plugin handling in non-e10s builds too? Should we
> backout the regressing patch from FF37 or will this be safe enough?

I'm not sure the regression point. But I'll check it with 37 tomorrow or next Monday.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Masayuki and Smaug, see bug 1110884.  It's Josh's patch for bug 1092630 that caused that bug, but it would be an *enormous* pain to back it out now.  We now have a good fix for this bug, which also (of course) fixes 1110884.  So fixing this bug is a much better way forward.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

I'm going to try to get this patch onto the 37 branch as soon as possible.  I think the risk in non-e10s mode is quite low.  The risk in e10s mode is a bit higher (I'd say "moderate"), but e10s mode isn't (yet) following the trains, so (for now) we only need to worry about it on the trunk.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8566804 [details] [diff] [review]
Fix rev3

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc5168c2922d

Comment 29

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8566804 [details] [diff] [review]
Fix rev3

Approval Request Comment
[Feature/regressing bug #]: 1092630
[User impact if declined]: IME won't work on OS X 10.7 and below, even in non-e10s mode
[Describe test coverage new/current, TreeHerder]: Manual tests by myself and Masayuki
[Risks and why]: Low.  The risks are slightly higher in e10s mode, but currently that's only on trunk.  On other branches this patch's new code will only run in non-e10s mode.
[String/UUID change made/needed]: none

It will be *much* better to fix bug 1110884 by landing this patch than by backing out the patch for bug 1092630 (which caused bug 1110884).  The patch for bug 1092630 made huge changes, many of which were significant improvements.  It also caused a number of regressions, but (if you include this patch) the most serious of them have now been fixed.  And a lot of other changes have since landed which depend on bug 1092630.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #29)

> [Describe test coverage new/current, TreeHerder]: Manual tests by myself and Masayuki

I forgot to mention that, besides my tests on mozilla-central, I also tested on aurora.  Everything went fine.

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8566804 [details] [diff] [review]
Fix rev3

This is a large, but expected change, for a noticeable regression on OSX. Although this hasn't yet landed on m-c, Steven has tested against m-c and m-a. We'll take this on Aurora now before the merge to Beta to get additional testing over the weekend. Aurora+

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8566804 [details] [diff] [review]
Fix rev3

Landed on mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e7abc21fea7

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c007a1fc61a