Closed
Bug 1201904
Opened 9 years ago
Closed 8 years ago
[x86_64][Win] IME is disabled on Flash Player
Categories
(Core Graveyard :: Plug-ins, defect, P2)
Tracking
(firefox41 affected, firefox42 affected, firefox43 affected, firefox47 fixed)
RESOLVED
FIXED
mozilla47
People
(Reporter: masayuki, Assigned: m_kato)
References
()
Details
(4 keywords)
Attachments
(4 files, 1 obsolete file)
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•9 years ago
|
||
Regression from sandboxing. We're not going to block Fx64 release on this.
Blocks: 1184432
Flags: needinfo?(bobowen.code)
Updated•9 years ago
|
Priority: -- → P2
Reporter | ||
Comment 2•9 years ago
|
||
(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.
Flags: needinfo?(benjamin)
Comment 3•9 years ago
|
||
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.
Flags: needinfo?(benjamin)
Reporter | ||
Comment 4•9 years ago
|
||
(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.
Flags: needinfo?(benjamin)
Reporter | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
||
Probably the blocking bug number is incorrect.
Comment 7•9 years ago
|
||
Yes, it will only be available from the "all systems and languages" link.
Flags: needinfo?(benjamin)
Reporter | ||
Comment 8•9 years ago
|
||
(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•9 years ago
|
||
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.
See Also: → 1180684
Assignee | ||
Comment 10•9 years ago
|
||
AddSandboxAllowedFiles kills some local file access... IME on Chrome is handled on render host. So it isn't on sandbox and, flash is PPAPI...
Assignee | ||
Comment 11•9 years ago
|
||
(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.
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Also, I doesn't have Fujitsu's Japanist, so I cannot test this.
Assignee | ||
Comment 14•9 years ago
|
||
This patch may not work on Microsoft IME on Windows 7 because path is different.
Comment 15•9 years ago
|
||
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?
Assignee | ||
Comment 17•9 years ago
|
||
(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•9 years ago
|
||
(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.
Reporter | ||
Comment 19•9 years ago
|
||
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
Flags: needinfo?(masayuki)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → m_kato
Assignee | ||
Comment 20•9 years ago
|
||
[Tracking Requested - why for this release]:
status-firefox41:
--- → affected
status-firefox42:
--- → affected
Assignee | ||
Updated•9 years ago
|
Attachment #8657701 -
Attachment is obsolete: true
Assignee | ||
Comment 21•9 years ago
|
||
hum, I misunderstand. I seem that current sandbox setting is too harder to enable IME on window created by plugin.
Assignee | ||
Comment 22•9 years ago
|
||
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
Attachment #8658522 -
Flags: review?(bobowen.code)
Comment 23•9 years ago
|
||
(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.
Flags: needinfo?(bobowen.code) → needinfo?(m_kato)
Assignee | ||
Comment 24•9 years ago
|
||
(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.
Flags: needinfo?(m_kato)
Comment 25•9 years ago
|
||
(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)?
Flags: needinfo?(m_kato)
Assignee | ||
Comment 26•9 years ago
|
||
(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().
Flags: needinfo?(m_kato)
Comment 27•9 years ago
|
||
How about forcing windowless mode on Win64 Fx? We only support Flash anyway.
Assignee | ||
Comment 28•9 years ago
|
||
(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.
Reporter | ||
Comment 30•9 years ago
|
||
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.
Updated•9 years ago
|
Keywords: regressionwindow-wanted
Comment 31•9 years ago
|
||
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.
Attachment #8658522 -
Flags: review?(bobowen.code)
Assignee | ||
Comment 32•9 years ago
|
||
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?
Attachment #8660715 -
Flags: feedback?(m_kato) → feedback+
Assignee | ||
Comment 33•9 years ago
|
||
(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.
Assignee | ||
Comment 34•9 years ago
|
||
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?
Attachment #8660715 -
Flags: feedback?(aklotz)
Comment 35•9 years ago
|
||
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.
Attachment #8660715 -
Flags: feedback?(aklotz) → feedback?(benjamin)
Comment 36•9 years ago
|
||
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•9 years ago
|
||
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
Attachment #8660715 -
Flags: feedback?(benjamin) → feedback-
Assignee | ||
Comment 38•9 years ago
|
||
Various tests use windowed plugin and these depend on it. So I must also fix tests.
Comment 39•9 years ago
|
||
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.
Assignee | ||
Comment 40•9 years ago
|
||
Bug 1201904 - Force windowless mode for Flash when sandbox level >= 2. r?bsmedberg
Attachment #8672878 -
Flags: review?(benjamin)
Reporter | ||
Comment 41•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8672878 -
Flags: review?(benjamin) → review+
Comment 42•9 years ago
|
||
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?
Reporter | ||
Comment 43•9 years ago
|
||
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.
Reporter | ||
Comment 44•9 years ago
|
||
FYI: I tried to fix to send IME messages to all windowless plugins in bug 1211824, though.
Comment 45•9 years ago
|
||
It would not be a problem because because only Flash will use NPAPI sandbox by default.
Reporter | ||
Comment 46•9 years ago
|
||
(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•9 years ago
|
||
(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
Assignee | ||
Comment 48•9 years ago
|
||
(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?
Flags: needinfo?(benjamin)
Assignee | ||
Comment 49•9 years ago
|
||
If test plugin is windowless only. I should update a lot of tests.
Comment 50•9 years ago
|
||
We will need window mode tests if we re-enable Silverlight on Win64.
Comment 51•9 years ago
|
||
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
Flags: needinfo?(benjamin)
Assignee | ||
Updated•8 years ago
|
Attachment #8672878 -
Attachment description: MozReview Request: Bug 1201904 - Force windowless mode for Flash when sandbox level >= 2. r?bsmedberg → MozReview Request: Bug 1201904 - Force windowless mode for Flash when sandbox level >= 2. r=bsmedberg
Assignee | ||
Comment 52•8 years ago
|
||
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/
Assignee | ||
Comment 53•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31019/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31019/
Attachment #8708234 -
Flags: review?(aklotz)
Assignee | ||
Comment 54•8 years ago
|
||
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)
Depends on: 676828
Comment 55•8 years ago
|
||
Comment on attachment 8708234 [details] MozReview Request: Bug 1201904 - Add test for force windowless mode. r?aklotz https://reviewboard.mozilla.org/r/31019/#review29277
Attachment #8708234 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 56•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c037ce8b8c6d
Comment 57•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b130f3402e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/a01862fb4e30
Comment 58•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5b130f3402e2 https://hg.mozilla.org/mozilla-central/rev/a01862fb4e30
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 59•8 years ago
|
||
(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•8 years ago
|
||
How about this bug now? Is there any solution for it? Thanks!
Comment 61•8 years ago
|
||
(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?
Flags: needinfo?(pxy4cc+firefox)
Updated•7 years ago
|
Flags: needinfo?(pxy4cc+firefox)
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•