Explore brokering Win32 APIs needed for SSL communication in plugin sandbox

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: handyman, Assigned: handyman)

Tracking

Trunk
mozilla60
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: sb+)

Attachments

(11 attachments, 85 obsolete attachments)

4.48 KB, patch
Details | Diff | Splinter Review
1.16 KB, patch
Details | Diff | Splinter Review
14.44 KB, patch
Details | Diff | Splinter Review
2.94 KB, patch
Details | Diff | Splinter Review
5.52 KB, patch
Details | Diff | Splinter Review
24.26 KB, patch
Details | Diff | Splinter Review
58.11 KB, patch
Details | Diff | Splinter Review
74.00 KB, patch
Details | Diff | Splinter Review
49.64 KB, patch
Details | Diff | Splinter Review
7.60 KB, patch
Details | Diff | Splinter Review
3.50 KB, patch
Details | Diff | Splinter Review
As we attempt to strengthen the sandbox settings, we need to broker some Win32 APIs.  This bug is a placeholder to represent work on brokering operations that permit SSL communication in the plugin process.  The idea is to expose a limited amount of the Win32 API to plugins -- just enough to allow SSL communication.  The rest of behavior is undisturbed.  In particular, this means that we will not (presently) attempt to broker non-SSL (SChannel) API usage.  We will also take advantage of the fact that we are limited to supporting Flash -- the API surface is large and a lot of the determination of what needs to be brokered is based on trial and error.
Priority: -- → P1
Whiteboard: sb+
Hey Bob,

This (huge) patch is the meat of the automatic brokering that I've been working on (the others you can ignore for now).  It actually works but I'm still playing with it and, since I haven't added the network APIs yet, it doesn't really fix anything.  I just wanted to get some other eyes on it early in case you have any recommendations going forward.  I see that you aren't accepting feedback requests but this is not at all urgent -- you can take a look (or not) whenever is convenient. 

I've managed to stay away from a lot of the complexity and overhead (no <functional>, for example) but it ain't exactly beginner code.  There are some docs in PluginHooksWin.h that I'm hoping clarify it enough for people to use.  I'd like it to be even simpler (there's still some boilerplate) but I don't see how without adding needless complexity.

The rest of the effort is adding the networking APIs and changing it to use another actor (still currently uses the chrome proc's main actor).
Flags: needinfo?(bobowencode)
I think I see what you're trying to achieve here from a brief look, but I'm not really that familiar with the hooking and brokering we already have in place for NPAPI (just the bit in the parent where we actually interface with the sandbox for the file permissions).
I doubt I'll have the time to get up to speed before I'm on PTO again.

So, I think jimm is better placed to provide feedback here.
Sorry I didn't get to this before.
Flags: needinfo?(bobowencode) → needinfo?(jmathies)
Many thanks Bob.  To be clear, I'm just posting this because it would be obnoxious to roll out with something so big all at once at the last minute.  jimm, I'm sure you don't have much time for this either and that's not a problem.  It'll be a while before the final product is ready anyway.
Flags: needinfo?(jmathies)
OS: Unspecified → Windows
Hardware: Unspecified → All
Version: unspecified → Trunk
Here we go.

Most of the patches in this series should be stable and these opcodes are enough to pass the TestDllInterceptor with the new methods added (patch #10) on my Win 10 machine and on try Windows 7 and 8 but I am still dealing with failures on Win 10 try machines, so I will be adding more opcodes to this patch.
Attachment #8892175 - Attachment is obsolete: true
Attachment #8892176 - Attachment is obsolete: true
Attachment #8892177 - Attachment is obsolete: true
Attachment #8892178 - Attachment is obsolete: true
Attachment #8892179 - Attachment is obsolete: true
Attachment #8892180 - Attachment is obsolete: true
Attachment #8892190 - Attachment is obsolete: true
Oops, that was the old version
Attachment #8926999 - Attachment is obsolete: true
Trivial -- this makes dealing with the DLL interceptor easier in later patches.
Attachment #8927002 - Flags: review?(aklotz)
The idea is that each function that we hook with the nsWindowsDllInterceptor gets a FunctionHook object, constructed statically and listed in sFunctionsToHook (the first example of this is in patch #8).  BasicFunctionHook is the implementation of FunctionHook -- we need FunctionHook because BasicFunctionHook is generic and can't be used directly in an array.

The static HookFunctions sets up the DLL interception.  HookProtectedMode is just copied from some old DLL interceptor calls (taken from PluginModuleChild in patch 8) -- I intend to move it to use the new FunctionHook system at some point.

The only interesting use of templates here is that the mShouldHook function is set to default to AlwaysHook but it represents a test, run by HookFunctions to decide at run time if we should really go through with it.  The field is specialized to change this behavior.  See patch 8 for examples that specialize the test to check the quirks mode.

Note that this object has nothing to do with brokering itself -- it is used by the brokering mechanism in later patches but there are also functions that we intercept that use this class directly (they are intercepted to alter their functionality but they are not brokered).
The (top level) parent/child actors used for brokering run in their own threads on their respective processes.  Specifically, we need to be able to create a thread in the NPAPI process.

This exposed that we never initialize the nsIThreadManager there...
which exposed that we never initialize XPCOM there...
which 'exposed' that we never shut down XPCOM there...
which exposed that a bunch of stuff that is wired into XPCOM shutdown never happens.  Notably, this includes the ClearOnShutdown mechanism [1], which is handled by XPCOM shutdown events [2] and is used all over the place to free static singletons.

XPCOM has two existing initialization functions [3][4] that don't seem to be enforcing a genuine minimal requirement for system init, despite the name.  I've had good success initializing only NS_LogInit and the thread manager in the NPAPI process so that's what I've done.  Not only is it dangerous to do more since weird things can break (at least one of the services in there randomly uses some JS helpers for logging -- invalid in NPAPI proc) but limited init should be good for performance.  If more is needed, it can still be added.

The PluginProcessChild changes can mostly be related to behavior in the GPUProcessChild.

[1] http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/xpcom/base/ClearOnShutdown.h#98
[2] http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/xpcom/build/XPCOMInit.cpp#856
[3] http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/xpcom/build/XPCOMInit.cpp#453
[4] http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/xpcom/build/XPCOMInit.cpp#740
IpdlTuple is used for run-time type validation of (untrusted) IPC objects.  Normally, IPDL would do this for us but, in our case, that would require a different IPDL message for each brokered API.  Instead, we have one IPDL message (in a later patch) that sends an IpdlTuple and returns another one.  The tuple is then accessed by the correct types at compile time, with run-time type guarantees inserted.

The tuple is essentially just an array of Variants (ie unions), where the Variant is over all of the types the IpdlTuple supports.

String memory management is a bit tricky since the Win32 APIs usually use char*s for strings so we have to be careful.  This is why the tuple has specializations that apply to nsDependentCString -- it comes up in a later patch.
This patch sets up the PluginBroker actors on their own threads.  Its kicked off by PluginModuleChromeParent::LoadModule, which already sets up the PluginModuleParent/Child.  After them, it now creates the PluginBrokerParent and sends InitPluginFunctionBroker to the PluginModuleChild, who creates and wires up the PluginBrokerChild.  After that, the threads run autonomously until shutdown.  The shutdown dance that they do is required to thread-safely handle messages still in the queue when ActorDestroy is received.
FunctionBroker is the meat of this series.  Its a subclass of BasicFunctionHook that brokers the method over the FunctionBroker* actors.  It makes sure that all IPC happens on the dedicated brokering thread.  If it fails to broker a method, it runs the local (presumably sandboxed) version.

The system is written is a very declarative style.  Most customization is done via template specialization, which allows us to e.g. generically support marshaling some types and not others since we know which parameters are fixed at compile time (as opposed to early implementations which leaned heavily on MOZ_ASSERT_UNREACHABLE).  These are the major components:

***

FunctionBroker:
Represents the whole brokering system (on both processes).  It has a RequestHandler and a ResponseHandler and a method, RunFunction.  The Handlers are defined below.  RunFunction is called to actually run the method on the server.  However, RunFunction can be specialized to do whatever on the server (parent) side.  See patch 8 for an example.

The FunctionBroker also supports returning the Win32 thread error when appropriate, which is often required for proper API behavior.

RequestHandler/ResponseHandler: 
Represents the two phases of the communication.  Each phase has a Marshal and Unmarshal method.  As usual, the progression of the operation is RequestHandler::Marshal (client parameters), RequestHandler::Unmarshal (server parameters), ResponseHandler::Marshal (server result), ResponseHandler::Unmarshal (client result).  Each handler has an Info object (RequestInfo or ResponseInfo) and two Marshalers (Marshaler and Unmarshaler).  The Info object is where users are expected to put data for a particular brokered API.  The marshalers are (fixed) functionality and are specific to the endpoint.  In cases where specializing the Info is insufficient, API hooks can specialize these Marshal/Unmarshal methods in the handlers.

A common pattern (indeed, practically the only examples I've written) when specializing Marshal/Unmarshal, is to have those methods delegate to another Handler, converting parameters to types that are IPDL-compatible.  This is only needed in complex cases, like when a buffer is given as a void* and an int length -- these two parameters can be used to call a delegate that works with nsCStrings (which we can create from the void* and length).  In cases where types are relatable one-object-to-one-object, its probably better to use the IPCTypeMap (below).   There are many examples of such delegates in patches 8 and 9.

The RequestHandler also has a ShouldBroker function that can be specialized to reject certain parameter combinations (in this case, the un-hooked version of the function is run).  This allows us to e.g. limit brokering to network calls that use SSL.

Note that the ServerCallData object is used in the server-side request phase (ie unmarshaling parameters) -- this is where most dynamic objects need to be created.  The ServerCallData is for tracking those objects and destroying them before we return from the IPDL call.
In the response, parameter # -1 is used for the return value.

RequestInfo/ResponseInfo:
This is where most API-specific brokering behavior will go (unless the handlers use a delegate as explained above).  Hooks can set some parameters to marshal and not others (again, see patches 8 and 9 for examples).  Others can be 'fixed', meaning we only broker if they match a certain value (this is the default ShouldBroker behavior) and, in that case, we don't bother to marshal that particular parameter.

Marshaler:
The marshaler walks over the parameter lists, doing basic type conversion and deciding if it should marshal/unmarshal something.  Its not intended to be specialized.

EndpointHandler:
This is a warehouse for behavior that is special to the endpoint (server/client).  Right now, its just used to Copy() between IPDL types and Win32 API types.  Noteworthy is that various handle types (such as HANDLEs, that typedef void*, and SecHandles, which are two) are 'obfuscated' by a mechanism that returns them to the client as odd numbers.  This assumes they are never naturally odd -- we then use this in later calls to know if a HANDLE or SecHandle is actually from the server process.

IPCTypeMap:
Used to map types to types.  The system will actually convert between the types by calling Copy(dest, src) on the proper EndpointHandler.

***

NOTE:
RequestHandler::CheckFixedParam and RequestInfo::HasFixedValue both use a weird template trick to test the existence of a field.  Consider CheckFixedParam:

>    // If no FixedValue<paramIndex> is defined and equal to FixedType then always pass.
>    template<int paramIndex, typename = int>
>    struct CheckFixedParam
>    {
>      template<typename ParamType>
>      static inline bool Check(const ParamType& param) { return true; }
>    };
>    
>    // If FixedValue<paramIndex> is defined then check equality.
>    template<int paramIndex>
>    struct CheckFixedParam<paramIndex,
>                           decltype(Info::template FixedValue<paramIndex>::value,0)>
>    {
>      template<typename ParamType>
>      static inline bool Check(ParamType& aParam)
>      {
>        return ParameterEquality(aParam,
>                                 Info::template FixedValue<paramIndex>::value);
>      }
>    };

The second type parameter is always 'int' -- which is good since that's what the default will force at all call sites.  In particular, if FixedValue<paramIndex>::value is defined then 'decltype(Info::template FixedValue<paramIndex>::value,0)' is int (since the type of 0 is int and the type of the sequence 'x, y' is the type of 'y').  But if FixedValue... is not defined then that would be a type error.  C++ template rules will pick the most specialized valid version so they pick the FixedValue version if and only if the FixedValue exists.
Migrating hooks for existing functionality.  Note that this includes examples of BasicFunctionHook and FunctionBroker.
New APIs for SSL.  About 8 of these may not be needed but were part of the experiment.
Added DLL Interceptor tests.  This test still fails in Win 10 try builds.
moz.build changes
Attachment #8927006 - Attachment description: 3. FunctionHook uses the DLL interceptor to redirect Win32 calls to a user-supplied function → 3. Add mechanism for automatically hooking DLL functions
Rebase update.  See original comment 10.  This patch covers the missing cases mentioned there.
Attachment #8927001 - Attachment is obsolete: true
Attachment #8927946 - Flags: review?(aklotz)
Trivial -- this makes dealing with the DLL interceptor easier in later patches.
Attachment #8927002 - Attachment is obsolete: true
Attachment #8927002 - Flags: review?(aklotz)
Attachment #8927948 - Flags: review?(aklotz)
See original comment 13.
Attachment #8927006 - Attachment is obsolete: true
Attachment #8927949 - Flags: review?(jmathies)
See original comment 14.
Attachment #8927007 - Attachment is obsolete: true
Attachment #8927952 - Flags: review?(erahm)
Original explanation in comment 15.
Attachment #8927008 - Attachment is obsolete: true
Attachment #8927953 - Flags: review?(jld)
Original comments on patch in comment 16.
Attachment #8927009 - Attachment is obsolete: true
Original comments in comment 17.
Attachment #8927011 - Attachment is obsolete: true
Attachment #8927956 - Flags: review?(jmathies)
@see comment 18
Attachment #8927012 - Attachment is obsolete: true
Attachment #8927957 - Flags: review?(jmathies)
@see comment 19
Attachment #8927013 - Attachment is obsolete: true
Attachment #8927959 - Flags: review?(jmathies)
Added DLL interceptor tests.  All tests are now green on try.
Attachment #8927014 - Attachment is obsolete: true
Attachment #8927960 - Flags: review?(aklotz)
moz.build changes
Attachment #8927961 - Flags: review?(jmathies)
Attachment #8927015 - Attachment is obsolete: true
Comment on attachment 8927949 [details] [diff] [review]
3. Add mechanism for automatically hooking DLL functions

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

Mostly just questions about implementation and nits.

::: dom/plugins/ipc/FunctionHook.cpp
@@ +77,5 @@
> +// So we hook CreateFileA too to use CreateFileW hook.
> +static HANDLE WINAPI
> +CreateFileAHookFn(LPCSTR fname, DWORD access, DWORD share,
> +                  LPSECURITY_ATTRIBUTES security, DWORD creation, DWORD flags,
> +                  HANDLE ftemplate)

nit - if the convention here is to use aVariable, lets use it throughout.

@@ +81,5 @@
> +                  HANDLE ftemplate)
> +{
> +  while (true) { // goto out
> +    // Our hook is for mms.cfg into \Windows\System32\Macromed\Flash
> +    // We don't requrie supporting too long path.

nit - require

@@ +117,5 @@
> +        wcscat(aPath, tempname.get());
> +        ::CoTaskMemFree(path);
> +        return true;
> +    }
> +    ::CoTaskMemFree(path);

standardize use of '::' here, you have mixed use.

@@ +121,5 @@
> +    ::CoTaskMemFree(path);
> +  }
> +
> +  // XP doesn't support SHGetKnownFolderPath and LocalLow
> +  if (!GetTempPathW(aLen, aPath)) {

we don't support Windows XP anymore. No need for this unless you want the fallback for other versions of the os.

@@ +130,5 @@
> +
> +HANDLE WINAPI
> +CreateFileWHookFn(LPCWSTR fname, DWORD access, DWORD share,
> +                  LPSECURITY_ATTRIBUTES security, DWORD creation, DWORD flags,
> +                  HANDLE ftemplate)

nit - variable naming

@@ +135,5 @@
> +{
> +  static const WCHAR kConfigFile[] = L"mms.cfg";
> +  static const size_t kConfigLength = ArrayLength(kConfigFile) - 1;
> +
> +  while (true) { // goto out, in sheep's clothing

I've never seen a while statement used like this. having a hidden loop someone might accidentally call continue on makes me a bit nervous.

::: dom/plugins/ipc/FunctionHook.h
@@ +20,5 @@
> +// "PluginHooks" logging helpers
> +extern mozilla::LazyLogModule sPluginHooksLog;
> +#define HOOK_LOG(lvl, msg) MOZ_LOG(mozilla::plugins::sPluginHooksLog, lvl, msg);
> +inline const char *SuccessMsg(bool aVal) { return aVal ? "succeeded" : "failed";  }
> +

nit - please add a header for this class

@@ +47,5 @@
> +  static void HookFunctions(int aQuirks);
> +
> +#if defined(XP_WIN)
> +  /**
> +   * Hook some kernel32.dll methods.

Maybe make this comment more specific? we're implementing very specific hooks vs. hooking something generic. Looks like this helper is more of a one-off for the config file.

@@ +80,5 @@
> +{
> +public:
> +  BasicFunctionHook(const char* aModuleName,
> +                 const char* aFunctionName, FunctionType* aOldFunction,
> +                 FunctionType* aNewFunction) :

nit - indentation off

@@ +114,5 @@
> +  // True if we have already hooked the function.
> +  bool mIsHooked;
> +
> +  // The name of the module containing the function to hook.  E.g. "user32.dll".
> +  const char* const mModuleName;

why do you store an external pointer to string data here? This seems a bit dangerous.
Attachment #8927949 - Flags: review?(jmathies) → review-
Comment on attachment 8927952 [details] [diff] [review]
4. Init/Shutdown parts of XPCOM needed in plugin process

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

My first question is do you really need XPCOM? Are you just looking to have a thread you tell to run a function periodically? It's possible the ipc threads are good enough for this (I'm not an ipc expert so it's completely reasonable if the answer is no).


If you need xpcom threads my general thought here is we shouldn't be calling shutdown on things that weren't initialized. How hard would it be to cleanup the services you used directly in `PluginProcessChild::Cleanup`? I'm thinking something like:

> nsThreadManager::get().Shutdown();                          // you initialized this
> mozilla::KillClearOnShutdown(ShutdownPhase::ShutdownFinal); // somewhere along the line things were registered
> NS_LogTerm();                                               // you initialized this

If this becomes a quagmire I'm open to re-reviewing the existing patch.

::: dom/plugins/ipc/PluginProcessChild.cpp
@@ +108,5 @@
>  
>      pluginFilename = WideToUTF8(values[0]);
>  
> +    // Even NS_InitMinimalXPCOM's profile is much more than we need in the
> +    // NPAPI process.  We need the nsThreadManager to use nsThread.

You probably want more of what we setup in NS_InitMinimalXPCOM, ie:

> NS_SetMainThread(); // so NS_IsMainThread works
> mozilla::TimeStamp::Startup(); // Pretty sure threading stuff uses this
> NS_LogInit(); // you already have this
> mozilla::LogModule::Init(); // if you want logging

@@ +152,5 @@
> +
> +    // We need to go through the entire XPCOM shutdown procedure in order
> +    // to pick up the events that are hooked to shutdown phases
> +    // (e.g. ClearOnShutdown())
> +    NS_ShutdownXPCOM(nullptr);

Since we're not doing a full xpcom init we probably shouldn't use the shutdown function (the GPU process doesn't), I think you're going to end up chasing a bunch of random crashes.
Attachment #8927952 - Flags: review?(erahm) → feedback+
Attachment #8927948 - Flags: review?(aklotz) → review+
Attachment #8927960 - Flags: review?(aklotz) → review+
Comment on attachment 8927956 [details] [diff] [review]
7. Add mechanism for automatically brokering DLL functions

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

Please file a pi-request for a sec review of this work.

::: dom/plugins/ipc/FunctionBroker.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef dom_plugins_ipc_PluginHooksWin_h
> +#define dom_plugins_ipc_PluginHooksWin_h 1

nit - mixed variable naming conventions throughout

@@ +42,5 @@
> + *
> + * * Intercepting a new Win32 method:
> + * Step 1: Add a typedef or subclass of either FunctionHook (non-brokering) or
> + * FunctionBroker (automatic brokering) to PluginHooksWin.h.
> + * For example: typedef FunctionBroker<decltype(SetCursorPos)> SetCursorPosMHI;

What's MHI stand for here?

@@ +87,5 @@
> + * See e.g. EndpointHandler<CLIENT>::IPCTypeMap<LPOPENFILENAMEW>.
> + */
> +// NOTE FOR DOCS: Delegate rule of thumb seems to be:
> +// * Haz FixedValue? Then include in delegate (isnt needed for check but IS needed for server-side 'copy')
> +// * Only case for leaving out of delegate is ... output parameters (vs input, which is the default, and vs input-output and vs fixed-value)

nit - cleanup needed here

@@ +117,5 @@
> +
> +#else // defined(XP_WIN)
> +
> +// Any methods we hook use the default calling convention.
> +#define HOOK_CALL 

nit - whitespace

@@ +122,5 @@
> +
> +#endif // defined(XP_WIN)
> +
> +inline bool IsOdd(uint64_t val) { return val & 1; }
> +enum Endpoint { SERVER, CLIENT };

nit - comment me

@@ +356,5 @@
> +class IpdlTupleContext
> +{
> +public:
> +  explicit IpdlTupleContext(const IpdlTuple* aTuple, ServerCallData* aScd=nullptr) :
> +    mTuple(aTuple), mScd(aScd) 

nit - whitespace

@@ +552,5 @@
> +
> +  // Specializations of this method may allocate memory for types that need it
> +  // during Unmarshaling.  They record the allocation in the ServerCallData.
> +  // When copying values in the SERVER, we should be sure to carefully validate
> +  // the information that came from the client.

I'd add a strong(er) note here that the client could be compromised and therefore should not be trusted.

@@ +754,5 @@
> +inline void EndpointHandler<SERVER>::Copy(ServerCallData* aScd, LPPRINTDLGW& dest, const IPCPrintDlg& src)
> +{
> +  MOZ_ASSERT(!dest);
> +  dest = aScd->Allocate<PRINTDLGW>();
> +  Copy(dest, src);

Does the networking code need this or is this for older api hooking code? Seems odd that we'd need a printdlg structure in here.

@@ +758,5 @@
> +  Copy(dest, src);
> +}
> +
> +template<>
> +inline void EndpointHandler<SERVER>::Copy(ServerCallData* aScd, PSCHANNEL_CRED& dest, const IPCSchannelCred& src)

What kind of handle is this copying?

@@ +968,5 @@
> +            typename ... VarParams>
> +  static bool UnmarshalParameters(IpdlTupleContext& aUnmarshaledTuple, int aNextTupleIdx,
> +                           VarParam& aFirstParam, VarParams&... aRemainingParams)
> +  {
> +    // TODO: DLP: I currently increment aNextTupleIdx in the method (its a reference).  This is awful.

I noticed this as well, looks a little unreliable. file a follow up on this maybe?

@@ +1004,5 @@
> +};
> +
> +/**
> + * This base stores the RequestHandler's IPCTypeMap.  It really only
> + * exists to circumvent the arbitrary C++ rule (enforced by mingw) forbidding 

nit - ws

@@ +1259,5 @@
> +  bool BrokerCallClient(uint32_t& aWinError, ResultType& aResult, ParamTypes&... aParameters) const;
> +  bool PostToDispatchThread(uint32_t& aWinError, ResultType& aRet, ParamTypes&... aParameters) const;
> +  
> +  static void 
> +  PostToDispatchHelper(const SelfType* bmhi, Monitor* monitor, bool* ok, uint32_t* winErr, ResultType* r, ParamTypes*... p)

nit - ws

@@ +1261,5 @@
> +  
> +  static void 
> +  PostToDispatchHelper(const SelfType* bmhi, Monitor* monitor, bool* ok, uint32_t* winErr, ResultType* r, ParamTypes*... p)
> +  {
> +    *ok = bmhi->BrokerCallClient(*winErr, *r, *p...);

do we need diagnostic asserts on these parameters?
Attachment #8927956 - Flags: review?(jmathies) → review+
Comment on attachment 8927957 [details] [diff] [review]
8. Migrate some previously hooked functions to FunctionHook/Broker

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

::: dom/plugins/ipc/FunctionBrokerIPCUtils.cpp
@@ +10,5 @@
> +
> +namespace mozilla {
> +namespace plugins {
> +  
> +mozilla::LazyLogModule sPluginHooksLog("PluginHooks");

nit - ws

::: dom/plugins/ipc/FunctionBrokerIPCUtils.h
@@ +1,1 @@
> +#ifndef dom_plugins_ipc_functionbrokeripcutils_h 

nit - ws
Attachment #8927957 - Flags: review?(jmathies) → review+
Attachment #8927961 - Flags: review?(jmathies) → review+
Comment on attachment 8927953 [details] [diff] [review]
5. Add IpdlTuple for type-safely marshaling tuples

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

A couple of nits, but looks good otherwise.

::: dom/plugins/ipc/IpdlTuple.h
@@ +3,5 @@
> +
> +#include "mozilla/plugins/FunctionBrokerIPCUtils.h"
> +#include "mozilla/Variant.h"
> +
> +namespace IPC {

This is plugin-specific, so it should be in mozilla::plugins; namespace IPC is for the core IPC code from Chromium.

@@ +59,5 @@
> +    MaybeVariantType& GetVariant() { return value; }
> +    const MaybeVariantType& GetVariant() const { return value; }
> +
> +  private:
> +    MaybeVariantType value;

Style nit: mValue
Attachment #8927953 - Flags: review?(jld) → review+
Comment on attachment 8927959 [details] [diff] [review]
9. Hook functions needed for SSL communication in NPAPI process

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

::: dom/plugins/ipc/FunctionBroker.cpp
@@ +49,5 @@
> +BOOL WINAPI
> +PHGetWindowInfoHook(HWND hWnd, PWINDOWINFO pwi)
> +{
> +  if (!pwi)
> +    return FALSE;

nit - add brackets

@@ +59,5 @@
> +  }
> +
> +  if (!sBrowserHwnd) {
> +    wchar_t szClass[20];
> +    if (GetClassNameW(hWnd, szClass, ArrayLength(szClass)) &&

does this insure null terminating char? wcscmp expects it.

@@ +74,5 @@
> +  GetWindowInfoPtr gwiFunc =
> +    static_cast<GetWindowInfoPtr>(sGetWindowInfoMHI.OriginalFunction());
> +  BOOL result = gwiFunc(hWnd, pwi);
> +  if (sBrowserHwnd && sBrowserHwnd == hWnd)
> +    pwi->rcWindow = pwi->rcClient;

nit - backets

@@ +370,5 @@
> +  if (!success) {
> +    return false;
> +  }
> +  
> +  if (str.Length()) {

nit - ws
Attachment #8927959 - Flags: review?(jmathies) → review+
> nit - if the convention here is to use aVariable, lets use it throughout.
> [...]
That was all cut-and-paste code but you're right anyway.  Cleaned up.

> we don't support Windows XP anymore. No need for this unless you
> want the fallback for other versions of the os.
This is also cut-and-paste code and, while the comment in there seems sure about what the code means, I'm not.  I'm hesitant to remove this without digging into what it does.

> I've never seen a while statement used like this. having a hidden loop
> someone might accidentally call continue on makes me a bit nervous.
This is more cut-and-paste code I think I should leave alone.  I _could_ update this but I'd want an experiment to test it out and I don't have one.

> nit - please add a header for this class
Which class do you mean?

> Maybe make this comment more specific? we're implementing very specific
> hooks vs. hooking something generic. Looks like this helper is more
> of a one-off for the config file.
Done.  FYI, the code is for disabling Flash protected mode (bug 1108035).

> why do you store an external pointer to string data here? This seems
> a bit dangerous.
They are all static C strings but the API doesn't enforce that so I switched to nsCString.
Attachment #8927949 - Attachment is obsolete: true
Attachment #8930252 - Flags: review?(jmathies)
Attachment #8930252 - Flags: review?(jmathies) → review+
(In reply to David Parks (dparks) [:handyman] from comment #42)
update this but I'd want an experiment to test it out and I don't have one.
> 
> > nit - please add a header for this class
> Which class do you mean?
> 

I was thinking FunctionHook might need a header. I think you have pretty good docs written in other location so this probably isn't necessary.
> My first question is do you really need XPCOM? Are you just looking
> to have a thread you tell to run a function periodically? It's
> possible the ipc threads are good enough for this (I'm not an ipc
> expert so it's completely reasonable if the answer is no).

In this case, I do need the thread.  I need the new top-level actor (PFunctionBroker) to run on a different thread than the other top-level actors (PPluginModule) use.  There are both performance issues (the main thread is already very busy) and deadlocks (at least one occasionally during NPP_Shutdown) that come up.

> If you need xpcom threads my general thought here is we shouldn't be
> calling shutdown on things that weren't initialized. How hard would
> it be to cleanup the services you used directly in
> `PluginProcessChild::Cleanup`? I'm thinking something like:

I wasn't sure this was a good idea but if its safe I'll take it.  Done.

> Since we're not doing a full xpcom init we probably shouldn't use the
> shutdown function (the GPU process doesn't), I think you're going to
> end up chasing a bunch of random crashes.

Ditto.  Removed.
Attachment #8927952 - Attachment is obsolete: true
Attachment #8931059 - Flags: review?(erahm)
> A couple of nits, but looks good otherwise.
All fixed.
Attachment #8927953 - Attachment is obsolete: true
Refreshed patch -- see comment 16.
Attachment #8927955 - Attachment is obsolete: true
> Please file a pi-request for a sec review of this work.
Will do.

> nit - mixed variable naming conventions throughout
Cleaned up.

> What's MHI stand for here?
Nothing good.  These docs are a mess of various implementations I've had as the code progressed.  I think the new comments are better.
Horrifyingly, I'm also noticing that I somehow missed that I left this naming convention for most of the objects in this class (GetKeyStateMHI still existed, even in code).  I've gone and renamed everything -- all in patches 8 and 9.  (FYI, MHI stood for MethodHookInfo, which is what FunctionHook used to be called)

> Does the networking code need this or is this for older api hooking code? Seems odd that we'd need a printdlg structure in here.
The PrintDlg hook... I thought I'd cleaned that up.  Its supposed to be just another cut-and-paste job from old code (no brokering).  In a later bug, I plan to redo it to use brokering to fix the print-dialog-window-depth-order issue we picked up a little while ago.

> I noticed this as well, looks a little unreliable. file a follow up on this maybe?
OK.

> What kind of handle is this copying?
SCHANNEL_CREDs are actually Win32 data objects used in the initialization of SSL.  Its used with AcquireCredentialsHandle [1][2]

> do we need diagnostic asserts on these parameters?
This is only used in a call with clearly non-null parameters.  This used to be an anonymous function right next to the call so that was obvious but its not anymore so I added the ASSERTs.  There is no decent way to test the elements of the parameter pack so I didn't bother with that.


In addition to all that, I made some cosmetic changes.  They were:

* I noticed that some stuff had gone into this patch that was removed in patch 9.  I've cleared that out.
(It was everything related to PSecBufferDesc and LPINTERNET_BUFFERS, whose API hooks weren't needed in the end.)
* I also made a minor change to:
inline void EndpointHandler<SERVER>::Copy(ServerCallData* aScd, char*& aDest, const nsDependentCSubstring& aSrc)
It had some extraneous const_cast code in it that had clearly been left over from something else I was doing.
* Finally, in PostToDispatchThread, I changed a stale cut-and-paste comment from:
  // Synchronously run GetFileNameTask from the main thread.
  // Start a semaphore at 0.  We release the semaphore (bringing its
  // count to 1) when the synchronous call is done.
to:
  // Run PostToDispatchHelper on the dispatch thread.  It will notify our
  // waiting monitor when it is done.
* Removed the log handler with this line:
    HOOK_LOG(LogLevel::Verbose, ("Parameter %d: TODO: Figure out why string array doesnt log.", aIndex));
  ...as it was old and that problem with the logging no longer exists (the default handler now works fine for string arrays).

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa374716(v=vs.85).aspx
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/aa379810(v=vs.85).aspx
Attachment #8927956 - Attachment is obsolete: true
Hey jimm, I made two changes that are worth a glance:

1. I added the non-brokered PrintDlgW hook to this patch.  It's adapted from the old PluginModuleChild PrintDlgW code.
2. I re-added InternetErrorDlg (it was one of the APIs I removed by the patch in comment 30).  This enables the test case in bug 1360804 comment 0.  With the current nightly, that example pops up a modal _behind_ the browser (it's properly complaining about an expired cert).  The patch series without the InternetErrorDlg brokering just shows connection failure (since InternetErrorDlg fails to display a message in that case).  With InternetErrorDlg brokered, I see no issues.
Attachment #8927957 - Attachment is obsolete: true
Attachment #8931066 - Flags: feedback?(jmathies)
Refreshing patch but I haven't had a chance to address jimm's comments yet so WIP.
Attachment #8927959 - Attachment is obsolete: true
Attachment #8927961 - Attachment is obsolete: true
Comment on attachment 8931059 [details] [diff] [review]
4. Init/Shutdown parts of XPCOM needed in plugin process

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

Thanks that's definitely cleaner. Just make sure you test debug / asan / Valgrind builds before landing.
Attachment #8931059 - Flags: review?(erahm) → review+
Comment on attachment 8931063 [details] [diff] [review]
6. Start/stop new top-level brokering actors on their own threads

See comment 16
Attachment #8931063 - Flags: review?(gkrizsanits)
Comment on attachment 8931063 [details] [diff] [review]
6. Start/stop new top-level brokering actors on their own threads

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

I still have some questions / nits so let's do one more round before r+.

::: dom/plugins/ipc/FunctionBrokerChild.cpp
@@ +29,5 @@
> +  PluginDispatchThread()
> +  {
> +    MOZ_RELEASE_ASSERT(NS_IsMainThread());
> +    mThread = nullptr;
> +    if (NS_FAILED(NS_NewNamedThread("Plugin Dispatch", getter_AddRefs(mThread)))) {

If you do this thread creation/init in the ctor and it fails for some reason, then there is no good way to detect/report/handle the error.

In general, what's the strategy if something fails here?

@@ +46,5 @@
> +private:
> +  nsCOMPtr<nsIThread> mThread;
> +};
> +
> +static FunctionBrokerChild* sInstance = nullptr;

This should be a static member of FunctionBrokerChild.

@@ +62,5 @@
> +  mPDThread->Dispatch(Move(runnable));
> +}
> +
> +/* static */ void
> +FunctionBrokerChild::CreateInstance(Endpoint<PFunctionBrokerChild>&& aBrokerEndpoint)

Please don't call this CreateInstance, it has a very specific meaning for XPCOM/COM and this is not an XPCOM component right? Also this is a singleton so we don't really create an instance here, except for the first call. It should be something like start/init/etc and it should probably return bool or nsresult since the thread creation can fail which would make the whole broker completely broken.

@@ +65,5 @@
> +/* static */ void
> +FunctionBrokerChild::CreateInstance(Endpoint<PFunctionBrokerChild>&& aBrokerEndpoint)
> +{
> +  if (!XRE_IsPluginProcess()) {
> +    MOZ_ASSERT_UNREACHABLE("FunctionBrokerChild can only be created in plugin processes");

Why not MOZ_RELEASE_ASSERT(XRE_IsPluginProcess(), "FunctionBrokerChild can only be created in plugin processes") ?

@@ +74,5 @@
> +  sInstance = new FunctionBrokerChild(Move(aBrokerEndpoint));
> +}
> +
> +/* static */ FunctionBrokerChild*
> +FunctionBrokerChild::GetInstance()

Do you need this for something?

@@ +96,5 @@
> +}
> +
> +FunctionBrokerChild::~FunctionBrokerChild()
> +{
> +  delete mPDThread;

Could you use nsAutoPtr for this?

@@ +147,5 @@
> +  // safely delete the actor.
> +  {
> +    MonitorAutoLock lock(sInstance->mMonitor);
> +    while (!sInstance->mShutdownDone) {
> +      // Release lock and wait.  Regain lock when we are notified that

I'm not sure I get this. If you call lock here and then wait. ShutdownOnDispatchThread gets called from the other thread and the first thing it tries to do is a lock as well, which seems like a deadlock to me. Am I missing something?

::: dom/plugins/ipc/FunctionBrokerParent.cpp
@@ +10,5 @@
> +namespace mozilla {
> +namespace plugins {
> +
> +#if defined(XP_WIN)
> +UlongPairToIdMap sPairToIdMap;

These statics should belong to the next patch I guess, also probably the best if they belong to some class.

@@ +16,5 @@
> +PtrToIdMap sPtrToIdMap;
> +IdToPtrMap sIdToPtrMap;
> +#endif // defined(XP_WIN)
> +
> +class PluginBrokerThread

Isn't this the same as PluginDispatchThread? If so, could we use the same class for both?

@@ +63,5 @@
> +FunctionBrokerParent::~FunctionBrokerParent()
> +{
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)
> +  MOZ_RELEASE_ASSERT(NS_IsMainThread());
> +  RemovePermissionsForProcess(OtherPid());

I don't understand why is this needed here. Could you explain/comment it?

@@ +65,5 @@
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)
> +  MOZ_RELEASE_ASSERT(NS_IsMainThread());
> +  RemovePermissionsForProcess(OtherPid());
> +#endif
> +  delete mPBThread;

nsAutoPtr here as well please.

@@ +89,5 @@
> +      return IPC_OK();
> +  }
> +  return IPC_FAIL_NO_REASON(this);
> +#else
> +  MOZ_ASSERT_UNREACHABLE("BrokerFunction is currently only implemented on Windows.");

Why not ifdef-ing out the entire classes?

@@ +113,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aInst);
> +
> +  {
> +    // Hold the lock while we destroy the actor on the broker thread.

Potentially the same deadlock case as above?

@@ +120,5 @@
> +      "FunctionBrokerParent::ShutdownOnBrokerThread", aInst,
> +      &FunctionBrokerParent::ShutdownOnBrokerThread));
> +
> +    // Wait for broker thread to complete destruction.
> +    while (!aInst->mShutdownDone) {

There is only one place you notify monitor right? So why is the while needed when you call wait with infinite timeout?

@@ +136,5 @@
> +}
> +
> +
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)
> +mozilla::SandboxPermissions sSandboxPermissions;

This should be member as well.

@@ +140,5 @@
> +mozilla::SandboxPermissions sSandboxPermissions;
> +
> +// static
> +void
> +FunctionBrokerParent::RemovePermissionsForProcess(base::ProcessId aClientId)

It's still not clear to me what is the role of this.

@@ +162,5 @@
> +
> +  // Check that the FunctionHook array is in the same order as the
> +  // FunctionHookId enum.
> +  MOZ_ASSERT(sFunctionsToHook[aFunctionId]->FunctionId() == aFunctionId);
> +  return sFunctionsToHook[aFunctionId]->RunOriginalFunction(aClientId, aInTuple,

Does this part belong to another patch? Where is sFunctionsToHook defined?

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +752,5 @@
> +PluginModuleChild::RecvInitPluginFunctionBroker(Endpoint<PFunctionBrokerChild>&& aEndpoint)
> +{
> +#if defined(XP_WIN)
> +    MOZ_ASSERT(mIsChrome);
> +    FunctionBrokerChild::CreateInstance(Move(aEndpoint));

If creating/setting up the thread here fails for some reason, should we crash the plugin process?

@@ +755,5 @@
> +    MOZ_ASSERT(mIsChrome);
> +    FunctionBrokerChild::CreateInstance(Move(aEndpoint));
> +    return IPC_OK();
> +#else
> +    return IPC_FAIL(this, "InitPluginFunctionBroker not supported on this platform.");

Returning false here is a crash, is that intentional?

@@ +805,5 @@
>      }
>  
> +#if defined(XP_WIN)
> +    FunctionBrokerChild::Destroy();
> +    FunctionHook::ClearDllInterceptorCache();

Where is ClearDllInterceptorCache defined?

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +496,5 @@
> +    Endpoint<PFunctionBrokerChild> brokerChildEnd;
> +    rv = PFunctionBroker::CreateEndpoints(base::GetCurrentProcId(), parent->OtherPid(),
> +                                        &brokerParentEnd, &brokerChildEnd);
> +    if (NS_FAILED(rv)) {
> +        parent->mShutdown = true;

Won't this leave a zombie process around?

@@ +500,5 @@
> +        parent->mShutdown = true;
> +        return nullptr;
> +    }
> +
> +    parent->mBrokerParent = new FunctionBrokerParent(Move(brokerParentEnd));

We should probably handle here if something went wrong with the thread creation/setup.

::: dom/plugins/ipc/PluginModuleParent.h
@@ +597,5 @@
>       */
>      void
>      FinishHangUI();
> +
> +    FunctionBrokerParent* mBrokerParent;

nsAutoPtr
Attachment #8931063 - Flags: review?(gkrizsanits)
Comment on attachment 8931066 [details] [diff] [review]
8. Migrate some previously hooked functions to FunctionHook/Broker (r=jimm)

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

::: dom/plugins/ipc/FunctionBroker.cpp
@@ +35,5 @@
> +template<>
> +ShouldHookFunc* const
> +GetWindowInfoFH::mShouldHook = &CheckQuirks<QUIRK_FLASH_HOOK_GETWINDOWINFO>;
> +
> +static const wchar_t * kMozillaWindowClass = L"MozillaWindowClass";

might be able to get at this with an extern, I think we define this down in widget.

@@ +36,5 @@
> +ShouldHookFunc* const
> +GetWindowInfoFH::mShouldHook = &CheckQuirks<QUIRK_FLASH_HOOK_GETWINDOWINFO>;
> +
> +static const wchar_t * kMozillaWindowClass = L"MozillaWindowClass";
> +static HWND sBrowserHwnd = nullptr;

no need to assign here.

@@ +47,5 @@
> +
> +BOOL WINAPI
> +GetWindowInfoHook(HWND hWnd, PWINDOWINFO pwi)
> +{
> +  if (!pwi)

nit - braces

@@ +67,5 @@
> +
> +  // Oddity: flash does strange rect comparisons for mouse input destined for
> +  // it's internal settings window. Post removing sub widgets for tabs, touch
> +  // this up so they get the rect they expect.
> +  // XXX potentially tie this to a specific major version?

don't think this is needed, flash updates pretty often and we blocklist old revs pretty quick.

@@ +73,5 @@
> +  GetWindowInfoPtr gwiFunc =
> +    static_cast<GetWindowInfoPtr>(sGetWindowInfoFH.OriginalFunction());
> +  BOOL result = gwiFunc(hWnd, pwi);
> +  if (sBrowserHwnd && sBrowserHwnd == hWnd)
> +    pwi->rcWindow = pwi->rcClient;

nit - braces
Attachment #8931066 - Flags: feedback?(jmathies) → feedback+
Comment on attachment 8930195 [details] [diff] [review]
1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering

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

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +1045,5 @@
>            }
> +        } else if (origBytes[nOrigBytes] == 0x8d) {
> +          // LEA reg, addr
> +          if ((origBytes[nOrigBytes + 1] & 0xc0) == 0x0 &&
> +              (origBytes[nOrigBytes + 1] & 0x07) == 0x5) {

s/0xc0/kMaskMod
s/0x07/kMaskRm
Attachment #8930195 - Flags: review?(aklotz) → review+
Previous version was r+ed after comment 42 but this is a pretty substantive change so I'm requesting re-review.  Most of the reason is this from the review in comment 34:

> @@ +114,5 @@
> > +  // True if we have already hooked the function.
> > +  bool mIsHooked;
> > +
> > +  // The name of the module containing the function to hook.  E.g. "user32.dll".
> > +  const char* const mModuleName;
> 
> why do you store an external pointer to string data here? This seems a bit
> dangerous.

Making this an nsCString lead to a cascading static-construction issue that led to a memory leak.  The const char* wasn't dangerous but it was sloppy and the static construction of all of the FunctionHook/FunctionBroker objects was even sloppier (and against the principles of Quantum anyway).  So I've refactored the system to create the FunctionHook/Broker objects on demand (which is actually rare since it only happens when you use Flash).  

In a little more detail, I added FunctionHookArray (FunctionHook::sFunctionHooks), which is an nsTArray that replaces the old global sFunctionsToHook C-Array.  Through this refactor, we eliminate any non-trivial static objects. 
I also added FunctionHook::GetHooks (which creates the singleton) and FunctionHook::AddFunctionHooks, for adding functions we simply hook (as opposed to functions we broker, which come later as FunctionBroker::AddBrokeredFunctionHooks).
The actual construction of the objects for the functions that are hooked/brokered come in later patches (patches 8 and 9).

The rest of the patch is essentially the same as the prior version.
Attachment #8930252 - Attachment is obsolete: true
Attachment #8937837 - Flags: review?(jmathies)
From the old patch review:

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #54)
> Comment on attachment 8931063 [details] [diff] [review]
> 6. Start/stop new top-level brokering actors on their own threads
> 
> Review of attachment 8931063 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/plugins/ipc/FunctionBrokerChild.cpp
> If you do this thread creation/init in the ctor and it fails for some
> reason, then there is no good way to detect/report/handle the error.
> 
> In general, what's the strategy if something fails here?
I've changed this to use a factory method and it bails if the thread is not created (in the parent, it proceeds with no broker -- in the child, it returns IPC_FAIL)

> > +FunctionBrokerChild::GetInstance()
> 
> Do you need this for something?
It is actually used in a later patch (#7).

> ::: dom/plugins/ipc/FunctionBrokerParent.cpp
> @@ +10,5 @@
> > +namespace mozilla {
> > +namespace plugins {
> > +
> > +#if defined(XP_WIN)
> > +UlongPairToIdMap sPairToIdMap;
> 
> These statics should belong to the next patch I guess, also probably the
> best if they belong to some class.
> 
I've moved this and a few other things you pointed out to the proper patch.

> @@ +16,5 @@
> > +PtrToIdMap sPtrToIdMap;
> > +IdToPtrMap sIdToPtrMap;
> > +#endif // defined(XP_WIN)
> > +
> > +class PluginBrokerThread
> 
> Isn't this the same as PluginDispatchThread? If so, could we use the same
> class for both?
Changed both to use FunctionBrokerThread

> ::: dom/plugins/ipc/PluginModuleChild.cpp
> @@ +752,5 @@
> > +PluginModuleChild::RecvInitPluginFunctionBroker(Endpoint<PFunctionBrokerChild>&& aEndpoint)
> If creating/setting up the thread here fails for some reason, should we
> crash the plugin process?
It now RELEASE_ASSERTs that the thread exists.

> > +    return IPC_FAIL(this, "InitPluginFunctionBroker not supported on this platform.");
> 
> Returning false here is a crash, is that intentional?
Yes.  The message is only valid on Windows.

> 
> ::: dom/plugins/ipc/PluginModuleParent.cpp
> Won't this leave a zombie process around?
I've cleaned all of that up.

> ::: dom/plugins/ipc/PluginModuleParent.h
> > +    FunctionBrokerParent* mBrokerParent;
> 
> nsAutoPtr
Its actually freed by calling Destroy.  Its destructor is private.

> > +      // Release lock and wait.  Regain lock when we are notified that
> > +      // we have ShutdownOnDispatchThread.
> > +      sInstance->mMonitor.Wait();
> If you call lock here and then wait. ShutdownOnDispatchThread gets called
> from the other thread and the first thing it tries to do is a lock as
> well, which seems like a deadlock to me.
Ah, this is one of my least favorite of the conventional synchronization primitives, but it does do what the comment says.  Wait() is basically PR_WaitCondVar.  From the docs [1]:

> ** The thread that waits on a condition is blocked in a "waiting on
> ** condition" state until another thread notifies the condition or a
> ** caller specified amount of time expires. The lock associated with
> ** the condition variable will be released, which must have be held
> ** prior to the call to wait.
> **
> ** Logically a notified thread is moved from the "waiting on condition"
> ** state and made "ready." When scheduled, it will attempt to reacquire
> ** the lock that it held when wait was called.

The point is that it _atomically_ releases the lock _and_ waits to be notified, so no one else can sneak in a nofity between the release and the wait.  This release is why it doesn't deadlock.

> There is only one place you notify monitor right? So why is the while
> needed when you call wait with infinite timeout?
Short answer: spurious wakes.  They are a part of life in Windows and POSIX.  This is the standard way to handle them.  For the longer answer, see [2].

[1] https://searchfox.org/mozilla-central/rev/477ac066b565ae0eb3519875581a62dfb1430e98/nsprpub/pr/include/prcvar.h#37
[2] https://en.wikipedia.org/wiki/Spurious_wakeup
Attachment #8931063 - Attachment is obsolete: true
This patch got some 'almost' substantive changes from the refactor but is mostly unchanged.  Things to note:

* FunctionBroker.h docs updated to reflect new architecture.
* LogParameterValue change to avoid string operations when the log is off.
* AddBrokeredFunctionHooks declaration (it adds the FunctionBroker objects to the array, instead of us inserting them as static globals).
* FunctionBrokerParent creates FunctionHook from the correct thread (main thread -- not the message thread).

(I think this is now stable enough for the sec-review you mentioned.)
Attachment #8931065 - Attachment is obsolete: true
Attachment #8937843 - Flags: feedback?(jmathies)
Fixed some of the nits but all were in old code that was just moved and some were things I'd rather not touch without understanding better.

(In reply to Jim Mathies [:jimm] from comment #55)
> Comment on attachment 8931066 [details] [diff] [review]
> 8. Migrate some previously hooked functions to FunctionHook/Broker (r=jimm)
> 
> Review of attachment 8931066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/ipc/FunctionBroker.cpp
> @@ +35,5 @@
> > +template<>
> > +ShouldHookFunc* const
> > +GetWindowInfoFH::mShouldHook = &CheckQuirks<QUIRK_FLASH_HOOK_GETWINDOWINFO>;
> > +
> > +static const wchar_t * kMozillaWindowClass = L"MozillaWindowClass";
> 
> might be able to get at this with an extern, I think we define this down in
> widget.
> 
> @@ +67,5 @@
> > +
> > +  // Oddity: flash does strange rect comparisons for mouse input destined for
> > +  // it's internal settings window. Post removing sub widgets for tabs, touch
> > +  // this up so they get the rect they expect.
> > +  // XXX potentially tie this to a specific major version?
> 
> don't think this is needed, flash updates pretty often and we blocklist old
> revs pretty quick.

This patch just moved that function from PluginModuleChild.cpp [1].

-------------

Some minor but consequential changes crept in:

* Moved BasicFunctionHook-ed functions from FunctionBroker.cpp to FunctionHook.cpp.  FunctionBroker.cpp now only has the brokered methods.
* Added those functions in FunctionHook::AddFunctionHooks instead of the previous statically-defined globals in FunctionBroker.cpp
* FunctionBrokerIPCUtils.h's FormatBlob function : needsEllipsis -> maybeEllipsis to avoid an AppendPrintf bug.  Apparently it does some flawed static analysis to try to optimize varargs behavior and was ending up incorrectly operating on the ?: result.  So I removed the ?: stuff.

[1] https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/dom/plugins/ipc/PluginModuleChild.cpp#1947
Attachment #8931066 - Attachment is obsolete: true
Attachment #8937845 - Flags: feedback?(jmathies)
This patch changed to add only brokered functions in FunctionBroker.cpp (again, the FunctionHook functions were moved to FunctionHook.cpp).

I also needed to broker two new functions : HttpSendRequestExA and InternetErrorDlgW.  HttpSendRequestExA required adding serialization of the LPINTERNET_BUFFERS type as IPCInternetBuffers in FunctionBrokerIPCUtils -- so that code is also new in thie patch.  It is used in upload/download cases (like tinywebgallery [1]).  InternetErrorDlgW was explained in comment 48.

[1] http://www.tinywebgallery.com/en/tfu/web_demo2.php
Attachment #8931067 - Attachment is obsolete: true
Attachment #8937848 - Flags: feedback?(jmathies)
Attachment #8931070 - Attachment is obsolete: true
Comment on attachment 8937841 [details] [diff] [review]
6. Start/stop new top-level brokering actors on their own threads

I'm switching reviewers from Gabor -- and acknowledging that a review will not come any time soon.

The previous review is in comment 54.  I addressed his notes in comment 62.
Attachment #8937841 - Flags: review?(jld)
Blocks: 1426733
Comment on attachment 8937841 [details] [diff] [review]
6. Start/stop new top-level brokering actors on their own threads

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

Looks good to me.
Attachment #8937841 - Flags: review?(jld) → review+
See comment 59 for notes on this version of the patch.
Attachment #8937837 - Attachment is obsolete: true
Attachment #8937837 - Flags: review?(jmathies)
Attachment #8940236 - Flags: review?(jmathies)
Looking for feedback on some mild changes -- see comment 63.
Attachment #8937843 - Attachment is obsolete: true
Attachment #8937843 - Flags: feedback?(jmathies)
Attachment #8940242 - Flags: feedback?(jmathies)
See comment 64.
Attachment #8937845 - Attachment is obsolete: true
Attachment #8937845 - Flags: feedback?(jmathies)
Attachment #8940243 - Flags: feedback?(jmathies)
See comment 65.
Attachment #8937848 - Attachment is obsolete: true
Attachment #8937848 - Flags: feedback?(jmathies)
Attachment #8940244 - Flags: feedback?(jmathies)
Attachment #8937850 - Attachment is obsolete: true
Attachment #8940236 - Flags: review?(jmathies) → review+
Attachment #8940242 - Flags: feedback?(jmathies) → feedback+
Attachment #8940243 - Flags: feedback?(jmathies) → feedback+
Comment on attachment 8940244 [details] [diff] [review]
9. Hook functions needed for SSL communication in NPAPI process (r=jimm)

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

-  a coupe white space issues here.
Attachment #8940244 - Flags: feedback?(jmathies) → feedback+
Blocks: 1429643
Keywords: checkin-needed
Comment on attachment 8941716 [details] [diff] [review]
8. Migrate some previously hooked functions to FunctionHook/Broker (r=jimm)

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

jimm is not on the IPC reviewer list for modifying sync-messages.ini, and therefore the hg hook that checks for appropriate reviewing of patches that modify sync-messages.ini is unhappy.  I am adding my r+ here (really kind of an rs+) so my name can go on the patch and pacify the hg hook, assuming we try to land this series of patches before jimm's name gets added.
Attachment #8941716 - Flags: review+
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56a6f6bcc5c5
Part 1 - Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ca7804d848
Part 2 - Allow constructing nsWindowsDllInterceptor from module name r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc95b140fd7
Part 3 - Add mechanism for automatically hooking DLL functions r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8262c37dfac
Part 4 - Init/Shutdown parts of XPCOM needed in plugin process r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6056433a119
Part 5 - Add IpdlTuple for type-safely marshaling tuples r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/7936c75851a2
Part 6 - Start/stop new top-level brokering actors on their own threads r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/e63fa691fa34
Part 7 - Add mechanism for automatically brokering DLL functions r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/77ccb100a258
Part 8 - Migrate some previously hooked functions to FunctionHook/Broker r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf011fc507d
Part 9 - Hook functions needed for SSL communication in NPAPI process r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9cce5779674
Part 10 - Add nsWindowsDllInterceptor tests for the new functions r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3ec37e0dbe
Part 11 - Update moz.build with new files r=jimm
Keywords: checkin-needed
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6159c8eb5442
Backed out 11 changesets for failing mochitest at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:318 r=backout a=backout on a CLOSED TREE
Attachment #8941719 - Attachment is obsolete: true
Trying again
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f531c6a6d8
Part 1 - Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f33ad77c5b4
Part 2 - Allow constructing nsWindowsDllInterceptor from module name r=akoltz
https://hg.mozilla.org/integration/mozilla-inbound/rev/24de376fb860
Part 3 - Add mechanism for automatically hooking DLL functions r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d82d0b69c9e
Part 4 - Init/Shutdown parts of XPCOM needed in plugin process r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/37cad137215f
Part 5 - Add IpdlTuple for type-safely marshaling tuples r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/95701ac611fa
Part 6 - Start/stop new top-level brokering actors on their own threads r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/09cb062d95ee
Part 7 - Add mechanism for automatically brokering DLL functions r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a304ff7873a
Part 8 - Migrate some previously hooked functions to FunctionHook/Broker r=jimm, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0ce4cae588
Part 9 - Hook functions needed for SSL communication in NPAPI process r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f8aba058c1
Part 10 - Add nsWindowsDllInterceptor tests for the new functions r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f447c3bde8
Part 11 - Update moz.build with new files r=jimm
Keywords: checkin-needed
This bug created build bustage on Windows-mingw32 Debug.

https://treeherder.mozilla.org/logviewer.html#?job_id=155645054&repo=mozilla-inbound&lineNumber=31302
Flags: needinfo?(davidp99)
Depends on: 1430586
I know this _used_ to pass mingw.  My error.  I'll try to be more thorough.

The clipboard mochitest failures seem to be exacerbated by this patch but aren't caused by it.  Still, I'm dealing with them in bug 1430586.
Blocks: 1358372
(Belated gratitude to froydnj for stepping in here.)
Attachment #8943480 - Attachment is obsolete: true
crosses fingers
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5ae6398dc52
Part 1 - Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering; r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/2213e9ff5f51
Part 2 - Allow constructing nsWindowsDllInterceptor from module name; r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5596661d5f
Part 3 - Add mechanism for automatically hooking DLL functions; r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa70e4cb93e
Part 4 - Init/Shutdown parts of XPCOM needed in plugin process; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0075f2885b
Part 5 - Add IpdlTuple for type-safely marshaling tuples; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/e442713e9946
Part 6 - Start/stop new top-level brokering actors on their own threads; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e37c536895f
Part 7 - Add mechanism for automatically brokering DLL functions; r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd2318482b5
Part 8 - Migrate some previously hooked functions to FunctionHook/Broker; r=jimm,froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/d66285664674
Part 9 - Hook functions needed for SSL communication in NPAPI process; r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/4db59285a8c8
Part 10 - Add nsWindowsDllInterceptor tests for the new functions; r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/d94c15bb4435
Part 11 - Update moz.build with new files; r=jimm
Keywords: checkin-needed
Depends on: 1433855
Depends on: 1433856
Depends on: 1445471
Depends on: 1462547
Depends on: 1462979
Depends on: 1468283
You need to log in before you can comment on or make changes to this bug.