1384260 - (win32k-plugin) Remove content process win32k use in plugin module

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement, P1)

Description

[Jim Mathies [:jimm]]

GetKeyState for one, there may be others.

Comment 1

[Jim Mathies [:jimm]]

June is on vacation, she'll be back next week. Lets ask her to do a run with flash, see what shows up and then find a way to get rid of it.

Comment 2

[David Parks [:handyman]]

I've manually gone over the plugin code, as well as done some basic runs collecting stacks, and these are the places that I've found that need work.

GetKeyState:
https://searchfox.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp#2163
masayuki says that this won't be as easy as grabbing the modifiers from the WidgetMouseEvent because synthetic ones don't have them. This will be tricky to get right.

SystemParametersInfo
https://searchfox.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp#2087
https://searchfox.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp#2117
I think these should be pushed to all PluginModuleParents in the chrome process when detected there. The question is where to do that detection. We want to catch the WM_SETTINGCHANGE message, which we already do for MouseScrollHandler::SystemSettings but then we do some stuff with it. We really want to catch it with that because e.g. https://searchfox.org/mozilla-central/source/widget/windows/WinMouseScrollHandler.h#289
OTOH, that setup does a bunch of caching and stuff that looks hard to work around (and we don't want caching). So nsWindow::ProcessMessage would be an option except that it would miss the overrides (which we currently don't get in Flash anyway).

GetForegroundWindow
https://searchfox.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceParent.cpp#1430
GetClassNameW
https://searchfox.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceParent.cpp#1432
This WM_KILLFOCUS stuff can be moved to the PluginInstanceChild and done in the plugin process (sandbox permitting, which I think it does). Note that this is easy as the next thing it does is an IPDL CallNPP_HandleEvent to the PluginInstanceChild. The hard part is figuring out which of the ton of HWNDs they hold is the right one between processes.

RegisterWindowMessage
https://searchfox.org/mozilla-central/source/dom/plugins/base/nsPluginNativeWindowWin.cpp#459
NS_PLUGIN_CUSTOM_MSG_ID is only sent in PluginWindowEvent::Run, which is stale code that can never run in content. So this call can be removed.
Note: PluginWindowEvent is unused. It is (only) created by PluginWndProc, which is unused since e10s was required: https://searchfox.org/mozilla-central/source/dom/plugins/base/nsPluginNativeWindowWin.cpp#183
[PluginWndProcInternal -> ProcessFlashMessageDelayed -> GetPluginWindowEvent]

--

Note that there is a LOT of additional use of Win32 APIs in the code base -- but its a mix of things that only run in the chrome or plugin processes or are stale code for things like in-process plugins, which haven't been supported on Windows in ... a very long time. Given that plugins are being completely removed from the code base soon, we are not cleaning the stale code.

There is also some IME code related to plugins that I need to look into more deeply.

Comment 3

[David Parks [:handyman]]

GetKeyState:
https://searchfox.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp#2163
masayuki says that this won't be as easy as grabbing the modifiers from the WidgetMouseEvent because synthetic ones don't have them. This will be tricky to get right.

We now broker GetKeyState from the plugin process [1] to the chrome process. The message being assembled here just ends up sent to the plugin process so that Flash can handle it [2]. It looks like its fine with moving this code there.

[1] https://searchfox.org/mozilla-central/source/dom/plugins/ipc/FunctionBroker.cpp#218
[2] https://searchfox.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceChild.cpp#2053

Comment 4

[David Parks [:handyman]]

GetForegroundWindow and GetClassNameW are apparently limited to windowed plugins, which we thought were somehow sneaking into runs because we saw them in telemetry but I believe I've confirmed that's not really the case (see Bug 1296400). GetForegroundWindow still gets run (STR: ctrl-tab away, then ctrl-tab back) but it always returns NULL (which is weird -- is that due to the current content sandbox?). So it has been just using that null to skip the fullscreen stuff. All of this can apparently just be removed.

The RegisterWindowMessage call is always done as part of plugin setup so no STR is really needed.

SystemParamtersInfo STR is "any horizontal or vertical scroll-wheel motion over a plugin".

I'm still verifying the GetKeyState stuff.

Comment 5

[David Parks [:handyman]]

Turns out the GetKeyState calls are not live in current code but the circumstances around this are complex and seemingly ad hoc so I don't trust them to stay that way. To be robust to changes, I've moved the GetKeyState calls to the plugin process where they can be filled in because we broker GetKeyState to the parent process.

The SystemParametersInfo case turned out the be the tricky one. The MouseScrollHandler::SystemSettings stuff I had planned to hook into is not fully functional -- for example, I never see the cache call RefreshCache after settings changes. Rather than try to hack something into there, I've ignored the settings overrides stuff (we have always ignored it for plugins) and worked in nsWindow::ProcessMessage. So far so good.

Comment 6

[Jim Mathies [:jimm]]

(In reply to David Parks (dparks) [:handyman] from comment #4)

GetForegroundWindow still gets run (STR: ctrl-tab away, then ctrl-tab back) but it always returns NULL (which is weird -- is that due to the current content sandbox?).

Differences in integrity levels probably? Desktop runs as medium integrity, content process at low so if this happens in content, that would explain the failure.

Comment 7

[David Parks [:handyman]]

Attachment: Bug 1384260: Part 1 - Remove RegisterWindowMessage from Windows plugin content process r=jmathies!

This message was always being created in the constructor for nsPluginNativeWindowWin. We want to remove RegisterWindowMessage from content for sandboxing (win32k-lockdown). The message is only used in the old windowed plugin behavior, which is no longer supported, although the code remains in the code base. I've simply moved RegisterWindowMessage to the windowed plugin path instead of removing it, since there is no need to break it.

Comment 8

[David Parks [:handyman]]

Attachment: Bug 1384260: Part 2 - Remove GetForegroundWindow from Windows plugin code path r=jmathies!

GetForegroundWindow in PluginInstanceParent is used as part of message throttling in windowed plugins -- which we no longer officially support. We need to remove it from normal behavior for sandboxing the content process as part of win32k-lockdown. We are not removing windowed plugin code yet so, rather than break the behavior, I've gated the win32 calls so that they aren't run with windowless plugins.

Note that the original behavior was fine as the sandbox just makes the function return NULL -- but it would still show up in stack analysis so the behavior in this patch is preferred.

Depends on D47935

Comment 9

[David Parks [:handyman]]

Attachment: Bug 1384260: Part 3 - Remove GetKeyState from Windows content proc plugin code r=jmathies!

As part of sandboxing the content process for Windows, we want to remove these calls from it. These instances of GetKeyState should not be in actual use since plugin mouse events are handled through different means. When they come to nsPluginInstanceOwner::ProcessEvent, they come with a pre-filled-in mPluginEvent, and this code is conditional on that not happening. Despite that, I am preserving the existing behavior by moving the GetKeyState calls to the plugin process (where they are brokered to the parent process). This fix is more robust to change than just removing the code would be.

Depends on D47936

Comment 10

[David Parks [:handyman]]

Attachment: Bug 1384260: Part 4 - Remove SystemParametersInfo from Windows content proc plugin code r=jmathies!

We use SystemParametersInfo to get the current system scroll wheel settings when we process scrollwheel movement in the content process. We need to remove SystemParametersInfo for sandboxing the content process so this code changes the plugin behavior to cache the system wheel settings. We do this by 1) sending wheel settings to the plugin whenever a plugin takes focus and 2) forwarding wheel settings update messages from Windows to the currently focused plugin.

Depends on D47937

Comment 11

[David Parks [:handyman]]

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

Comment 12

[David Parks [:handyman]]

FYI, a (difficult to use) example of a plugin with a text field that can be used to test the scroll wheel settings (the SystemParametersInfo case):

http://www.flashkit.com/movies/launcher.php?url=/imagesvr_ce/flashkit/movies/Applications/rss_rdf-theo_the-9778/rss_rdf-theo_the-9778.swf&width=792&height=594&bg=004284

The behavior can be triggered by changing Windows system settings: "Change how far you scroll with the mouse wheel".

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e24292c4767
Part 1 - Remove RegisterWindowMessage from Windows plugin content process r=jmathies
https://hg.mozilla.org/integration/autoland/rev/4e2fa645dfb3
Part 2 - Remove GetForegroundWindow from Windows plugin code path r=jmathies
https://hg.mozilla.org/integration/autoland/rev/1fad5f642dd8
Part 3 - Remove GetKeyState from Windows content proc plugin code r=jmathies
https://hg.mozilla.org/integration/autoland/rev/3106f51fca89
Part 4 - Remove SystemParametersInfo from Windows content proc plugin code r=jmathies

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8e24292c4767
https://hg.mozilla.org/mozilla-central/rev/4e2fa645dfb3
https://hg.mozilla.org/mozilla-central/rev/1fad5f642dd8
https://hg.mozilla.org/mozilla-central/rev/3106f51fca89