1201904 - [x86_64][Win] IME is disabled on Flash Player

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

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

Only on x86_64 build, IME isn't available on Flash Player. Although, I've not investigated the symptom yet, it looks like that the plug-in window isn't associated IME context.

According to the original reporter, this is started from 8/24's build.

As far as I tested on the testcase of the URI, windowed plugin doesn't have this problem. (Although, commit string isn't inserted to the windowless flash player, it must be another bug.)

Comment 1

[Benjamin Smedberg]

Regression from sandboxing. We're not going to block Fx64 release on this.

Comment 2

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Regression from sandboxing. We're not going to block Fx64 release on this.

If sandboxing is enabled in default settings, it sounds bad because Japanese major video site, Nico Nico Doga can post comment from Flash Player's input field (and the comment is scrolled over the movie). Therefore, cannot use IME on x64 build makes our product image for them too bad since the regression causes that they cannot use one of main purposes to use web browser.

Comment 3

[Benjamin Smedberg]

This is for Firefox 64-bit, which is still a tech preview and will not go to anyone by default. We're not willing to ship it without a Flash sandbox.

Comment 4

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> This is for Firefox 64-bit, which is still a tech preview and will not go to
> anyone by default. We're not willing to ship it without a Flash sandbox.

Did you mean that Firefox x64 for Windows won't be released as official release? I.e., users will need to one or more steps for downloading the x64 version than x86 version? E.g., they need to click a link like "Other languages" in the current download page? (Or hidden, like ESR) If so, it sounds not bad.

Comment 5

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

FYI: Some modern Japanese IME need to communicate with another process in Windows. So, if sandboxing kills all communications between processes, IME won't be available.

Comment 6

[Masatoshi Kimura [:emk]]

Probably the blocking bug number is incorrect.

Comment 7

[Benjamin Smedberg]

Yes, it will only be available from the "all systems and languages" link.

Comment 8

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Yes, it will only be available from the "all systems and languages" link.

Sounds good ;-)

Comment 9

[Bob Owen (:bobowen)]

I'm guessing this is a similar problem to bug 1180684, although that is only for old ActionScript 1 and 2 content.

It seems the low integrity sandbox interrupts the way Flash handles keyboard events.
From what I can tell in Flash protected mode, I think they might explicitly proxy calls to GetKeyState to their broker process.

I'll leave the ni, so I can look at this specific issue as it might give me some ideas for fixing both of these.

Comment 10

[Makoto Kato [:m_kato]]

AddSandboxAllowedFiles kills some local file access...

IME on Chrome is handled on render host.  So it isn't on sandbox and, flash is PPAPI...

Comment 11

[Makoto Kato [:m_kato]]

(In reply to Bob Owen (:bobowen) from comment #9)
> I'm guessing this is a similar problem to bug 1180684, although that is only
> for old ActionScript 1 and 2 content.

No.

IME wants to access user profile to save user dictionary.

- Microsoft IME uses AppData\LocalLow\IME etc... (ex. IMEJP10)
- Google Japanese IME uses AppData\LocalLow\Google\Google Japanese Input
- ATOK users AppData\Roaming\Justsystem\ATOK (Why doesn't they use LocalLow??)

So we need white list for IME.

Comment 12

[Makoto Kato [:m_kato]]

Attachment: Allow r/w access for IMEs

Comment 13

[Makoto Kato [:m_kato]]

Also, I doesn't have Fujitsu's Japanist, so I cannot test this.

Comment 14

[Makoto Kato [:m_kato]]

This patch may not work on Microsoft IME on Windows 7 because path is different.

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 8657701 [details] [diff] [review]
Allow r/w access for IMEs

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

Thanks for looking into this.

::: dom/plugins/ipc/PluginProcessParent.cpp
@@ +113,5 @@
> +                          NS_WIN_LOCAL_APPDATA_LOW_DIR,
> +                          NS_LITERAL_STRING("\\Microsoft\\IME\\*"));
> +    AddSandboxAllowedFile(aAllowedFilesReadWrite, dirSvc,
> +                          NS_WIN_LOCAL_APPDATA_LOW_DIR,
> +                          NS_LITERAL_STRING("\\Google\\Google Japanese Input\\*"));

As it currently stands the sandboxed process should already have write access to the low integrity directory.

@@ +115,5 @@
> +    AddSandboxAllowedFile(aAllowedFilesReadWrite, dirSvc,
> +                          NS_WIN_LOCAL_APPDATA_LOW_DIR,
> +                          NS_LITERAL_STRING("\\Google\\Google Japanese Input\\*"));
> +    AddSandboxAllowedFile(aAllowedFilesReadWrite, dirSvc, NS_WIN_APPDATA_DIR,
> +                          NS_LITERAL_STRING("\\Justsystem\\*"));

Do we have any contacts at JustSystems, to see if they might move their dictionary?

Comment 16

[Bob Owen (:bobowen)]

Masayuki - are you able to test this patch.

Comment 17

[Makoto Kato [:m_kato]]

(In reply to Bob Owen (:bobowen) from comment #15)
> Comment on attachment 8657701 [details] [diff] [review]
> Allow r/w access for IMEs
> 
> Review of attachment 8657701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for looking into this.
> 
> ::: dom/plugins/ipc/PluginProcessParent.cpp
> @@ +113,5 @@
> > +                          NS_WIN_LOCAL_APPDATA_LOW_DIR,
> > +                          NS_LITERAL_STRING("\\Microsoft\\IME\\*"));
> > +    AddSandboxAllowedFile(aAllowedFilesReadWrite, dirSvc,
> > +                          NS_WIN_LOCAL_APPDATA_LOW_DIR,
> > +                          NS_LITERAL_STRING("\\Google\\Google Japanese Input\\*"));
> 
> As it currently stands the sandboxed process should already have write
> access to the low integrity directory.

If sandbox level is 2+, we restrict file access to read-only under home directory at this AddSandboxAllowedFiles function.

If we can allow r/w on user's LocalLow, the fix for Microsoft IME and Google IME is more simple.

> @@ +115,5 @@
> > +    AddSandboxAllowedFile(aAllowedFilesReadWrite, dirSvc,
> > +                          NS_WIN_LOCAL_APPDATA_LOW_DIR,
> > +                          NS_LITERAL_STRING("\\Google\\Google Japanese Input\\*"));
> > +    AddSandboxAllowedFile(aAllowedFilesReadWrite, dirSvc, NS_WIN_APPDATA_DIR,
> > +                          NS_LITERAL_STRING("\\Justsystem\\*"));
> 
> Do we have any contacts at JustSystems, to see if they might move their
> dictionary?

I don't have "official" contact.  But I and Nakano-san can request it to them. (I don't know that they accept it).

Comment 18

[Bob Owen (:bobowen)]

(In reply to Makoto Kato [:m_kato] from comment #17)
> (In reply to Bob Owen (:bobowen) from comment #15)

> > > +                          NS_WIN_LOCAL_APPDATA_LOW_DIR,
> > > +                          NS_LITERAL_STRING("\\Google\\Google Japanese Input\\*"));
> > 
> > As it currently stands the sandboxed process should already have write
> > access to the low integrity directory.
> 
> If sandbox level is 2+, we restrict file access to read-only under home
> directory at this AddSandboxAllowedFiles function.

This is only if we move to level > 2.

Comment 19

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

I'm not sure if I can test it today. But I guess that we need to check with Baidu IME, Japanist 2003, some SKK IMEs? Although, I'm not familiar with SKK.

Looks like that Japanist 2003 created this:
AppData\Roaming\Fujitsu\Japanist3
Baidu IME created this:
AppData\LocalLow\Baidu\IME
CorvusSKK created this, but it's empty in my environment (this IME doesn't work on my environment, though):
AppData\Roaming\CorvusSKK

Comment 20

[Makoto Kato [:m_kato]]

[Tracking Requested - why for this release]:

Comment 21

[Makoto Kato [:m_kato]]

hum, I misunderstand.  I seem that current sandbox setting is too harder to enable IME on window created by plugin.

Comment 22

[Makoto Kato [:m_kato]]

Attachment: Use sandbox::USER_NON_ADMIN as access token level for IME

Window mode flash creates own window by CreateWindow API.  But this window cannot allow IME with sandbox::USER_INTERACTIVE.

So to enable IME, we should use sandbox::USER_NON_ADMIN via access token level = 2

Comment 23

[Bob Owen (:bobowen)]

(In reply to Makoto Kato [:m_kato] from comment #22)
> Created attachment 8658522 [details] [diff] [review]
> Use sandbox::USER_NON_ADMIN as access token level for IME
> 
> Window mode flash creates own window by CreateWindow API.  But this window
> cannot allow IME with sandbox::USER_INTERACTIVE.
> 
> So to enable IME, we should use sandbox::USER_NON_ADMIN via access token
> level = 2

So, this will give access back to the Authenticated Users identity and Interactive identity:
https://technet.microsoft.com/en-us/library/bb726982.aspx

Ideally we would want to be strengthening this setting not weakening it.

Is there a way we can get the parent process to allow IME for the flash window?

Presumably we would still have the issue with write access to user dictionaries, but perhaps that is less important.

Comment 24

[Makoto Kato [:m_kato]]

(In reply to Bob Owen (:bobowen) from comment #23)
> (In reply to Makoto Kato [:m_kato] from comment #22)
> > Created attachment 8658522 [details] [diff] [review]
> > Use sandbox::USER_NON_ADMIN as access token level for IME
> > 
> > Window mode flash creates own window by CreateWindow API.  But this window
> > cannot allow IME with sandbox::USER_INTERACTIVE.
> > 
> > So to enable IME, we should use sandbox::USER_NON_ADMIN via access token
> > level = 2
> 
> So, this will give access back to the Authenticated Users identity and
> Interactive identity:
> https://technet.microsoft.com/en-us/library/bb726982.aspx
> 
> Ideally we would want to be strengthening this setting not weakening it.
> 
> Is there a way we can get the parent process to allow IME for the flash
> window?

Cannot.  If TSF works correctly, we may be able to hook IME APIs, but IMM32!CtfImmTIMActivate cannot initialize TSF on this process.

When activating IME, IMM32!CtfImmTIMActivate checks NULL token handle by following code. (This is Windows 10 case)

SID_IDENTIFIER_AUTHORITY authority = SECURITY_NULL_SID_AUTHORITY;
PSID sid;
BOOL member = FALSE
AllocateAndInitializeSid(&authority, 1, SECURITY_INTERACTIVE_RID, 0, 0, 0, 0, 0, 0, 0, &sid);
CheckTokenMembershipEx(NULL, sid, CTMF_INCLUDE_APPCONTAINER, &member);
if (!member) {
  // ERROR
}

When USER_INTERACTIVE, member is always FALSE.

Comment 25

[Bob Owen (:bobowen)]

(In reply to Makoto Kato [:m_kato] from comment #24)

> Cannot.  If TSF works correctly, we may be able to hook IME APIs, but
> IMM32!CtfImmTIMActivate cannot initialize TSF on this process.

I know virtually nothing about IME or TSF, so sorry for all the questions ...

Is TSF initialisation a one off thing per process?
If it is could we possibly initialise it ourselves before we lower the sandbox (and restrict the Interactive identity)?

Comment 26

[Makoto Kato [:m_kato]]

(In reply to Bob Owen (:bobowen) from comment #25)
> (In reply to Makoto Kato [:m_kato] from comment #24)
> 
> > Cannot.  If TSF works correctly, we may be able to hook IME APIs, but
> > IMM32!CtfImmTIMActivate cannot initialize TSF on this process.
> 
> I know virtually nothing about IME or TSF, so sorry for all the questions ...
> 
> Is TSF initialisation a one off thing per process?

Yes.  TSF/IME is implemented by DLL, so it is initialized per process and window. 


> If it is could we possibly initialise it ourselves before we lower the
> sandbox (and restrict the Interactive identity)?

Humm, TSF/IMM32 sometimes seems to check SID.  So it may not work...

Also, this initialization is the first time call of SetFocus() via PluginInstanceChild::AnswerSetPluginFocus().

Comment 27

[Masatoshi Kimura [:emk]]

How about forcing windowless mode on Win64 Fx? We only support Flash anyway.

Comment 28

[Makoto Kato [:m_kato]]

(In reply to Masatoshi Kimura [:emk] from comment #27)
> How about forcing windowless mode on Win64 Fx? We only support Flash anyway.

Yes.  This issue doesn't occur on windowless mode.

Also, if we change sandbox level to 2 on x86 firefox, this issue occurs.

Comment 29

[Masatoshi Kimura [:emk]]

Attachment: Force windowless plugin mode if the sandbox level >= 2

Like this.

Comment 30

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

Hmm, Flash Player cannot handle composition string inline in windowless mode due to not accessible to our IMC from the plugin process. So, using windowless mode causes big regression for users, especially for Nico Nico Doga users.

And IIRC, flash player doesn't handle wheel messages in windowless mode. So, I guess that it causes breaking some flash contents.

Comment 31

[Bob Owen (:bobowen)]

Comment on attachment 8658522 [details] [diff] [review]
Use sandbox::USER_NON_ADMIN as access token level for IME

Cancelling this for the moment, while we're working out what is feasible.

Comment 32

[Makoto Kato [:m_kato]]

Comment on attachment 8660715 [details] [diff] [review]
Force windowless plugin mode if the sandbox level >= 2

But, does force window less mode causes break plugin content except to Nakano-san's comment?

Comment 33

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> Hmm, Flash Player cannot handle composition string inline in windowless mode
> due to not accessible to our IMC from the plugin process. So, using
> windowless mode causes big regression for users, especially for Nico Nico
> Doga users.

I filed as Bug 1208944.  I am writing a concept code using hook IMM32 APIs.

Comment 34

[Makoto Kato [:m_kato]]

Comment on attachment 8660715 [details] [diff] [review]
Force windowless plugin mode if the sandbox level >= 2

Since sandbox of plugin process is too harder setting (level is 2 on Win64, and it cannot change by preference), it cannot load IME modules on plugin process due to security check of TSF DLL.  It mean that windowed plugin cannot use IME.

Of course, if sandbox level changes to weak, windowed plugin can use IME.

So :emk-san suggests all flash plug uses windows less.  How do you think this change?  acceptable fix?

Comment 35

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8660715 [details] [diff] [review]
Force windowless plugin mode if the sandbox level >= 2

I don't think I'm familiar enough with the nuances between windowed and windowless mode to make this call. Redirecting to bsmedberg.

Comment 36

[Benjamin Smedberg]

We've thought about forcing windowless for all Flash for a while. Doing this for win64 first sounds like a good idea, since the primary reason to keep using windowed mode was IMEs/accessibility, and those no longer work because of the sandbox anyway.

Comment 37

[Benjamin Smedberg]

Comment on attachment 8660715 [details] [diff] [review]
Force windowless plugin mode if the sandbox level >= 2

But we should default to "opaque" instead of "transparent" by default, if the site hasn't specified transparent. Transparent plugins require extra memory and compositor work to render.

Here's the forcing should work (please let me know if you disagree):

page specified wmode:    forced wmode:
<missing>                opaque
transparent              transparent
<anything else>          opaque

Comment 38

[Makoto Kato [:m_kato]]

Various tests use windowed plugin and these depend on it.  So I must also fix tests.

Comment 39

[Masatoshi Kimura [:emk]]

Why the x86 bug blocks this bug? We don't have to support random plug-ins on Win64. We should deliver wheel messages only to Flash first.

Comment 40

[Makoto Kato [:m_kato]]

Attachment: MozReview Request: Bug 1201904 - Force windowless mode for Flash when sandbox level >= 2. r=bsmedberg

Bug 1201904 - Force windowless mode for Flash when sandbox level >= 2. r?bsmedberg

Comment 41

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

(In reply to Masatoshi Kimura [:emk] from comment #39)
> Why the x86 bug blocks this bug? We don't have to support random plug-ins on
> Win64. We should deliver wheel messages only to Flash first.

Not only Win32. There is no way to specify x86 and x86_64.

Although, the patches sent WM_MOUSE*WHEEL messages to all plugins when they're windowless, but I believe that they don't cause any problem because we send them in windowed mode.

I'd like to know we'll uplift the patch for this to Aurora or Beta. At that time, we should uplift bug 376679 too since Nico Nico Doga which is one the most major movie site has a listbox in the player.

Comment 42

[Benjamin Smedberg]

Comment on attachment 8672878 [details]
MozReview Request: Bug 1201904 - Force windowless mode for Flash when sandbox level >= 2. r=bsmedberg

https://reviewboard.mozilla.org/r/21809/#review19995

No tests? Seems like this deserves some sort of test regime. r+ on the code but please add a test before landing. This should be testable at least using testplugin/flashplugin, no?

Comment 43

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

Hmm, if we'll continue to support Silverlight (bug 1225293), IME won't work on it on windowless mode since we don't send IME messages to windowless plugins except Flash Player. Although, I'm not sure that Silverlight can handle IME messages in windowless mode too.

Comment 44

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

FYI: I tried to fix to send IME messages to all windowless plugins in bug 1211824, though.

Comment 45

[Masatoshi Kimura [:emk]]

It would not be a problem because because only Flash will use NPAPI sandbox by default.

Comment 46

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

(In reply to Masatoshi Kimura [:emk] from comment #45)
> It would not be a problem because because only Flash will use NPAPI sandbox
> by default.

Oh, but I don't understand why we don't need to use NPAPI sandbox with Silverlight...

Comment 47

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline 11/21-11/24, JST) from comment #46)
> (In reply to Masatoshi Kimura [:emk] from comment #45)
> > It would not be a problem because because only Flash will use NPAPI sandbox
> > by default.
> 
> Oh, but I don't understand why we don't need to use NPAPI sandbox with
> Silverlight...

Because the pref says so:
https://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js?rev=d87a6c6accf1#1155

Comment 48

[Makoto Kato [:m_kato]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #42)
> Comment on attachment 8672878 [details]
> MozReview Request: Bug 1201904 - Force windowless mode for Flash when
> sandbox level >= 2. r?bsmedberg
> 
> https://reviewboard.mozilla.org/r/21809/#review19995
> 
> No tests? Seems like this deserves some sort of test regime. r+ on the code
> but please add a test before landing. This should be testable at least using
> testplugin/flashplugin, no?

test plugin should be window less only on x64?

Comment 49

[Makoto Kato [:m_kato]]

If test plugin is windowless only.  I should update a lot of tests.

Comment 50

[Masatoshi Kimura [:emk]]

We will need window mode tests if we re-enable Silverlight on Win64.

Comment 51

[Benjamin Smedberg]

The test plugin is in the tree and works in both windowed and windowless mode currently. You can make it do whatever you need it to do for testing purposes. You can reflect any wmode parameters back into JS just to make sure that we are doing the correct translations (opaque/transparent):

http://hg.mozilla.org/mozilla-central/annotate/45273bbed8ef/dom/plugins/test/testplugin/nptest.cpp#l849 and you can add a property to the scriptable object just like these: http://hg.mozilla.org/mozilla-central/annotate/45273bbed8ef/dom/plugins/test/testplugin/nptest.cpp#l826

Comment 52

[Makoto Kato [:m_kato]]

Comment on attachment 8672878 [details]
MozReview Request: Bug 1201904 - Force windowless mode for Flash when sandbox level >= 2. r=bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21809/diff/1-2/

Comment 53

[Makoto Kato [:m_kato]]

Attachment: MozReview Request: Bug 1201904 - Add test for force windowless mode. r?aklotz

Review commit: https://reviewboard.mozilla.org/r/31019/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31019/

Comment 54

[Makoto Kato [:m_kato]]

application/x-shockwave-flash-test detests as Flash to test this.  Also, this requires bug 676828 for test.  (xpcshell test doesn't work when AsyncInit plugin is installed)

Comment 55

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8708234 [details]
MozReview Request: Bug 1201904 - Add test for force windowless mode. r?aklotz

https://reviewboard.mozilla.org/r/31019/#review29277

Comment 56

[Makoto Kato [:m_kato]]

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

Comment 57

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b130f3402e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/a01862fb4e30

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b130f3402e2
https://hg.mozilla.org/mozilla-central/rev/a01862fb4e30

Comment 59

[Jim Mathies [:jimm]]

(In reply to Carsten Book [:Tomcat] from comment #58)
> https://hg.mozilla.org/mozilla-central/rev/5b130f3402e2
> https://hg.mozilla.org/mozilla-central/rev/a01862fb4e30

+ // Force windowless mode (bug 1201904) when sandbox level >= 2

Why don't we force flash to windowless regardless of sandbox level? We eliminate hang problems in the process.

Comment 60

[pxy4cc]

How about this bug now? Is there any solution for it?
Thanks!

Comment 61

[Bob Owen (:bobowen)]

(In reply to pxy4cc from comment #60)
> How about this bug now? Is there any solution for it?
> Thanks!

A fix landed on Firefox 47, so it should be fixed on Aurora (Developer Edition).
Would you mind testing with that?