1208944 - windowless Flash should handle IME composition

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Makoto Kato [:m_kato]]

This is long time bug after out-of-process plugin.   When using windowless Flash, IME composition isn't handed by Flash correctly.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: WIP

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Part 1. Dispatch WidgetPluginEvent to content process on e10s

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Part 2. Allow IME window message to plugin process

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Part 3. Hook IMM32 APIs on plugin process to access IME data on chrome process

Comment 5

[Makoto Kato [:m_kato]]

TODO
- Add injection code to hook IMM API correctly. 
- This will crash on few mochitest.  Now I am investigating it.

Comment 6

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

Comment on attachment 8667697 [details] [diff] [review]
Part 1.  Dispatch WidgetPluginEvent to content process on e10s

>+#if defined(XP_WIN)
>+  case ePluginEventClass:
>+    {
>+      const NPEvent* pluginEvent =
>+        static_cast<const NPEvent*>(aEvent.AsPluginEvent()->mPluginEvent);
>+      return widget::WinUtils::IsWindowMessageForPluginEvent(
>+               pluginEvent->event);
>+    }
>+#endif

Instead of using #ifdef XP_WIN, how about to add a bool field to WidgetPluginEvent? Like mIsCrossProcessSafe?

Comment 7

[Makoto Kato [:m_kato]]

Attachment: Part 4. Analyze je opcode to hook Imm API on x64

WIP part 4.

Comment 8

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

Comment on attachment 8667699 [details] [diff] [review]
Part 3. Hook IMM32 APIs on plugin process to access IME data on chrome process

>+bool
>+nsPluginInstanceOwner::GetCompositionString(uint32_t aType, nsString* aString)
>+{
>+  if (!mPluginFrame) {
>+    NS_WARNING("Plugin owner has no plugin frame.");
>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIWidget> widget = GetContainingWidgetIfOffset();
>+  if (!widget) {
>+    widget = GetRootWidgetForPluginFrame(mPluginFrame);
>+    if (!widget) {
>+      return false;
>+    }
>+  }
>+
>+  widget->GetCompositionStringForPlugin(aType, aString);
>+  return true;
>+}

I wonder, cannot we create TextComposition instance from WidgetPluginEvent? E.g., adding new event message like ePluginCompositionEvent and adding new files mData and mRanges. Then, we can store the composition string information in the child process.


And are you trying to hook only W APIs? If so, I guess that some Chinese IMEs won't work well on Flash Player. See here:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/IMMHandler.cpp?rev=4d1821f24254&mark=1398-1401,1409-1412,1427-1443#1387

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 10/31 - 11/3, JST) from comment #8)
> Comment on attachment 8667699 [details] [diff] [review]

> I wonder, cannot we create TextComposition instance from WidgetPluginEvent?
> E.g., adding new event message like ePluginCompositionEvent and adding new
> files mData and mRanges. Then, we can store the composition string
> information in the child process.

Although I don't check aType that using Flash, If we use TextComposition, we re-calculate / re-generate strings by requested aType.

> And are you trying to hook only W APIs? If so, I guess that some Chinese
> IMEs won't work well on Flash Player. See here:
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/IMMHandler.
> cpp?rev=4d1821f24254&mark=1398-1401,1409-1412,1427-1443#1387

They use ImmGetCompositionStringW directly.  So even if hooking *A, it isn't used.

Comment 10

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

(In reply to Makoto Kato [:m_kato] from comment #9)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 10/31 -
> 11/3, JST) from comment #8)
> > Comment on attachment 8667699 [details] [diff] [review]
> 
> > I wonder, cannot we create TextComposition instance from WidgetPluginEvent?
> > E.g., adding new event message like ePluginCompositionEvent and adding new
> > files mData and mRanges. Then, we can store the composition string
> > information in the child process.
> 
> Although I don't check aType that using Flash, If we use TextComposition, we
> re-calculate / re-generate strings by requested aType.

Yeah, but I guess that it's more reasonable than using IPC.

> > And are you trying to hook only W APIs? If so, I guess that some Chinese
> > IMEs won't work well on Flash Player. See here:
> > http://mxr.mozilla.org/mozilla-central/source/widget/windows/IMMHandler.
> > cpp?rev=4d1821f24254&mark=1398-1401,1409-1412,1427-1443#1387
> 
> They use ImmGetCompositionStringW directly.  So even if hooking *A, it isn't
> used.

Oh, really? Did you see it from the binary? or tested manually? If latter, I think that we should ask the API list used by Flash to Adobe.

Comment 11

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 10/31 - 11/3, JST) from comment #10)
> (In reply to Makoto Kato [:m_kato] from comment #9)
> > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 10/31 -
> > 11/3, JST) from comment #8)
> > > Comment on attachment 8667699 [details] [diff] [review]
> > 
> > > I wonder, cannot we create TextComposition instance from WidgetPluginEvent?
> > > E.g., adding new event message like ePluginCompositionEvent and adding new
> > > files mData and mRanges. Then, we can store the composition string
> > > information in the child process.
> > 
> > Although I don't check aType that using Flash, If we use TextComposition, we
> > re-calculate / re-generate strings by requested aType.
> 
> Yeah, but I guess that it's more reasonable than using IPC.
> 
> > > And are you trying to hook only W APIs? If so, I guess that some Chinese
> > > IMEs won't work well on Flash Player. See here:
> > > http://mxr.mozilla.org/mozilla-central/source/widget/windows/IMMHandler.
> > > cpp?rev=4d1821f24254&mark=1398-1401,1409-1412,1427-1443#1387
> > 
> > They use ImmGetCompositionStringW directly.  So even if hooking *A, it isn't
> > used.
> 
> Oh, really? Did you see it from the binary? or tested manually? If latter, I
> think that we should ask the API list used by Flash to Adobe.

dumped.  Flash will use the following IMM API by dynamic load.

ImmGetProperty
ImmIsIME
ImmSetConversionStatus
ImmGetConversionStatus
ImmAssociateContextEx
ImmNotifyIME
ImmSetOpenStatus
ImmGetOpenStatus
ImmSetCandidateWindow
ImmSetCompositionStringW
ImmGetCompositionStringW
ImmSetCompositionWindow
ImmGetCompositionFontW
ImmSetCompositionFontW
ImmReleaseContext
ImmGetContext 

To support inline composition, we should hook ImmReleaseContext, ImmGetContext, ImmGetCompositionStringW and ImmSetCandidateWindow at least.

Also, Chrome that NPAPI was supported hooks these 4 APIs only too.

Comment 12

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 10/31 - 11/3, JST) from comment #10)
> (In reply to Makoto Kato [:m_kato] from comment #9)
> > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 10/31 -
> > 11/3, JST) from comment #8)
> > > Comment on attachment 8667699 [details] [diff] [review]
> > 
> > > I wonder, cannot we create TextComposition instance from WidgetPluginEvent?
> > > E.g., adding new event message like ePluginCompositionEvent and adding new
> > > files mData and mRanges. Then, we can store the composition string
> > > information in the child process.
> > 
> > Although I don't check aType that using Flash, If we use TextComposition, we
> > re-calculate / re-generate strings by requested aType.
> 
> Yeah, but I guess that it's more reasonable than using IPC.

Do you mean that TextComposition should be created on dom/plugin/ipc/ (PluginInstanceParent.cpp) ?

But at least, setting candidate window has to use IPC on main process.

Comment 13

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

(In reply to Makoto Kato [:m_kato] from comment #12)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 10/31 -
> 11/3, JST) from comment #10)
> > (In reply to Makoto Kato [:m_kato] from comment #9)
> > > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 10/31 -
> > > 11/3, JST) from comment #8)
> > > > Comment on attachment 8667699 [details] [diff] [review]
> > > 
> > > > I wonder, cannot we create TextComposition instance from WidgetPluginEvent?
> > > > E.g., adding new event message like ePluginCompositionEvent and adding new
> > > > files mData and mRanges. Then, we can store the composition string
> > > > information in the child process.
> > > 
> > > Although I don't check aType that using Flash, If we use TextComposition, we
> > > re-calculate / re-generate strings by requested aType.
> > 
> > Yeah, but I guess that it's more reasonable than using IPC.
> 
> Do you mean that TextComposition should be created on dom/plugin/ipc/
> (PluginInstanceParent.cpp) ?

I meant that WidgetPluginEvent should cause creating an instance of TextComposition in dom/events.

> But at least, setting candidate window has to use IPC on main process.

Yeah, setting something needs IPC, anyway. I'm afraid that in e10s mode, 2 IPCs are necessary per call of IMM API in windowless plugin. This must make damage to the performance.

Comment 14

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

Hi guys, I wanted to see how things are going here. It looks like the patches are pretty far along.

David is getting closer to finishing bug 1217665, which would make windowless plugins perform as well as windowed plugins. After that, IME will be the only thing stopping us from switching Flash to be entirely windowless.

Also, I wanted to point out that we have a direct link from the plugin process to the chrome process: gChromeInstance. It can only be used for asynchronous messages, but it might be useful for SetCandidateWindowForPlugin. That would avoid one IPC operation.

Comment 15

[Makoto Kato [:m_kato]]

current status:
I am rewriting composition handling to use TextComposition object.  Most codes are completed, so I can send review for this.

Comment 16

[Makoto Kato [:m_kato]]

(In reply to Bill McCloskey (:billm) from comment #14)
> Hi guys, I wanted to see how things are going here. It looks like the
> patches are pretty far along.
> 
> David is getting closer to finishing bug 1217665, which would make
> windowless plugins perform as well as windowed plugins. After that, IME will
> be the only thing stopping us from switching Flash to be entirely windowless.

Even if this is fixed, if Flash runs on their protected mode, we cannot hook IME.  Because Flash calls IME APIs on their protected mode's process.  If they call IME APIs on DLL (on our process), we can hook it.

Of course, they abandon protected mode for Flash, this will work.

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Part 1. Dispatch CompositionEvent to plugin

Comment 18

[Makoto Kato [:m_kato]]

Attachment: Part 2. Handle CompositionEvent on plugin

Comment 19

[Makoto Kato [:m_kato]]

Attachment: Part 3. Allow IME message for NPRemoteEvent

Comment 20

[Makoto Kato [:m_kato]]

Attachment: Part 4. nsWindowsDllInterceptor should supports some IMM APIs hook

Comment 21

[Makoto Kato [:m_kato]]

Attachment: Part 5. Send PluginEvent to content process

Comment 22

[Makoto Kato [:m_kato]]

I don't add part 6 patch yet.  So these patches aren't reviewable.

Comment 23

[Makoto Kato [:m_kato]]

Attachment: Part 6. get valid TextRangeArray during compositionupdate

Comment 24

[Makoto Kato [:m_kato]]

Attachment: Part 1. Dispatch CompositionEvent to plugin

Comment 25

[Makoto Kato [:m_kato]]

Attachment: Part 2. Handle CompositionEvent on plugin

Comment 26

[Makoto Kato [:m_kato]]

Attachment: Part 7. Don't send WM_IME_REQUEST to plugin

Comment 27

[Makoto Kato [:m_kato]]

Attachment: Part 8. Don't get selection when plugin has focus

Comment 28

[Makoto Kato [:m_kato]]

Attachment: Part 9. Hook IMM32 APIs on plugin process

Comment 29

[Makoto Kato [:m_kato]]

Attachment: Part 10. Call CallWindowProc when event isn't handled by plugin

Comment 30

[Makoto Kato [:m_kato]]

Attachment: Part 11. Add test

Comment 31

[Makoto Kato [:m_kato]]

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

Comment 32

[Makoto Kato [:m_kato]]

Attachment: Part 1. Dispatch CompositionEvent to plugin

Now, we don't dispatch composition event to plugin from widget.  To use TextComposition on plugin instance, we will dispatch it.

Comment 33

[Makoto Kato [:m_kato]]

Attachment: Part 2. Handle CompositionEvent on plugin

TextComposition sometimes re-generate composition event, so when generating or cloning it, add NPEvent for Windows plugin.

Also nsPluginInstance should recognize composition event and send it to plugin.

Comment 34

[Makoto Kato [:m_kato]]

Attachment: Part 3. Allow IME message for NPRemoteEvent

Add safe window message for IME to use IPC

Comment 35

[Makoto Kato [:m_kato]]

Attachment: Part 5. Send PluginEvent to content process

When using e10s, Chrome process doesn't send PluginEvent to content process.  We should send it.

Comment 36

[Makoto Kato [:m_kato]]

Attachment: Part 9. Hook IMM32 APIs on plugin process

Flash x64 doesn't have protected mode, Flash will call Imm API on our plugin process.  So to emulate ImmGetCompositionString using TextComposition, it can return correct string.

Also this code introduces QUIRK_WINLESS_HOOK_IME whether hook APIs and message.
If QUIRK_WINLESS_HOOK_IME isn't set, we don't send WM_IME_*COMPOSITION to plugin.

Flash x86 eats WM_IME_*COMPOSITION, but they doesn't call Imm API on our process.  So I don't set QUIRK_WINLESS_HOOK_IME yet.

Comment 37

[Makoto Kato [:m_kato]]

Attachment: Part 10. Call CallWindowProc when event isn't handled by plugin

When windowless plugin doesn't handle WM_IME_*COMPOSITION message, we have to call DefWindowProc to use on-the-spot input.

Instead of calling DefWindowProc with GCS_RESULTSTR, I call WM_CHAR.  Because, when using e10s, Parent to content is async, so GCS_RESULTSTR is too late, then it may generate unnecessary WM_CHAR such as using Korean IME.  So to optimize, I generate WM_CHAR by GCS_RESULTSTR.

Comment 38

[Makoto Kato [:m_kato]]

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

Comment 39

[Makoto Kato [:m_kato]]

Comment on attachment 8694558 [details] [diff] [review]
Part 1. Dispatch CompositionEvent to plugin

Currently, we dispatch PluginWidgetEvent when plugin has focus.  To use TextComposition on plugin instance, dispatch composition event from widget instead.

Comment 40

[Makoto Kato [:m_kato]]

Comment on attachment 8694559 [details] [diff] [review]
Part 2. Handle CompositionEvent on plugin

TextComposition will generate composition event depends on composition status.  So add NPEvent for it.

Nakano-san, should I add it on TextComposition only?  TextComposition calls EventDispatcher::Dispatch at some code.  So to simply, for commit and composition, I add it on TextComposition and for start/end, it adds nsPluginInstanceOnwer.

Comment 41

[Makoto Kato [:m_kato]]

Comment on attachment 8694560 [details] [diff] [review]
Part 3. Allow IME message for NPRemoteEvent

Add some WM_IME_* message to use NPRemoteEvent

Comment 42

[Makoto Kato [:m_kato]]

Comment on attachment 8691258 [details] [diff] [review]
Part 4. nsWindowsDllInterceptor should supports some IMM APIs hook

:dmajor leaves mozilla, so I don't know the right reviewer of DLL hook.

ImmGetCompositionStringW uses je opcode into first 14 bytes.  So, to hook it, I need analyze it.

At least, we need hook ImmGetContext, ImmGetCompsoitionStringW and ImmSetCandidateWindow for IME support on windowless plugin.  But ImmReleaseContext is unnecessary because all OS implementation of it is that just return TRUE.

Comment 43

[Makoto Kato [:m_kato]]

Comment on attachment 8694561 [details] [diff] [review]
Part 5. Send PluginEvent to content process

PluginWidgetEvent doesn't mark as safe for cross process.  So I want to post it to content process.

Comment 44

[Makoto Kato [:m_kato]]

Comment on attachment 8692363 [details] [diff] [review]
Part 7. Don't send WM_IME_REQUEST to plugin

The params of WM_IME_REQUEST have pointer, So we should send it to plugin because plugin is always OOP.

Comment 45

[Makoto Kato [:m_kato]]

Comment on attachment 8692364 [details] [diff] [review]
Part 8. Don't get selection when plugin has focus

When plugin has focus, ContentCache won't work because selection request isn't completed.  So I set some member to 0 when plugin has focus.

Comment 46

[Makoto Kato [:m_kato]]

Comment on attachment 8694564 [details] [diff] [review]
Part 9. Hook IMM32 APIs on plugin process

Flash x64 with windowless mode calls Imm API on our plugin process.  So I hook APIs and emulate it by TextComposition.

When calling ImmGetContext, HWND class is SWFlash_PlaceholderX.  If so, I hook APIs.

Also, this code isn't turned on Flash x86.  Because it consumes WM_IME_COMPOSITION, but they call Imm API on their protected mode process.  (We already reports to Adobe)

Comment 47

[Makoto Kato [:m_kato]]

Comment on attachment 8691772 [details] [diff] [review]
Part 6. get valid TextRangeArray during compositionupdate

I would like to access RangeArray to emulate COMPATTR.  But during composition event, we cannot get it correctly.

Comment 48

[Makoto Kato [:m_kato]]

Comment on attachment 8694566 [details] [diff] [review]
Part 10. Call CallWindowProc when event isn't handled by plugin

If plugin doesn't handle IME message, we should call DefWindowProc (call next window proc).

And when WM_IME_COMPOSITION with GCS_RESULTSTR, I will emulate it, then use WM_CHAR.  Because, when e10s, message order is changed.  So when using Microsoft Korean IME, it generates duplicated WM_CHAR.

Comment 49

[Makoto Kato [:m_kato]]

Comment on attachment 8692795 [details] [diff] [review]
Part 11. Add test

Add test for windowless plugin

Comment 50

[Makoto Kato [:m_kato]]

Attachment: Part 12. Remove unused code into widget/windows

mIsComposingOnPlugin is unnecessary now.

Comment 51

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

Comment on attachment 8694558 [details] [diff] [review]
Part 1. Dispatch CompositionEvent to plugin

Looks like that you're removing mIsComposingOnPlugin, but I don't see it's actually dropped from the header file.

> -  if (!mIsComposingOnPlugin &&
> -      aWritingMode.IsVertical() && IsVerticalWritingSupported()) {
> +  if (aWritingMode.IsVertical() && IsVerticalWritingSupported()) {

I wonder, do you need this change? I assumed that Flash Player doesn't support vertical writing. Therefore, even if the <object> or <embed>'s writing-mode is specified as vertical, we need to treat it as horizontal layout.

Comment 52

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

And you're adding wParam and lParam to DispatchCompositionChangeEvent(), but looks like that they are not used. Why?

Comment 53

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

Comment on attachment 8694559 [details] [diff] [review]
Part 2. Handle CompositionEvent on plugin

>@@ -118,16 +122,42 @@ TextComposition::CloneAndDispatchAs(
>   compositionEvent.mFlags.mIsSynthesizedForTests =
>     aCompositionEvent->mFlags.mIsSynthesizedForTests;
> 
>   nsEventStatus dummyStatus = nsEventStatus_eConsumeNoDefault;
>   nsEventStatus* status = aStatus ? aStatus : &dummyStatus;
>   if (aMessage == eCompositionUpdate) {
>     mLastData = compositionEvent.mData;
>   }
>+#ifdef XP_WIN
>+  NPEvent newEvent;
>+  if (aMessage == eCompositionUpdate) {
>+    // Original may be eCompositionCommit, but this update shouldn't have commit
>+    // lParam.
>+    newEvent.event = WM_IME_COMPOSITION;
>+    newEvent.wParam = 0;
>+    newEvent.lParam = GCS_COMPSTR | GCS_COMPATTR | GCS_COMPCLAUSE |
>+                      GCS_CURSORPOS;
>+    compositionEvent.mPluginEvent.Copy(newEvent);
>+  } else if (aMessage == eCompositionChange) {
>+    newEvent.event = WM_IME_COMPOSITION;
>+    newEvent.wParam = 0;
>+    if (aCompositionEvent->mMessage == eCompositionCommit ||
>+        aCompositionEvent->mMessage == eCompositionCommitAsIs) {
>+      newEvent.lParam = GCS_RESULTSTR | GCS_CURSORPOS;
>+    } else {
>+      const NPEvent* pPluginEvent =
>+        static_cast<const NPEvent*>(aCompositionEvent->mPluginEvent);
>+      compositionEvent.mPluginEvent.Copy(*pPluginEvent);
>+      newEvent.lParam = GCS_COMPSTR | GCS_COMPATTR | GCS_COMPCLAUSE |
>+                        GCS_CURSORPOS;
>+     }

nit: wrong indent

>+    compositionEvent.mPluginEvent.Copy(newEvent);
>+  }
>+#endif

I don't like you to add #ifdef XP_* into TextComposition.cpp. Cannot you create a static method like nsPluginInstanceOwner::GeneratePluginEvent(WidgetCompositionEvent& aCompositionEvent, TextComposition* aComposition) and call it on all platforms but implemented only on Windows?

> nsresult
>+nsPluginInstanceOwner::DispatchCompositionToPlugin(nsIDOMEvent* aEvent)
>+{
>+#ifdef XP_WIN
>+  if (!mPluginWindow || (mPluginWindow->type == NPWindowTypeWindow)) {
>+    return aEvent->PreventDefault(); // consume event
>+  }
>+  WidgetCompositionEvent* compositionEvent =
>+    aEvent->GetInternalNSEvent()->AsCompositionEvent();
>+  if (compositionEvent) {
>+    // TextComposition won't generate NPEvent by compositionstart/end
>+    if (compositionEvent->mMessage == eCompositionStart) {
>+      NPEvent event;
>+      event.event = WM_IME_STARTCOMPOSITION;
>+      event.wParam = 0;
>+      event.lParam = 0;
>+      compositionEvent->mPluginEvent.Copy(event);
>+    } else if (compositionEvent->mMessage == eCompositionEnd) {
>+      NPEvent event;
>+      event.event = WM_IME_ENDCOMPOSITION;
>+      event.wParam = 0;
>+      event.lParam = 0;
>+      compositionEvent->mPluginEvent.Copy(event);
>+    }

Then, these blocks should be in the new static method.

>@@ -1621,16 +1654,22 @@ nsPluginInstanceOwner::HandleEvent(nsIDO
>   }
>   if (eventType.EqualsLiteral("keydown") ||
>       eventType.EqualsLiteral("keyup")) {
>     return DispatchKeyToPlugin(aEvent);
>   }
>   if (eventType.EqualsLiteral("keypress")) {
>     return ProcessKeyPress(aEvent);
>   }
>+  if (eventType.EqualsLiteral("compositionstart") ||
>+      eventType.EqualsLiteral("compositionupdate") ||
>+      eventType.EqualsLiteral("compositionend") ||
>+      eventType.EqualsLiteral("text")) {
>+    return DispatchCompositionToPlugin(aEvent);

I guess that this should work fine even if the windowless plugin element is in an HTML editor.

>@@ -2447,16 +2486,22 @@ nsPluginInstanceOwner::Destroy()
>   content->RemoveEventListener(NS_LITERAL_STRING("drag"), this, true);
>   content->RemoveEventListener(NS_LITERAL_STRING("dragenter"), this, true);
>   content->RemoveEventListener(NS_LITERAL_STRING("dragover"), this, true);
>   content->RemoveEventListener(NS_LITERAL_STRING("dragleave"), this, true);
>   content->RemoveEventListener(NS_LITERAL_STRING("dragexit"), this, true);
>   content->RemoveEventListener(NS_LITERAL_STRING("dragstart"), this, true);
>   content->RemoveEventListener(NS_LITERAL_STRING("draggesture"), this, true);
>   content->RemoveEventListener(NS_LITERAL_STRING("dragend"), this, true);
>+  content->RemoveEventListener(NS_LITERAL_STRING("compositionstart"), this,
>+                               true);
>+  content->RemoveEventListener(NS_LITERAL_STRING("compositionupdate"), this,
>+                               true);
>+  content->RemoveEventListener(NS_LITERAL_STRING("compositionend"), this, true);
>+  content->RemoveEventListener(NS_LITERAL_STRING("text"), this, true);

Please use content->RemoveSystemEventListener().

>@@ -2842,16 +2887,20 @@ nsresult nsPluginInstanceOwner::Init(nsI
>   aContent->AddEventListener(NS_LITERAL_STRING("drag"), this, true);
>   aContent->AddEventListener(NS_LITERAL_STRING("dragenter"), this, true);
>   aContent->AddEventListener(NS_LITERAL_STRING("dragover"), this, true);
>   aContent->AddEventListener(NS_LITERAL_STRING("dragleave"), this, true);
>   aContent->AddEventListener(NS_LITERAL_STRING("dragexit"), this, true);
>   aContent->AddEventListener(NS_LITERAL_STRING("dragstart"), this, true);
>   aContent->AddEventListener(NS_LITERAL_STRING("draggesture"), this, true);
>   aContent->AddEventListener(NS_LITERAL_STRING("dragend"), this, true);
>+  aContent->AddEventListener(NS_LITERAL_STRING("compositionstart"), this, true);
>+  aContent->AddEventListener(NS_LITERAL_STRING("compositionupdate"), this, true);
>+  aContent->AddEventListener(NS_LITERAL_STRING("compositionend"), this, true);
>+  aContent->AddEventListener(NS_LITERAL_STRING("text"), this, true);

Please use content->AddSystemEventListener() and don't accept untrusted event.

If you use normal event group listeners, the events may not be reached to the element since web contents can do Event.stopPropagation(). I believe that IME composition should be a default action of composition events. And composition events shouldn't be cancellable.

And I think that you need to create TextComposition::CompositionChangeEventHandlingMarker() in nsPluginInstanceOwner::DispatchCompositionToPlugin(). It manages some stateful members of TextComposition.

Comment 54

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

Comment on attachment 8692364 [details] [diff] [review]
Part 8. Don't get selection when plugin has focus

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -1798,16 +1798,21 @@ public:
>      * keyboard focus.  Should be sent when the keyboard focus changes too or
>      * from a plugin.
>      *
>      * aFocused  Whether or not a plugin is focused
>      */
>     NS_IMETHOD SetPluginFocused(bool& aFocused) = 0;
> 
>     /*
>+     * Tell the plugin has focus.  If this isn't implemented, return false;
>+     */
>+    virtual bool PluginHasFocus() const = 0;

How about you implement this in nsIWidget.h with final keyboard.

You can implement this as:

{
  return GetInputContext().mIMEState.mEnabled == IMEState::PLUGIN;
}

Then, please remove the implementation in nsWindowBase.h.

And also this shouldn't be called in child processes since it needs to do synchronous IPC, so, please add that into the comment.

Comment 55

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

Comment on attachment 8695139 [details] [diff] [review]
Part 12. Remove unused code into widget/windows

Oh, you're removing mIsComposingOnPlugin here!

But please check the vertical layout issue of plugin as I mentioned above.

Comment 56

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

Comment on attachment 8692363 [details] [diff] [review]
Part 7. Don't send WM_IME_REQUEST to plugin

Although, this is sad...

Comment 57

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #51)
> Comment on attachment 8694558 [details] [diff] [review]
> Part 1. Dispatch CompositionEvent to plugin
> 
> Looks like that you're removing mIsComposingOnPlugin, but I don't see it's
> actually dropped from the header file.
> 
> > -  if (!mIsComposingOnPlugin &&
> > -      aWritingMode.IsVertical() && IsVerticalWritingSupported()) {
> > +  if (aWritingMode.IsVertical() && IsVerticalWritingSupported()) {
> 
> I wonder, do you need this change? I assumed that Flash Player doesn't
> support vertical writing. Therefore, even if the <object> or <embed>'s
> writing-mode is specified as vertical, we need to treat it as horizontal
> layout.

OnSelectionChange etc will call this method, so I kept it.  But I should chechk PluginHasFocus instead.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #52)
> And you're adding wParam and lParam to DispatchCompositionChangeEvent(), but
> looks like that they are not used. Why?

Ah, I forget removing it.  I will update it.

Comment 58

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #56)
> Comment on attachment 8692363 [details] [diff] [review]
> Part 7. Don't send WM_IME_REQUEST to plugin
> 
> Although, this is sad...

After fixing this, I will think a way to pass it.

Comment 59

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

Comment on attachment 8694564 [details] [diff] [review]
Part 9. Hook IMM32 APIs on plugin process

># HG changeset patch
># Parent  7dec211829d568ab3a5ce92ca52609b97d0df419
>Bug 1208944 - Part 9. Hook IMM32 APIs on plugin process
>
>diff --git a/dom/ipc/PBrowser.ipdl b/dom/ipc/PBrowser.ipdl
>--- a/dom/ipc/PBrowser.ipdl
>+++ b/dom/ipc/PBrowser.ipdl
>@@ -277,16 +277,24 @@ parent:
>      * a plugin (inside the widget) has the keyboard focus.  Should be sent
>      * when the keyboard focus changes too or from a plugin.
>      *
>      * aFocused  Whether or not a plugin is focused
>      */
>     prio(urgent) async SetPluginFocused(bool aFocused);
> 
>     /**
>+     * Set IME candidate window by windowless plugin if plugin has focus.
>+     */
>+    async SetCandidateWindowForPlugin(uint32_t aIndex, uint32_t aStyle,
>+                                      uint32_t aX, uint32_t aY,
>+                                      uint32_t aLeft, uint32_t aTop,
>+                                      uint32_t aRight, uint32_t aBottom);

Could you add explanations for the arguments. And aStyle must be odd for other platform developers. I think that you should define some constants here for them.

> bool
>+TabParent::RecvSetCandidateWindowForPlugin(const uint32_t& aIndex,
>+                                           const uint32_t& aStyle,
>+                                           const uint32_t& aX,
>+                                           const uint32_t& aY,
>+                                           const uint32_t& aLeft,
>+                                           const uint32_t& aTop,
>+                                           const uint32_t& aRight,
>+                                           const uint32_t& aBottom)
>+{
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  if (!widget) {
>+    return true;
>+  }
>+
>+  widget->SetCandidateWindowForPlugin(aIndex, aStyle, aX, aY, aLeft, aTop,
>+aRight, aBottom);

nit: break the line.

>+
>+bool
>+nsPluginInstanceOwner::GetCompositionString(uint32_t aType,
>+                                            nsTArray<uint8_t>* aDist,
>+                                            int32_t* aLength)
>+{
>+  if (!mPluginFrame) {
>+    NS_WARNING("Plugin owner has no plugin frame.");

I think |if (NS_WARN_IF(!mPluginFrame)) {| is enough (i.e., you can remove NS_WARNING()).

>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIWidget> widget = GetContainingWidgetIfOffset();
>+  if (!widget) {
>+    widget = GetRootWidgetForPluginFrame(mPluginFrame);
>+    if (!widget) {
>+      NS_WARNING("No widget to get native composition attribute.");

ditto.

>+      return false;
>+    }
>+  }
>+
>+  RefPtr<TextComposition> composition =
>+    IMEStateManager::GetTextCompositionFor(widget);
>+  if (!composition) {
>+    NS_WARNING("Cannot get TextComposition. We have no composition yet?");

ditto.

>+    *aLength = 0;
>+    return true;
>+  }
>+
>+  switch(aType) {
>+  case GCS_COMPSTR: {

I like indent for case sentence. But up to you (depending on other switch statements in this file).

>+    uint32_t len = composition->LastData().Length() * sizeof(char16_t);
>+    if (len > 0) {

nit: |if (len) {|

>+      aDist->SetLength(len);
>+      memcpy(aDist->Elements(), composition->LastData().get(), len);
>+    }
>+    *aLength = len;
>+    return true;
>+  }
>+
>+  case GCS_RESULTSTR: {
>+    // IsComposing() during eCompositionChange isn't valid state yet.
>+    // But plugin only calls this WM_IME_COMPOSITION with GCS_RESULTSTR.
>+    uint32_t len = composition->LastData().Length() * sizeof(char16_t);
>+    if (len > 0) {

nit: |if (len) {|

>+      aDist->SetLength(len);
>+      memcpy(aDist->Elements(), composition->LastData().get(), len);
>+    }
>+    *aLength = len;
>+    return true;
>+  }
>+
>+  case GCS_CURSORPOS: {
>+    *aLength = 0;
>+    TextRangeArray* ranges = composition->GetLastRanges();
>+    if (!ranges) {
>+      return true;
>+    }
>+    if (ranges && ranges->Length()) {
>+      for (TextRange& range : *ranges) {
>+        if (range.mRangeType == NS_TEXTRANGE_CARETPOSITION) {
>+          *aLength = range.mStartOffset;
>+          break;
>+        }
>+      }
>+    }

Please implement this in TextRangeArray as GetCaretPosition().

>+    return true;
>+  }
>+
>+  case GCS_COMPATTR: {
>+    TextRangeArray* ranges = composition->GetLastRanges();
>+    if (!ranges || !ranges->Length()) {

nit: ranges->IsEmpty()?

>+  case GCS_COMPCLAUSE: {
>+    RefPtr<TextRangeArray> ranges = composition->GetLastRanges();
>+    if (!ranges || !ranges->Length()) {

nit: ranges->IsEmpty()?

>+      aDist->SetLength(sizeof(uint32_t));
>+      memset(aDist->Elements(), 0, sizeof(uint32_t));
>+      *aLength = aDist->Length();
>+      return true;
>+    }
>+    nsAutoTArray<uint32_t, 16> clauses;
>+    clauses.AppendElement(0);
>+    for (TextRange& range : *ranges) {
>+      if (range.mRangeType == NS_TEXTRANGE_CARETPOSITION) {

nit: range.IsClause().

>+bool
>+nsPluginInstanceOwner::SetCandidateWindow(
>+  uint32_t aIndex, uint32_t aStyle, uint32_t aX, uint32_t aY, uint32_t aLeft,
>+  uint32_t aTop, uint32_t aRight, uint32_t aBottom)
>+{
>+  if (!mPluginFrame) {
>+    NS_WARNING("Plugin owner has no plugin frame.");

nit: use NS_WARN_IF().

>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIWidget> widget = GetContainingWidgetIfOffset();
>+  if (!widget) {
>+    widget = GetRootWidgetForPluginFrame(mPluginFrame);
>+    if (!widget) {
>+      NS_WARNING("No widget to set candidate window.");

nit: use NS_WARN_IF().

>+      return false;
>+    }
>+  }
>+
>+  widget->SetCandidateWindowForPlugin(aIndex, aStyle, aX, aY, aLeft, aTop,
>+                                      aRight, aBottom);
>+  return true;
>+}
> #endif


I'd like you to change this line as: #endif // #ifdef XP_WIN

>+// static
>+LONG
>+PluginInstanceChild::ImmGetCompositionStringProc(HIMC aIMC, DWORD aIndex,
>+                                                 LPVOID aBuf, DWORD aLen)
>+{
>+    if (aIMC != sHookIMC || !sCurrentPluginInstance) {
>+        return sImm32ImmGetCompositionStringStub(aIMC, aIndex, aBuf, aLen);
>+    }
>+    nsTArray<uint8_t> dist;

Cannot use nsAutoTArray prevents heap fragmentation?

>diff --git a/dom/plugins/ipc/PluginQuirks.cpp b/dom/plugins/ipc/PluginQuirks.cpp
>--- a/dom/plugins/ipc/PluginQuirks.cpp
>+++ b/dom/plugins/ipc/PluginQuirks.cpp
>@@ -29,16 +29,22 @@ int GetQuirksFromMimeTypeAndFilename(con
>     if (specialType == nsPluginHost::eSpecialType_Flash) {
>         quirks |= QUIRK_FLASH_RETURN_EMPTY_DOCUMENT_ORIGIN;
> #ifdef OS_WIN
>         quirks |= QUIRK_WINLESS_TRACKPOPUP_HOOK;
>         quirks |= QUIRK_FLASH_THROTTLE_WMUSER_EVENTS;
>         quirks |= QUIRK_FLASH_HOOK_SETLONGPTR;
>         quirks |= QUIRK_FLASH_HOOK_GETWINDOWINFO;
>         quirks |= QUIRK_FLASH_FIXUP_MOUSE_CAPTURE;
>+#if defined(_M_X64) || defined(__x86_64__)
>+        // Flash x86 has protected mode, so don't consume IME window message
>+        // XXX If we can know that protected mode is disabled on Flash x86,
>+        // this flag should be set.
>+        quirks |= QUIRK_WINLESS_HOOK_IME;
>+#endif

Cannot we apply this change to x86 build simply? If so, this fix should be applied to x86 build. But I don't know major web service which uses windowless mode for flash contents which have text editor. (ustream is now using <input>, yay!)

>+void
>+PuppetWidget::SetCandidateWindowForPlugin(uint32_t aIndex, uint32_t aStyle,
>+                                          uint32_t aX, uint32_t aY,
>+                                          uint32_t aLeft, uint32_t aTop,
>+                                          uint32_t aRight, uint32_t aBottom)
>+{
>+  if (!mTabChild) {
>+    return;
>+  }
>+
>+  mTabChild->SendSetCandidateWindowForPlugin(aIndex, aStyle, aX, aY, aLeft, aTop, aRight, aBottom);

nit: wrap the line.



Looks excellent, but I'd like to check the new patch. Thank you very much for your great work!!

Comment 60

[(no longer active)]

Comment on attachment 8691258 [details] [diff] [review]
Part 4. nsWindowsDllInterceptor should supports some IMM APIs hook

Review of attachment 8691258 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +375,5 @@
> +  // To patch for JMP and JE
> +
> +  enum JumpType {
> +   Je,
> +   Jmp

Nit: indentation.

@@ +413,5 @@
> +      aCode[offset + 1] = 0x25;
> +      *reinterpret_cast<int32_t*>(aCode + offset + 2) = 0;
> +
> +      // Jump table
> +      *reinterpret_cast<int64_t*>(aCode + offset + 2 + 4) = mJumpAddress;

Do you mind please adding assertions verifying that these two addresses that you're dereferencing are aligned?

Comment 61

[Makoto Kato [:m_kato]]

Comment on attachment 8694566 [details] [diff] [review]
Part 10. Call CallWindowProc when event isn't handled by plugin

I will change some code by part 2 and part 9.  so, I cancel this until refreshing some patches.

Comment 62

[Makoto Kato [:m_kato]]

Comment on attachment 8694564 [details] [diff] [review]
Part 9. Hook IMM32 APIs on plugin process

I will remove x86 check and modify handling of IMM APIs that can use for x86 if Flash calls IMM APIs on correct process.

Comment 63

[Makoto Kato [:m_kato]]

Comment on attachment 8694558 [details] [diff] [review]
Part 1. Dispatch CompositionEvent to plugin

I will update it to remove unnecessary change.

Comment 64

[Makoto Kato [:m_kato]]

Attachment: Part 1. Dispatch CompositionEvent to plugin

Comment 65

[Jim Mathies [:jimm]]

Comment on attachment 8692795 [details] [diff] [review]
Part 11. Add test

sorry, forgot about this.

Comment 66

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

According to the using API list provided by Adobe, I think that following APIs should be hooked too:

* ImmSetCompositionWindow (for Chinese IME?)
* ImmIsIME (always returns true is enough? because Vista and later always returns true)
* ImmNotifyIME (perhaps, Flash tries to commit or cancel composition)

I guess that Imm(Get|Set)ComversionStatus and Imm(Get|Set)OpenStatus are not necessary since they must be used for changing IME state. This is one of the major reasons of some websites still using Flash Player. We should make them stop depending on Flash Player.

ImmGetProperty may be used to support some Chinese IME. However, for supporting this, we need more work to store active IME information in child process. So, I think that we should support this later if we need to do it actually.

Comment 67

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #66)
> According to the using API list provided by Adobe, I think that following
> APIs should be hooked too:
> 
> * ImmSetCompositionWindow (for Chinese IME?)
> * ImmIsIME (always returns true is enough? because Vista and later always
> returns true)
> * ImmNotifyIME (perhaps, Flash tries to commit or cancel composition)

When inputting composing text using various IMEs, it isn't called.


> I guess that Imm(Get|Set)ComversionStatus and Imm(Get|Set)OpenStatus are not
> necessary since they must be used for changing IME state. This is one of the
> major reasons of some websites still using Flash Player. We should make them
> stop depending on Flash Player.

That will be used by Capabilities.hasIME, IME.conversionMode and etc.  So it is unnecessary to implement on the spot input.  I will consider it after this is landed.

Comment 68

[Makoto Kato [:m_kato]]

Attachment: Part 2. Handle CompositionEvent on plugin

Comment 69

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

(In reply to Makoto Kato [:m_kato] from comment #67)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #66)
> > According to the using API list provided by Adobe, I think that following
> > APIs should be hooked too:
> > 
> > * ImmSetCompositionWindow (for Chinese IME?)
> > * ImmIsIME (always returns true is enough? because Vista and later always
> > returns true)
> > * ImmNotifyIME (perhaps, Flash tries to commit or cancel composition)
> 
> When inputting composing text using various IMEs, it isn't called.

Even on WinXP with Chinese IMEs?

Comment 70

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #69)
> (In reply to Makoto Kato [:m_kato] from comment #67)
> > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #66)
> > > According to the using API list provided by Adobe, I think that following
> > > APIs should be hooked too:
> > > 
> > > * ImmSetCompositionWindow (for Chinese IME?)
> > > * ImmIsIME (always returns true is enough? because Vista and later always
> > > returns true)
> > > * ImmNotifyIME (perhaps, Flash tries to commit or cancel composition)
> > 
> > When inputting composing text using various IMEs, it isn't called.
> 
> Even on WinXP with Chinese IMEs?

At 2 is nothing.  (I use Pinyin and New Phonetic).

But, ImmNotifyIME(..., NI_COMPOSITIONSTR, CPS_COMPLETE) will be always called at blur.  But gecko also commits/cancels composition at blur, it is no effect not to implement it.  But, maybe it should be implemented for changing application focus.

Comment 71

[Makoto Kato [:m_kato]]

Ops, I commit no-completed comment.

ImmNotifyIME(..., NI_COMPOSITIONSTR, CPS_COMPLETE) will be always called at blur.  But gecko also commits/cancels composition at blur, Even if not implementing it, on the spot input works.  But, maybe it should be implemented for changing application focus.

Comment 72

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

Okay, then, you should add only ImmNotifyIME() case for now. Let's watch regression reports. (I worry about the WinXP share in China and there are a lot of 3rd party's IMEs in China...)

Comment 73

[Makoto Kato [:m_kato]]

Attachment: Part 2-a. Handle CompositionEvent on plugin

Comment 74

[Makoto Kato [:m_kato]]

Attachment: Part 8. Don't get selection when plugin has focus

Comment 75

[Makoto Kato [:m_kato]]

Attachment: Part 10-a. Call CallWindowProc when WidgetPluginEvent isn't handled by plugin

Comment 76

[Makoto Kato [:m_kato]]

Attachment: Part 10-b. Call DefaultProc When CompositionEvent isn't handled correctly by plugin

Comment 77

[Makoto Kato [:m_kato]]

Comment on attachment 8698338 [details] [diff] [review]
Part 1. Dispatch CompositionEvent to plugin

Update part 1 patch.

OnIMEStartCompositionOnPlugin, OnIMECompositionOnPlugin and OnIMEEndCompositionOnPlugin will be used by part 10 patch series.  So it isn't removed.

Comment 78

[Makoto Kato [:m_kato]]

Comment on attachment 8699796 [details] [diff] [review]
Part 2-a. Handle CompositionEvent on plugin

To add NPEvent to composition event, I add new method to nsPluginInstanceOwner.

Comment 79

[Makoto Kato [:m_kato]]

Comment on attachment 8699799 [details] [diff] [review]
Part 8. Don't get selection when plugin has focus

Reduce IPC to check plugin focus

Comment 80

[Makoto Kato [:m_kato]]

Attachment: Part 2-b. Workaround for OSX

nsPluginInstanceOwner.h includes OS headers.  So some defines (mozilla::dom::Comment, TextRange, TextRangeArray) will be conflicted with audio structure etc

Comment 81

[Makoto Kato [:m_kato]]

Comment on attachment 8699801 [details] [diff] [review]
Part 10-a. Call CallWindowProc when WidgetPluginEvent isn't handled by plugin

WidgetPluginEvent that uses some IME window messages isn't consumed, we should call DefWindowProc.

Comment 82

[Makoto Kato [:m_kato]]

Attachment: Part 9. Hook IMM32 APIs on plugin process

Comment 83

[Makoto Kato [:m_kato]]

Attachment: Part 9. Hook IMM32 APIs on plugin process

Comment 84

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

Comment on attachment 8698338 [details] [diff] [review]
Part 1. Dispatch CompositionEvent to plugin

>-    case WM_IME_COMPOSITION:
>-      EnsureHandlerInstance();
>-      return gIMMHandler->OnIMECompositionOnPlugin(aWindow, wParam, lParam,
>-                                                   aResult);
>-    case WM_IME_STARTCOMPOSITION:
>-      EnsureHandlerInstance();
>-      return gIMMHandler->OnIMEStartCompositionOnPlugin(aWindow, wParam,
>-                                                        lParam, aResult);
>-    case WM_IME_ENDCOMPOSITION:
>-      EnsureHandlerInstance();
>-      return gIMMHandler->OnIMEEndCompositionOnPlugin(aWindow, wParam, lParam,
>-                                                      aResult);
>+      aRet = ProcessInputLangChangeMessage(aWindow, wParam, lParam, aResult);
>+      return true;
>     case WM_IME_CHAR:
>       EnsureHandlerInstance();
>-      return gIMMHandler->OnIMECharOnPlugin(aWindow, wParam, lParam, aResult);
>+      aRet = gIMMHandler->OnIMECharOnPlugin(aWindow, wParam, lParam, aResult);
>+      return true;
>     case WM_IME_SETCONTEXT:
>-      return OnIMESetContextOnPlugin(aWindow, wParam, lParam, aResult);
>+      aRet = OnIMESetContextOnPlugin(aWindow, wParam, lParam, aResult);
>+      return true;

Assuming that you remove these methods completely in another patch.

>@@ -2608,18 +2594,18 @@ IMMHandler::AdjustCompositionFont(const 
>   }
>   // Need to reset some information which should be recomputed with new font.
>   logFont.lfWidth = 0;
>   logFont.lfWeight = FW_DONTCARE;
>   logFont.lfOutPrecision = OUT_DEFAULT_PRECIS;
>   logFont.lfClipPrecision = CLIP_DEFAULT_PRECIS;
>   logFont.lfPitchAndFamily = DEFAULT_PITCH;
> 
>-  if (!mIsComposingOnPlugin &&
>-      aWritingMode.IsVertical() && IsVerticalWritingSupported()) {
>+  if (!aWindow->PluginHasFocus() &&
>+    aWritingMode.IsVertical() && IsVerticalWritingSupported()) {

nit: fix this odd indent.

Comment 85

[Makoto Kato [:m_kato]]

Comment on attachment 8699835 [details] [diff] [review]
Part 9. Hook IMM32 APIs on plugin process

ImmSetCandidateWindow on Flash process uses CFS_CANDIDATEPOS, It implements that this option only. Also, ImmNotifyIME uses a few options, I also do it only.

Comment 86

[Makoto Kato [:m_kato]]

Comment on attachment 8699802 [details] [diff] [review]
Part 10-b. Call DefaultProc When CompositionEvent isn't handled correctly by plugin

Protected mode Flash return consumed value for WM_IME_*COMPOSITION, but they  calls ImmGetCompositionStringW on thier protected mode process.  So they cannot get vaild composing string.  So, even if NPP_HandleEvent(WM_IME_COMPOSITION) returns consumed, we don't believe it.  If ImmGetCompositionStringW be called during this message, we trust this return value.

If NPP_HandleEvent(WM_IME_COMPOSITION) returns not-consumed, we should call default proc to show composition by OS.  At this time, we will post WM_IME_STARTCOMPOSITON if necessary.

If it is no problem  that composing string keeps top-left of screen on protected mode, this fix won't unnecessary.

Comment 87

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

Comment on attachment 8699796 [details] [diff] [review]
Part 2-a. Handle CompositionEvent on plugin

>+void
>+TextComposition::DispatchEvent(const WidgetCompositionEvent *aOriginalEvent,
>+                               WidgetCompositionEvent* aDispatchEvent,
>+                               nsEventStatus* aStatus,
>+                               EventDispatchingCallback* aCallBack)

Looks odd arguments to me...

How about:

DispatchEvent(WidgetCompositionEvent* aEvent,
              nsEventStatus* aStatus,
              EventDispatchingCallback* aCallback,
              const WidgetCompositionEvent* aOriginalEvent = nullptr)

Then, TextComposition::DispatchCompositionEvent() code looks better to me.

> nsresult
>+nsPluginInstanceOwner::DispatchCompositionToPlugin(nsIDOMEvent* aEvent)
>+{
>+#ifdef XP_WIN
>+  if (!mPluginWindow || (mPluginWindow->type == NPWindowTypeWindow)) {

nit: remove the unnecessary () after ||.

>+    return aEvent->PreventDefault(); // consume event

Hmm, this won't work as you expected because our CompositionEvent isn't cancellable. So, this should be just |return NS_OK;|.

>+  }
>+  WidgetCompositionEvent* compositionEvent =
>+    aEvent->GetInternalNSEvent()->AsCompositionEvent();
>+  if (compositionEvent) {

I think that this should be:

if (NS_WARN_IF(!compositionEvent)) {
  return NS_ERROR_INVALID_ARG;
}

>+    nsEventStatus rv = ProcessEvent(*compositionEvent);
>+    // XXX This isn't e10s aware.
>+    // If the event isn't consumed, we cannot post result to chrome process.
>+    if (nsEventStatus_eConsumeNoDefault == rv) {
>+      aEvent->PreventDefault();

Ditto. This does nothing.

>+      aEvent->StopPropagation();

You meant aEvent->StopImmediatePropagation()?

>+    }
>+  }
>+#endif
>+  return NS_OK;
>+}

>+  if (eventType.EqualsLiteral("compositionstart") ||
>+      eventType.EqualsLiteral("compositionupdate") ||
>+      eventType.EqualsLiteral("compositionend") ||
>+      eventType.EqualsLiteral("text")) {
>+    return DispatchCompositionToPlugin(aEvent);
>+  }

I wonder, *I* should rewrite here with checking WidgetEvent::mMessage...

>+  content->RemoveSystemEventListener(NS_LITERAL_STRING("compositionupdate"),
>+                                      this, true);

>+  aContent->AddSystemEventListener(NS_LITERAL_STRING("compositionupdate"),
>+    this, true);

Odd. Why do you need to handle compositionupdate? eCompositionUpdate is dispatched only when eCompositionChange causes changing composition string. So, eCompositionChange event must be followed. Therefore, you should be enough to handle "text" events caused by eCompositionUpdate.

>+// static
>+void
>+nsPluginInstanceOwner::GeneratePluginEvent(
>+  const WidgetCompositionEvent* aSrcCompositionEvent,
>+  WidgetCompositionEvent* aDistCompositionEvent)
>+{
>+#ifdef XP_WIN
>+  NPEvent newEvent;
>+  if (aDistCompositionEvent->mMessage == eCompositionUpdate) {

Why don't you use switch statement?

>+    // Original may be eCompositionCommit, but this update shouldn't have commit
>+    // lParam.
>+    newEvent.event = WM_IME_COMPOSITION;
>+    newEvent.wParam = 0;
>+    newEvent.lParam = GCS_COMPSTR | GCS_COMPATTR | GCS_COMPCLAUSE |
>+                      GCS_CURSORPOS;

So, I think that you don't need to do this for eCompositionUdpate.


>+  } else if (aDistCompositionEvent->mMessage == eCompositionChange) {
>+    newEvent.event = WM_IME_COMPOSITION;
>+    newEvent.wParam = 0;
>+    if (aSrcCompositionEvent &&
>+        (aSrcCompositionEvent->mMessage == eCompositionCommit ||
>+         aSrcCompositionEvent->mMessage == eCompositionCommitAsIs)) {
>+      newEvent.lParam = GCS_RESULTSTR | GCS_CURSORPOS;
>+    } else {
>+      newEvent.lParam = GCS_COMPSTR | GCS_COMPATTR | GCS_COMPCLAUSE |
>+                        GCS_CURSORPOS;
>+    }

I think that GCS_CURSORPOS should be set only when WidgetCompositionEvent::mRange has caret range. IIRC, Korean IME doesn't include GCS_CURSORPOS.


Otherwise, looks good to me.

Comment 88

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

Comment on attachment 8699801 [details] [diff] [review]
Part 10-a. Call CallWindowProc when WidgetPluginEvent isn't handled by plugin

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -1796,16 +1796,22 @@ public:
>       return GetInputContext().mIMEState.mEnabled == IMEState::PLUGIN;
>     }
> 
>     /**
>      * Set IME candidate window position by windowless plugin.
>      */
>     virtual void SetCandidateWindowForPlugin(int32_t aX, int32_t aY) = 0;
> 
>+    /**
>+     * Handle default action when PluginEvent isn't handled
>+     */
>+    virtual void DefaultProcOfPluginEvent(
>+                   const mozilla::WidgetPluginEvent& aEvent) = 0;

nit: You need to update uuid.

>+void
>+nsWindow::DefaultProcOfPluginEvent(const WidgetPluginEvent& aEvent)
>+{
>+  const NPEvent* pPluginEvent =
>+   static_cast<const NPEvent*>(aEvent.mPluginEvent);
>+
>+  if (NS_WARN_IF(!pPluginEvent)) {
>+    return;
>+  }
>+
>+  CallWindowProcW(GetPrevWindowProc(), mWnd, pPluginEvent->event,
>+                  pPluginEvent->wParam, pPluginEvent->lParam);

I guess you need to check mWnd before calling CallWindowProcW(), isn't it?

Comment 89

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

Comment on attachment 8699802 [details] [diff] [review]
Part 10-b. Call DefaultProc When CompositionEvent isn't handled correctly by plugin

>+  // Protected mode Flash returns noDefault by NPP_HandleEvent, but
>+  // composition information into plugin is invalid because plugin's bug.
>+  // So if plugin doesn't get composition data by WM_IME_COMPOSITION, we
>+  // recongnize it isn't handled
>+  AutoRestore<bool> restore(mGotCompositionData);
>+  mGotCompositionData = false;
>+
>+  nsEventStatus rv = ProcessEvent(*compositionEvent);

The variable name |rv| should be used only for nsresult. Could you rename this to |status|?

>+  aEvent->PreventDefault();

So, this won't work.

>+  aEvent->StopPropagation();

StopImmediatePropagation()?

>+
>+  // Composition event isn't handled by plugin, so we have to call default proc.
>+  const NPEvent* pPluginEvent =
>+    static_cast<const NPEvent*>(compositionEvent->mPluginEvent);
>+  if (NS_WARN_IF(!pPluginEvent)) {
>+    return NS_OK;
>+  }
>+
>+  if (pPluginEvent->event == WM_IME_STARTCOMPOSITION) {
>+    // Flash's protected mode lies that composition event is handled, but it
>+    // cannot do it well.  So even if handled, we should post this message when
>+    // no IMM API calls during WM_IME_COMPOSITION.
>+    if (nsEventStatus_eConsumeNoDefault != rv)  {
>+      CallDefaultProc(compositionEvent);
>+      mSentStartComposition = true;
>+    } else {
>+      mSentStartComposition = false;
>+    }
>+    return NS_OK;
>+  }
>+
>+  if (nsEventStatus_eConsumeNoDefault != rv) {
>+    // Not handled. Should call default proc
>+    CallDefaultProc(compositionEvent);
>+    return NS_OK;
>+  }
>+
>+  if (pPluginEvent->event == WM_IME_ENDCOMPOSITION) {
>+    // Always post WM_END_COMPOSITION to default proc. Because Flash may lies

s/may lies/may lie

>+    // that it doesn't handle composition well, but event is handled.
>+    // Even if posting this message, default proc do nothing if unnecessary.
>+    CallDefaultProc(compositionEvent);
>+    return NS_OK;
>+  }
>+
>+  if (pPluginEvent->event == WM_IME_COMPOSITION && !mGotCompositionData) {
>+    nsCOMPtr<nsIWidget> widget = GetContainingWidgetIfOffset();
>+    if (!widget) {
>+      widget = GetRootWidgetForPluginFrame(mPluginFrame);
>+    }
>+
>+    if (pPluginEvent->lParam & GCS_RESULTSTR) {
>+      // GCS_RESULTSTR's default proc will generate WM_CHAR. So emurate it.

s/emurate/emulate

>+      for (size_t i = 0; i < compositionEvent->mData.Length(); i++) {
>+        WidgetPluginEvent charEvent(true, ePluginInputEvent, widget);
>+        NPEvent event;
>+        event.event = WM_CHAR;
>+        event.wParam = compositionEvent->mData[i];
>+        event.lParam = 0;
>+        charEvent.mPluginEvent.Copy(event);
>+        ProcessEvent(charEvent);
>+      }

I think that you can use early return style here and remove indent for next else block.

>+    } else {
>+      if (!mSentStartComposition) {
>+        // We post WM_IME_COMPOSITION to default proc, but
>+        // WM_IME_STARTCOMPOSITION isn't post yet.  We should post it at first.
>+        WidgetPluginEvent event(true, ePluginInputEvent, widget);
>+        NPEvent npevent;
>+        npevent.event = WM_IME_STARTCOMPOSITION;
>+        npevent.wParam = 0;
>+        npevent.lParam = 0;
>+        event.mPluginEvent.Copy(npevent);
>+        CallDefaultProc(&event);
>+        mSentStartComposition = true;
>+      }
>+
>+      CallDefaultProc(compositionEvent);
>     }
>   }
>-#endif
>+#endif // #ifdef XP_WIN
>   return NS_OK;
> }

>-bool
>+void
> IMMHandler::OnIMEStartCompositionOnPlugin(nsWindow* aWindow,
>                                           WPARAM wParam,
>-                                          LPARAM lParam,
>-                                          MSGResult& aResult)
>+                                          LPARAM lParam)

Oh, reused!

Comment 90

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

Comment on attachment 8699835 [details] [diff] [review]
Part 9. Hook IMM32 APIs on plugin process

>+bool
>+nsPluginInstanceOwner::GetCompositionString(uint32_t aType,
>+                                            nsTArray<uint8_t>* aDist,
>+                                            int32_t* aLength)
>+{
>+  if (NS_WARN_IF(!mPluginFrame)) {
>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIWidget> widget = GetContainingWidgetIfOffset();
>+  if (!widget) {
>+    widget = GetRootWidgetForPluginFrame(mPluginFrame);
>+    if (NS_WARN_IF(!widget)) {
>+      return false;
>+    }
>+  }
>+
>+  RefPtr<TextComposition> composition =
>+    IMEStateManager::GetTextCompositionFor(widget);
>+  if (NS_WARN_IF(!composition)) {
>+    *aLength = 0;
>+    return true;
>+  }
>+
>+  switch(aType) {
>+    case GCS_COMPSTR: {
>+      uint32_t len = composition->LastData().Length() * sizeof(char16_t);
>+      if (len) {
>+        aDist->SetLength(len);
>+        memcpy(aDist->Elements(), composition->LastData().get(), len);
>+      }
>+      *aLength = len;
>+      return true;
>+    }
>+
>+    case GCS_RESULTSTR: {
>+      // IsComposing() during eCompositionChange isn't valid state yet.

I think that this is wrong. At handling eCompositionChange event in plugin instance owner, CompositionChangeEventHandlingMarker should be created into the stack.

>+      // But plugin only calls this WM_IME_COMPOSITION with GCS_RESULTSTR.
>+      uint32_t len = composition->LastData().Length() * sizeof(char16_t);
>+      if (len) {
>+        aDist->SetLength(len);
>+        memcpy(aDist->Elements(), composition->LastData().get(), len);
>+      }
>+      *aLength = len;
>+      return true;
>+    }
>+
>+    case GCS_CURSORPOS: {
>+      *aLength = 0;
>+      TextRangeArray* ranges = composition->GetLastRanges();
>+      if (!ranges) {
>+        return true;
>+      }
>+      *aLength = ranges->GetCaretPosition();

Hmm, if there is no caret range, this returns 0. I wonder, what happens when we try to retrieve caret pos with legacy Korean IME? (Perhaps, on XP)

>+// static
>+LONG
>+PluginInstanceChild::ImmGetCompositionStringProc(HIMC aIMC, DWORD aIndex,
>+                                                 LPVOID aBuf, DWORD aLen)
>+{
>+    if (aIMC != sHookIMC) {
>+        return sImm32ImmGetCompositionStringStub(aIMC, aIndex, aBuf, aLen);
>+    }
>+    if (!sCurrentPluginInstance) {
>+        return IMM_ERROR_GENERAL;
>+    }
>+    nsTArray<uint8_t> dist;

nsAutoTArray?

>+            case WM_IME_STARTCOMPOSITION:
>+            case WM_IME_COMPOSITION:
>+            case WM_IME_ENDCOMPOSITION:
>+                if (!(mParent->GetQuirks() & QUIRK_WINLESS_HOOK_IME)) {
>+                  // Such as Flash x86 w/ protected mode, window message for
>+                  // IME, but it doesn't handle IME composition correctly.

I don't understand "window message for IME". Don't you forget some words to input here?

>+  uint32_t GetCaretPosition() const
>+  {
>+    for (const TextRange& range : *this) {
>+      if (range.mRangeType == NS_TEXTRANGE_CARETPOSITION) {
>+        return range.mStartOffset;
>+      }
>+    }
>+    return 0;

I think that this should return UINT32_MAX if there is no caret range. And the callers should treat it.

>diff --git a/widget/nsIWidget.h b/widget/nsIWidget.h
>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -1791,16 +1791,21 @@ public:
>     /*
>      * Tell the plugin has focus.
>      */
>     bool PluginHasFocus() 
>     {
>       return GetInputContext().mIMEState.mEnabled == IMEState::PLUGIN;
>     }
> 
>+    /**
>+     * Set IME candidate window position by windowless plugin.
>+     */
>+    virtual void SetCandidateWindowForPlugin(int32_t aX, int32_t aY) = 0;

nit: update the uuid.



If you need to change GCS_CURSORPOS handler, please ask review me again.

Comment 91

[Makoto Kato [:m_kato]]

Attachment: Part 2-a. Handle CompositionEvent on plugin (v3)

Comment 92

[Makoto Kato [:m_kato]]

Comment on attachment 8700915 [details] [diff] [review]
Part 2-a. Handle CompositionEvent on plugin (v3)

Update by comment.

Comment 93

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

Comment on attachment 8700915 [details] [diff] [review]
Part 2-a. Handle CompositionEvent on plugin (v3)

>+void
>+TextComposition::DispatchEvent(nsEventStatus* aStatus,
>+                               EventDispatchingCallback* aCallBack,
>+                               WidgetCompositionEvent* aDispatchEvent,
>+                               const WidgetCompositionEvent *aOriginalEvent)

Hmm, the order of arguments is unusual...

Could you use this order?

(WidgetCompositionEvent* aDispatchEvent,
 nsEventStatus* aStatus,
 EventDispatchingCallback* aCallback,
 const WidgetCompositionEvent* aOriginalEvent)

>+  switch (aDistCompositionEvent->mMessage) {
>+    case eCompositionChange: {
>+      newEvent.event = WM_IME_COMPOSITION;
>+      newEvent.wParam = 0;
>+      if (aSrcCompositionEvent &&
>+          (aSrcCompositionEvent->mMessage == eCompositionCommit ||
>+           aSrcCompositionEvent->mMessage == eCompositionCommitAsIs)) {
>+        newEvent.lParam = GCS_RESULTSTR;
>+      } else {
>+        newEvent.lParam = GCS_COMPSTR | GCS_COMPATTR | GCS_COMPCLAUSE;
>+      }
>+      TextRangeArray* ranges = aDistCompositionEvent->mRanges;
>+      if (ranges) {
>+        if (ranges->HasCaret()) {

nit: if (ranges && ranges->HasCaret()) {

Otherwise, looks good to me.

Comment 94

[Makoto Kato [:m_kato]]

Attachment: Part 9. Hook IMM32 APIs on plugin process

Comment 95

[Makoto Kato [:m_kato]]

Attachment: Part 9. Hook IMM32 APIs on plugin process

Comment 96

[Makoto Kato [:m_kato]]

Comment on attachment 8701819 [details] [diff] [review]
Part 9. Hook IMM32 APIs on plugin process

Use CompositionChangeEventHandlingMarker on composition change.

Comment 97

[Makoto Kato [:m_kato]]

Comment on attachment 8701819 [details] [diff] [review]
Part 9. Hook IMM32 APIs on plugin process

cancel until building test on OSX.

Comment 98

[Makoto Kato [:m_kato]]

Attachment: Part 9. Hook IMM32 APIs on plugin process

Comment 99

[Makoto Kato [:m_kato]]

Comment on attachment 8701849 [details] [diff] [review]
Part 9. Hook IMM32 APIs on plugin process

Use CompositionChangeEventHandlingMarker on composition change.

Comment 100

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33cfddbbf5f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4213a355fbdc
https://hg.mozilla.org/integration/mozilla-inbound/rev/fde7e439844d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4974e397b655
https://hg.mozilla.org/integration/mozilla-inbound/rev/18160d306493
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2b9372dd36
https://hg.mozilla.org/integration/mozilla-inbound/rev/7685dcb63e5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0d70a6e809
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c47f2980b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e337d912b7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf73cf8f0fda
https://hg.mozilla.org/integration/mozilla-inbound/rev/759b425b5503

Comment 101

[Pulsebot]

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

Comment 102

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=54a19e8d57c4

M(2) test failure on OS X:
https://treeherder.mozilla.org/logviewer.html#?job_id=19078795&repo=mozilla-inbound

07:26:53     INFO -  1193 INFO TEST-START | dom/html/test/test_object_plugin_nav.html
07:26:56     INFO -  ###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
07:26:56     INFO -  ###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
07:26:56     INFO -  ###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
07:32:19     INFO -  TEST-INFO | started process screencapture
07:32:19     INFO -  TEST-INFO | screencapture: exit 0
07:32:19     INFO -  1194 INFO must wait for load
07:32:19     INFO -  1195 INFO TEST-PASS | dom/html/test/test_object_plugin_nav.html | undefined assertion name
07:32:19     INFO -  1196 INFO TEST-PASS | dom/html/test/test_object_plugin_nav.html | the plugin shouldn't get focus while navigating in the document
07:32:19     INFO -  1197 INFO TEST-PASS | dom/html/test/test_object_plugin_nav.html | first focused element should be the link
07:32:19     INFO -  1198 INFO TEST-PASS | dom/html/test/test_object_plugin_nav.html | second focused element should be the text field
07:32:19     INFO -  1199 INFO TEST-PASS | dom/html/test/test_object_plugin_nav.html | third focused element should be the button
07:32:19     INFO -  1200 INFO TEST-PASS | dom/html/test/test_object_plugin_nav.html | fourth focused element should be the object
07:32:19     INFO -  1201 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_object_plugin_nav.html | Test timed out.
07:32:19     INFO -      reportError@SimpleTest/TestRunner.js:114:7
07:32:19     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:134:7
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:32:19     INFO -      TestRunner.runTests@SimpleTest/TestRunner.js:366:5
07:32:19     INFO -      RunSet.runtests@SimpleTest/setup.js:188:3
07:32:19     INFO -      RunSet.runall@SimpleTest/setup.js:167:5
07:32:19     INFO -      hookupTests@SimpleTest/setup.js:260:5
07:32:19     INFO -  parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
07:32:19     INFO -  getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
07:32:19     INFO -  EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
07:32:19     INFO -      hookup@SimpleTest/setup.js:240:5
07:32:19     INFO -  EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=%2Fvar%2Ffolders%2Fqg%2F7pj314vs6ns665sg_7g6q8x000000w%2FT:11:1

M(5) failure on OS X:
https://treeherder.mozilla.org/logviewer.html#?job_id=19078770&repo=mozilla-inbound

Linux x64 opt VP[Tier-2](b-m)
https://treeherder.mozilla.org/logviewer.html#?job_id=19082858&repo=mozilla-inbound

08:27:25     INFO -   0:05.34 TEST_START: MainThread test_video_playback.py TestVideoPlayback.test_playback_starts
08:27:35     INFO -   0:14.92 LOG: MainThread INFO https://youtu.be/AbAACm1IQE0
08:27:41     INFO -   0:20.99 LOG: MainThread INFO https://www.youtube.com/watch?v=yOQQCoxs8-k
08:27:54     INFO -   0:33.93 LOG: MainThread INFO https://www.youtube.com/watch?v=1visYpIREUM
08:28:59    ERROR -   1:38.50 TEST_END: MainThread FAIL, expected PASS
08:28:59     INFO -  Traceback (most recent call last):
08:28:59     INFO -    File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/marionette_test.py", line 344, in run
08:28:59     INFO -      testMethod()
08:28:59     INFO -    File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/media_test_harness/testcase.py", line 121, in test_playback_starts
08:28:59     INFO -      self.check_playback_starts(video)
08:28:59     INFO -    File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/media_test_harness/testcase.py", line 64, in check_playback_starts
08:28:59    ERROR -      raise self.failureException(e)
08:28:59    ERROR -  AssertionError: TimeoutException: Timed out after 61.4 seconds; condition: playback_started
08:28:59     INFO -  VideoPuppeteer - test url: https://www.youtube.com/watch?v=1visYpIREUM: {
08:28:59     INFO -  	(video)
08:28:59     INFO -  	current_time: 8.057324,
08:28:59     INFO -  	duration: 8.056802,
08:28:59     INFO -  	expected_duration: 8.057324,
08:28:59     INFO -  	lag: 1.26,
08:28:59     INFO -  	url: https://www.youtube.com/watch?v=whj_vwhPsO8
08:28:59     INFO -  	src: mediasource:https://www.youtube.com/3a906b7d-b3a4-447c-9000-f1ded9397f9b
08:28:59     INFO -  	frames total: 240
08:28:59     INFO -  	 - dropped: 0
08:28:59     INFO -  	 - corrupted: 0
08:28:59     INFO -  }

Comment 103

[Makoto Kato [:m_kato]]

Humm, when I posted on try server, OSX 10.6 isn't orange...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7982fb8ce789

Comment 104

[Makoto Kato [:m_kato]]

Ah, I mistake merging patch.

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

Comment 105

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fb392fd3842
https://hg.mozilla.org/integration/mozilla-inbound/rev/5becd8aebc9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd8ab65dd29
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c21f80beba9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8290be2a82
https://hg.mozilla.org/integration/mozilla-inbound/rev/c25b4ada2dd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5a994af2ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9602769696e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a88721cadfc4
https://hg.mozilla.org/integration/mozilla-inbound/rev/18186849973c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e43204b34ed1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0565d59cf0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f199f2e1b1b

Comment 106

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8fb392fd3842
https://hg.mozilla.org/mozilla-central/rev/5becd8aebc9e
https://hg.mozilla.org/mozilla-central/rev/4cd8ab65dd29
https://hg.mozilla.org/mozilla-central/rev/9c21f80beba9
https://hg.mozilla.org/mozilla-central/rev/2d8290be2a82
https://hg.mozilla.org/mozilla-central/rev/c25b4ada2dd0
https://hg.mozilla.org/mozilla-central/rev/2d5a994af2ec
https://hg.mozilla.org/mozilla-central/rev/a9602769696e
https://hg.mozilla.org/mozilla-central/rev/a88721cadfc4
https://hg.mozilla.org/mozilla-central/rev/18186849973c
https://hg.mozilla.org/mozilla-central/rev/e43204b34ed1
https://hg.mozilla.org/mozilla-central/rev/d0565d59cf0a
https://hg.mozilla.org/mozilla-central/rev/7f199f2e1b1b