1456294 - IME Enabled on Flash Password Fields

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jeromie Clark]

Attachment: screencap of problem.swf

FAIL: Win7 + Firefox 56.0.1 & Firefox Nightly 58.0a1 + 27.0.0.185 
FAIL: Win8.1 + Firefox 56.0.1 & Firefox Nightly 58.0a1 + 27.0.0.185
FAIL: Win10_17017 + Firefox 56.0.1 & Firefox Nightly 58.0a1 + 27.0.0.185
FAIL: Win7 OS + Index of /pub/firefox/nightly/2016/08/2016-08-01-00-40-02-mozilla-aurora/ firefox-49.0a2.en-US.win64.installer.exe + 27.0.0.185
FAIL: Win8.1 OS + Index of /pub/firefox/nightly/2016/08/2016-08-01-00-40-02-mozilla-aurora/ firefox-49.0a2.en-US.win64.installer.exe + 27.0.0.185
FAIL: Win10_17017 OS + Index of /pub/firefox/nightly/2016/08/2016-08-01-00-40-02-mozilla-aurora/ firefox-49.0a2.en-US.win64.installer.exe + 27.0.0.185
FAIL: Win 7 OS + Firefox 49.0.2 + 27.0.0.185 (This is the injection firefox build)
FAIL: Win8.1 OS + Firefox 49.0.2 + 27.0.0.185
FAIL: Win10_17017 OS + Firefox 49.0.2 + 27.0.0.185
PASS: Win7 OS + Firefox 49.0.1 + 27.0.0.185
PASS: Win8.1 OS + Firefox 49.0.1 + 27.0.0.185
PASS: Win10_17017 OS + Firefox 49.0.1 + 27.0.0.185
PASS: Win7 + IE11 & Chrome + 27.0.0.185
PASS: Win8.1 + IE11 & Chrome + 27.0.0.185
PASS: Win10_17017 OS + IE11 & Chrome & Opera + 27.0.0.185
PASS:Mac10.11 os + Safari & Firefox 56.0 & Chrome + 27.0.0.185

Method:
1. Download the FP build and install it on the desktop.
2. Run the attached SWF: PasswordIMEDisabled.swf
3. Click the password TextField and input some characters, verify that IME is disabled.

Result:
IME is enabled when input password.

Expected:
IME should be disabled when input password.

Workaround:

2017-11-16T02:00:44: INFO : Narrowed nightly regression window from [2015-12-06, 2015-12-11] (5 days) to [2015-12-09, 2015-12-11] (2 days) (~1 steps left)
2017-11-16T02:00:57: INFO : Running mozilla-central build for 2015-12-10
2017-11-16T02:01:01: INFO : Launching c:\Users\labuser\AppData\Local\Temp\tmpgsrguc\firefox\firefox.exe
2017-11-16T02:01:01: INFO : Application command: c:\Users\labuser\AppData\Local\Temp\tmpgsrguc\firefox\firefox.exe -profile c:\users\labuser\appdata\local\temp\tmpfxrusz.mozrunner
2017-11-16T02:01:01: INFO : application_buildid: 20151210030212
2017-11-16T02:01:01: INFO : application_changeset: b40ba117fa757861c9caa660ae989122718b494b
2017-11-16T02:01:01: INFO : application_display_name: Nightly
2017-11-16T02:01:01: INFO : application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
2017-11-16T02:01:01: INFO : application_name: Firefox
2017-11-16T02:01:01: INFO : application_remotingname: firefox
2017-11-16T02:01:01: INFO : application_repository: https://hg.mozilla.org/mozilla-central
2017-11-16T02:01:01: INFO : application_vendor: Mozilla
2017-11-16T02:01:01: INFO : application_version: 45.0a1
2017-11-16T02:01:01: INFO : platform_buildid: 20151210030212
2017-11-16T02:01:01: INFO : platform_changeset: b40ba117fa757861c9caa660ae989122718b494b
2017-11-16T02:01:01: INFO : platform_repository: https://hg.mozilla.org/mozilla-central
2017-11-16T02:01:01: INFO : platform_version: 45.0a1

Comment 1

[Jeromie Clark]

Attachment: PasswordIMEDisabled.swf

Comment 2

[Jeromie Clark]

Sorry we couldn't regress this further, but it looks like the rest of the assets are unreachable.  :)

Comment 3

[Daniel Veditz [:dveditz]]

Is this likely an IME code issue or something in the plugin code?

Comment 4

[David Parks [:handyman]]

Confirming the mozregression window is between 12/09/15 and 12/11/15.  The push log window is:

dec 12/09/15:
https://hg.mozilla.org/mozilla-central/rev/bd229ced8be7
https://hg.mozilla.org/mozilla-central/pushloghtml/414

dec 12/11/15
https://hg.mozilla.org/mozilla-central/rev/754b4805a65c
https://hg.mozilla.org/mozilla-central/pushloghtml/413

A quick glance shows that this window includes turning on async plugin drawing.  Given how big and hairy that was, it seems like a good place to start.

Comment 5

[David Parks [:handyman]]

Spoke too soon. Bug 1179632 looks even more likely.

Comment 6

[Makoto Kato [:m_kato]]

Should we hook ImmAssociateContextEx to disable IME?

Comment 7

[Makoto Kato [:m_kato]]

OK, Flash seems to call ImmAssociateContextEx to disable IME.  But since we have to call ImmAssociateContextEx on chrome process via IPC, it might be risky to fix this.  So we should be fixed by 61, not 60.

I think that this isn't regression since we don't delegate ImmAssociateContextEx from Flash's process to Chrome process when we implement windowless plugin OOP

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Hook ImmAssociateContextEx

Comment 9

[Makoto Kato [:m_kato]]

attached patch works on win64, but it doesn't work on win32.  I am looking for a way to get PluginInstanceChild since ImmAssociateContextEx is called by varies message routine such as WM_LBUTTONDOWN.

Comment 10

[Makoto Kato [:m_kato]]

Ah, there is no way to implement this on 32bit plugin.  Because Flash calls ImmAssociateContextEx on their process even if windowless mode....

Comment 11

[Makoto Kato [:m_kato]]

Attachment: Hook ImmAssociateContextEx

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8974262 [details] [diff] [review]
Hook ImmAssociateContextEx

When entering password field on Flash, Flash plugin calls ImmAssociateContextEx to disable IME.  So we need hook this API to watch IME state.

When changing focus from chrome process to Flash, Flash calls ImmAssociateContextEx before receiving WM_SETFOCUS message.  So we have to watch ImmAssociateContextEx call for current state even if not having focus, then it have to set current state on chrome.

And as long as I test, WM_LBUTTONDOWN for changing focus calls ImmAssociateContextEx when taking focus.  But our implementation has PluginInstanceChild is into window's property.  So if cannot get it from global variant, it uses it from window's property.

This works well on 64bit Flash, but doesn't work on 32bit Flash due to protected mode bug of Flash.  Flash with protected mode calls ImmAssociateContextEx on their process even if windowless.  So there is no way to steal this API call.

Comment 13

[Makoto Kato [:m_kato]]

Adding jeclark since this is Flash issue and no way to implement on protected mode Flash (32-bit).

jeclark, why Flash doesn't delegate ImmAssociateContextEx call to our plugin process on 32bit Flash?  ImmGetCompositonString and ImmSetCandidateWindow already delegate to Firefox's plugin-container.exe.

Comment 14

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

Comment on attachment 8974262 [details] [diff] [review]
Hook ImmAssociateContextEx

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +2105,5 @@
> +BOOL
> +PluginInstanceChild::ImmAssociateContextExProc(HWND hWND, HIMC hImc,
> +                                               DWORD dwFlags)
> +{
> +    PluginInstanceChild* self = sCurrentPluginInstance;

I assume that PluginInstanceChild is created per Flash content, is this right? Or created only one for Flash Player? Or created per content process or per document?

@@ +2111,5 @@
> +        // If ImmAssociateContextEx calls unexpected window message,
> +        // we can use child instance object from window property if available.
> +        self = reinterpret_cast<PluginInstanceChild*>(
> +                   GetProp(hWND, kFlashThrottleProperty));
> +    }

If HWND is different from self's window handle, what should we do? At least, we should put warning or something, perhaps.

::: dom/plugins/ipc/PluginInstanceParent.h
@@ +320,5 @@
>          const mozilla::widget::CandidateWindowPosition& aPosition) override;
>      virtual mozilla::ipc::IPCResult
>      RecvRequestCommitOrCancel(const bool& aCommitted) override;
> +    virtual mozilla::ipc::IPCResult
> +     RecvEnableIME(const bool& aEnable) override;

nit: remove a space.

::: widget/nsIWidget.h
@@ +1824,5 @@
>  
>      /*
> +     * Enable or Disable IME by windowless plugin.
> +     */
> +    virtual void EnableIMEForPlugin(bool aEnable) = 0;

Well, if you implement empty body in nsIWidget directly, you can get rid of the change of nsBaseWidget.h, but up to you.

::: widget/windows/nsWindow.cpp
@@ +8359,5 @@
>  
> +void
> +nsWindow::EnableIMEForPlugin(bool aEnable)
> +{
> +  IMEHandler::AssociateIMEContext(this, aEnable);

Hmm, this might be really wrong approach.

When a password field in HTML gets focus, we notify IMM of "password" context with IMEHandler::SetInputScopeForIMM32() which is called by IMEHandler::SetInputContext():
https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/widget/windows/WinIMEHandler.cpp#620,691-694
https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/widget/windows/WinIMEHandler.cpp#449,474,490,492-493
As far as I know, this is important for the touch keyboard (i.e., currently, we don't support it on Flash).

So, sounds like that we need to add PLUGIN_PASSWORD state to IMEState::Enabled:
https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/widget/IMEData.h#118,129,153

And we should use IMEStateManager::OnChangeFocus() or IMEStateManager::UpdateIMEState() to call IMEHandler::SetInputContext() as usual rather than using new nsIWidget method.

However, this ideal fix requires a lot of changes in widget/windows and around IMEStateManager. So, I think that we shouldn't do that.

Anyway, I'm really afraid to this method. Even if we take this new nsIWidget method, we need to check if the state is still PLUGIN since if the IPC message comes after chrome gets focus, the IME state was already set to non-PLUGIN. Then, forcibly changing IME state may cause unexpected condition for our content.

Additionally, how about to call IMEHandler::SetInputContext() with PLUGIN and setting mHTMLInputType to "password"? (However, if Flash calls ImmAssociateContextEx() when no input field has focus, it might be wrong approach to set mHTMLInputType to "password".)

Comment 15

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (away: 4/28 - 5/6) from comment #14)
> Comment on attachment 8974262 [details] [diff] [review]
> Hook ImmAssociateContextEx
> 
> Review of attachment 8974262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/ipc/PluginInstanceChild.cpp
> @@ +2105,5 @@
> > +BOOL
> > +PluginInstanceChild::ImmAssociateContextExProc(HWND hWND, HIMC hImc,
> > +                                               DWORD dwFlags)
> > +{
> > +    PluginInstanceChild* self = sCurrentPluginInstance;
> 
> I assume that PluginInstanceChild is created per Flash content, is this
> right? Or created only one for Flash Player? Or created per content process
> or per document?
> 
> @@ +2111,5 @@
> > +        // If ImmAssociateContextEx calls unexpected window message,
> > +        // we can use child instance object from window property if available.
> > +        self = reinterpret_cast<PluginInstanceChild*>(
> > +                   GetProp(hWND, kFlashThrottleProperty));
> > +    }
> 
> If HWND is different from self's window handle, what should we do? At least,
> we should put warning or something, perhaps.

If HWND isn't Flash's dummy window, we should call original ImmAssociateContextEx call.  Since we support Flash only as plugin, it might be Flash bug or unexpected call.   OK, I will add MOZ_WARNING_ASSERTION.

> ::: dom/plugins/ipc/PluginInstanceParent.h
> @@ +320,5 @@
> >          const mozilla::widget::CandidateWindowPosition& aPosition) override;
> >      virtual mozilla::ipc::IPCResult
> >      RecvRequestCommitOrCancel(const bool& aCommitted) override;
> > +    virtual mozilla::ipc::IPCResult
> > +     RecvEnableIME(const bool& aEnable) override;
> 
> nit: remove a space.
> 
> ::: widget/nsIWidget.h
> @@ +1824,5 @@
> >  
> >      /*
> > +     * Enable or Disable IME by windowless plugin.
> > +     */
> > +    virtual void EnableIMEForPlugin(bool aEnable) = 0;
> 
> Well, if you implement empty body in nsIWidget directly, you can get rid of
> the change of nsBaseWidget.h, but up to you.

OK

> 
> ::: widget/windows/nsWindow.cpp
> @@ +8359,5 @@
> >  
> > +void
> > +nsWindow::EnableIMEForPlugin(bool aEnable)
> > +{
> > +  IMEHandler::AssociateIMEContext(this, aEnable);
> 
> Hmm, this might be really wrong approach.
> 
> When a password field in HTML gets focus, we notify IMM of "password"
> context with IMEHandler::SetInputScopeForIMM32() which is called by
> IMEHandler::SetInputContext():
> https://searchfox.org/mozilla-central/rev/
> 5ff2d7683078c96e4b11b8a13674daded935aa44/widget/windows/WinIMEHandler.
> cpp#620,691-694
> https://searchfox.org/mozilla-central/rev/
> 5ff2d7683078c96e4b11b8a13674daded935aa44/widget/windows/WinIMEHandler.
> cpp#449,474,490,492-493
> As far as I know, this is important for the touch keyboard (i.e., currently,
> we don't support it on Flash).
> 
> So, sounds like that we need to add PLUGIN_PASSWORD state to
> IMEState::Enabled:
> https://searchfox.org/mozilla-central/rev/
> 5ff2d7683078c96e4b11b8a13674daded935aa44/widget/IMEData.h#118,129,153
> 
> And we should use IMEStateManager::OnChangeFocus() or
> IMEStateManager::UpdateIMEState() to call IMEHandler::SetInputContext() as
> usual rather than using new nsIWidget method.
> 
> However, this ideal fix requires a lot of changes in widget/windows and
> around IMEStateManager. So, I think that we shouldn't do that.
> 
> Anyway, I'm really afraid to this method. Even if we take this new nsIWidget
> method, we need to check if the state is still PLUGIN since if the IPC
> message comes after chrome gets focus, the IME state was already set to
> non-PLUGIN. Then, forcibly changing IME state may cause unexpected condition
> for our content.

Yes, I think it at first time, but it is a lot of risk and we are abandoning plugin support at near feature...
 
> Additionally, how about to call IMEHandler::SetInputContext() with PLUGIN
> and setting mHTMLInputType to "password"? (However, if Flash calls
> ImmAssociateContextEx() when no input field has focus, it might be wrong
> approach to set mHTMLInputType to "password".)

Hmm, I think that we should add a check whether current IME state is PLUGIN or not before calling ImmAssociateContextEx on chrome process.  And if PLUGIN, 

IMEHandler::SetInputContext doesn't disable IME with PLUGIN, but it might be able to disable when adding new condition of PLUGIN + mHTMLInputType = password.  I will try it.

Comment 16

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

> Hmm, I think that we should add a check whether current IME state is PLUGIN or not before calling
> ImmAssociateContextEx on chrome process.

Please do not use GetInputContext() to check current IME state since getting IME open state from TSF is really slow. Please refer the input context directly.

> IMEHandler::SetInputContext doesn't disable IME with PLUGIN, but it might be able to disable when
> adding new condition of PLUGIN + mHTMLInputType = password.  I will try it.

Ah, yes. Thanks.

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Hook ImmAssociateContextEx

Comment 18

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

Comment on attachment 8975222 [details] [diff] [review]
Hook ImmAssociateContextEx

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

Almost looks good to me, but I'd like to check new patch again. So, temporarily r-.

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +2214,5 @@
>        SetFocus(focusHwnd);
>      }
>  
> +    if (event.event == WM_SETFOCUS) {
> +        // When focus is changed from chrome process to plugin process,

nit: Looks like one level indent in this file looks like 2 whiespaces.

@@ +2215,5 @@
>      }
>  
> +    if (event.event == WM_SETFOCUS) {
> +        // When focus is changed from chrome process to plugin process,
> +        // Flash may call ImmAssociateContextEx before getting focus.

What you are trying to say here is unclear to me. Do you mean "Flash may call ImmAssociateContextEx even while it doesn't have focus.  However, our main process doesn't associate/disassociate IM context with its window while plugin does not have focus but the request came from a plugin process.  Therefore, we need to make the main process associate/disassociate IM context right now for the last ImmAssociateContextEx call."?

@@ +2218,5 @@
> +        // When focus is changed from chrome process to plugin process,
> +        // Flash may call ImmAssociateContextEx before getting focus.
> +        SendEnableIME(mLastEnableIMEState);
> +    } else if (event.event == WM_KILLFOCUS) {
> +        mLastEnableIMEState = true;

I guess that if password fields of Flash don't have focus, Flash doesn't call ImmAssociateContextEx before WM_SETFOCUS.  Therefore, for making sure the accessibility of IME users on normal input fields, we should reset mLastEnableIMEState to true here? Please explain it with comment here.

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +2299,5 @@
>  
> +mozilla::ipc::IPCResult
> +PluginInstanceParent::RecvEnableIME(const bool& aEnable)
> +{
> +#if defined(OS_WIN)

Do we really need this #if defined(OS_WIN) here? SendEnableIME() is called only on Windows anyway.

If you want to making other developers confirm here when they use this API on the other platforms, perhaps, you should add #else block for putting warning message or MOZ_ASSERT.  Then, they can realize here has not been tested on the other platforms yet.

::: widget/PuppetWidget.cpp
@@ +1419,5 @@
> +  if (NS_WARN_IF(HaveValidInputContextCache() &&
> +                 mInputContext.mIMEState.mEnabled != IMEState::UNKNOWN &&
> +                 mInputContext.mIMEState.mEnabled != IMEState::PLUGIN)) {
> +   return;
> + }

nit: Odd indent this and previous line.

Comment 19

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (away: 4/28 - 5/6) from comment #18)
> Comment on attachment 8975222 [details] [diff] [review]
> Hook ImmAssociateContextEx
> 
> Review of attachment 8975222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost looks good to me, but I'd like to check new patch again. So,
> temporarily r-.
> 
> ::: dom/plugins/ipc/PluginInstanceChild.cpp
> @@ +2214,5 @@
> >        SetFocus(focusHwnd);
> >      }
> >  
> > +    if (event.event == WM_SETFOCUS) {
> > +        // When focus is changed from chrome process to plugin process,
> 
> nit: Looks like one level indent in this file looks like 2 whiespaces.

No, PluginInstanceChild is 4 (vim: ws=4 ts=4)

> 
> @@ +2215,5 @@
> >      }
> >  
> > +    if (event.event == WM_SETFOCUS) {
> > +        // When focus is changed from chrome process to plugin process,
> > +        // Flash may call ImmAssociateContextEx before getting focus.
> 
> What you are trying to say here is unclear to me. Do you mean "Flash may
> call ImmAssociateContextEx even while it doesn't have focus.  However, our
> main process doesn't associate/disassociate IM context with its window while
> plugin does not have focus but the request came from a plugin process. 
> Therefore, we need to make the main process associate/disassociate IM
> context right now for the last ImmAssociateContextEx call."?

This is hack for moving focus from chrome process to flash process.

When moving focus from chrome process to Flash process directly  (When focus is chrome's address bar, user click Flash's password field),  Gecko sends mouse message (WM_LBUTTONDOWN etc) at first, then WM_SETFOCUS is sent at second.  So when sending WM_LBUTTONDOWN to Flash, focus might be still on chrome process.  Then when leaving focus on chrome process and taking focus on content process, ImmAssociateCotnextEx will be called by xul!nsWebBrowser::Activate -> ... -> nsFocusManager::Focus -> ... -> mozilla::IMEStateManager::SetInputContext after calling ImmAssociateCotnextEx on Flash.  So I have to set IME state again.

> @@ +2218,5 @@
> > +        // When focus is changed from chrome process to plugin process,
> > +        // Flash may call ImmAssociateContextEx before getting focus.
> > +        SendEnableIME(mLastEnableIMEState);
> > +    } else if (event.event == WM_KILLFOCUS) {
> > +        mLastEnableIMEState = true;
> 
> I guess that if password fields of Flash don't have focus, Flash doesn't
> call ImmAssociateContextEx before WM_SETFOCUS.  Therefore, for making sure

No, reason is above.

> the accessibility of IME users on normal input fields, we should reset
> mLastEnableIMEState to true here? Please explain it with comment here.

As long as I test (and trace by debugger), Flash always calls ImmAssociateContextEx by taking focus.  Although this flag doesn't have to be reset, I add it for safety.

> ::: dom/plugins/ipc/PluginInstanceParent.cpp
> @@ +2299,5 @@
> >  
> > +mozilla::ipc::IPCResult
> > +PluginInstanceParent::RecvEnableIME(const bool& aEnable)
> > +{
> > +#if defined(OS_WIN)
> 
> Do we really need this #if defined(OS_WIN) here? SendEnableIME() is called
> only on Windows anyway.

This IPC is only for Windows.

> If you want to making other developers confirm here when they use this API
> on the other platforms, perhaps, you should add #else block for putting
> warning message or MOZ_ASSERT.  Then, they can realize here has not been
> tested on the other platforms yet.

OK, I will add MOZ_CRASH("Not reachable")

Comment 20

[Makoto Kato [:m_kato]]

(I cannot make sense that Flash calls ImmAssociateContextEx without focus, but it might be historical workaround for any reasons or be simply bug)

Comment 21

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

> When moving focus from chrome process to Flash process directly  (When focus is chrome's address bar,
> user click Flash's password field),  Gecko sends mouse message (WM_LBUTTONDOWN etc) at first, then
> WM_SETFOCUS is sent at second.  So when sending WM_LBUTTONDOWN to Flash, focus might be still on chrome
> process.  Then when leaving focus on chrome process and taking focus on content process,
> ImmAssociateCotnextEx will be called by xul!nsWebBrowser::Activate -> ... -> nsFocusManager::Focus ->
>  ... -> mozilla::IMEStateManager::SetInputContext after calling ImmAssociateCotnextEx on Flash.  So I
> have to set IME state again.

Okay, please document it in the patch as comment. (Although ideally, IMEStateManager::SetInputContext should set input type when it sets PLUGIN...)

> As long as I test (and trace by debugger), Flash always calls ImmAssociateContextEx by taking focus.
> Although this flag doesn't have to be reset, I add it for safety.

Same. Please document this in the patch as comment. This is really necessary information to understand what you do.

Comment 22

[Makoto Kato [:m_kato]]

Attachment: Hook ImmAssociateContextEx

Comment 23

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

Comment on attachment 8976020 [details] [diff] [review]
Hook ImmAssociateContextEx

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

Thank you very much!

Comment 24

[Makoto Kato [:m_kato]]

Comment on attachment 8976020 [details] [diff] [review]
Hook ImmAssociateContextEx

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Yes and No. it depends on IME from patch and it is related that Flash can enable IME or disable IME.  But most people think that this is a new Flash feature by Adobe.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I don't add test case.

Which older supported branches are affected by this flaw?

Firefox 3.6.

If not all supported branches, which bug introduced the flaw?

3.6 has out of process plugin support.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Not difficult, but this change is risky since it is related to Flash behavior.

How likely is this patch to cause regressions; how much testing does it need?

User cannot enable IME when taking focus on Flash.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8976020 [details] [diff] [review]
Hook ImmAssociateContextEx

As a sec-vector rated bug, this doesn't need sec-approval. It can just land on trunk.

Comment 26

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e0c05903821

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/9e0c05903821

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Is this something we'd eventually want to consider for backport (like maybe to a later ESR60 release after it's baked), or should this just ride Fx62 to release?

Comment 29

[Makoto Kato [:m_kato]]

Comment on attachment 8976020 [details] [diff] [review]
Hook ImmAssociateContextEx

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
When Flash content sets password attribute by ActionScript,  user can turn on IME even if password field.

Fix Landed on Version:
62

Risk to taking this patch (and alternatives if risky): 
User may not be able to turn on IME on other fields on Flash content if there is other bug.  But we don't have new report after landing this (except bug 1464061).

Also, when landing this, this need land bug 1464061's fix to fix crash bug too.

String or UUID changes made by this patch: 
Nothing

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 30

[Makoto Kato [:m_kato]]

Comment on attachment 8976020 [details] [diff] [review]
Hook ImmAssociateContextEx

Approval Request Comment
[Feature/Bug causing the regression]:
Out of process plugin support that was from Firefox 3.6.3

[User impact if declined]:
Even if user uses password attribute of text field on Flash content/ActionScript, IME can turn on this field.

[Is this code covered by automated tests?]:
No.  because no way to test this.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
bug 1464061

[Is the change risky?]:
middle.

[Why is the change risky/not risky?]:
User may not be able to turn on IME on other fields on Flash content after moving focus to non-password field if there is other bug.  But no report after landing this. 

[String changes made/needed]:
No

Comment 31

[Ryan VanderMeulen [:RyanVM]]

I'm worried about whether our Nightly user population is large and diverse enough to find regressions in this specific area of functionality. This patch makes me nervous, honestly. What do you think about letting this go to Beta with 62 and then consider backporting to ESR60 for 60.2 instead?

Comment 32

[Makoto Kato [:m_kato]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)
> I'm worried about whether our Nightly user population is large and diverse
> enough to find regressions in this specific area of functionality. This
> patch makes me nervous, honestly. What do you think about letting this go to
> Beta with 62 and then consider backporting to ESR60 for 60.2 instead?

Yes I think that it is better that it is fixed by ESR60.2 for ESR branch and we need more testing for Nightly users.  This is risky for beta and ESR60.1.

Also, actually, Adobe has to fix 32-bit Flash plugin because there is another bug in 32-bit Flash, although 64-bit is no issue.  (We are talking about it with Adobe)

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Thanks for the info, that helps. Updating the various flags to reflect the revised plan.

Comment 34

[Rares Doghi, Desktop QA]

Hi, i managed to reproduce this issue in older version of Firefox but i could no longer reproduce it in Nightly 62.0a1 (2018-06-18) 64-bit. I will mark this issue Verified as Fixed.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Hi Makoto-san, are you aware of any issues that this patch has caused or are we good to proceed with the uplift consideration?

Comment 36

[Makoto Kato [:m_kato]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)
> Hi Makoto-san, are you aware of any issues that this patch has caused or are
> we good to proceed with the uplift consideration?

No regression report.  It is good time to uplift this to ESR.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8976020 [details] [diff] [review]
Hook ImmAssociateContextEx

Thanks for the reply, approving for ESR 60.2.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/cb8f0498a8b1

Comment 39

[Rares Doghi, Desktop QA]

Hi, I can no longer reproduce this issue in Firefox 60.1.1esr (64-bit), I will mark this issue accordingly.