1382251 - Explore brokering Win32 APIs needed for SSL communication in plugin sandbox

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[David Parks [:handyman]]

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.

Comment 1

[David Parks [:handyman]]

Attachment: WIP: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering

Posting WIPs since this is getting heavy.

Comment 2

[David Parks [:handyman]]

Attachment: WIP: 2. Add DLLInterceptor tests for WinInet + SSPI methods

Comment 3

[David Parks [:handyman]]

Attachment: WIP: 3. Refactor GetKeyState brokering to align with other brokered methods

Comment 4

[David Parks [:handyman]]

Attachment: WIP 4. Move PluginModuleChild into UNIFIED_SOURCES

Comment 5

[David Parks [:handyman]]

Attachment: WIP 5. Refactor DLL interceptor behavior out of PluginModuleChild

Comment 6

[David Parks [:handyman]]

Attachment: WIP 6. Use a data object to specify APIs to intercept in plugin process

Comment 7

[David Parks [:handyman]]

Attachment: WIP 7. Simplify Windows brokering mechanism in plugin process

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).

Comment 8

[Bob Owen (:bobowen)]

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.

Comment 9

[David Parks [:handyman]]

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.

Comment 10

[David Parks [:handyman]]

Attachment: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering

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.

Comment 11

[David Parks [:handyman]]

Attachment: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering

Oops, that was the old version

Comment 12

[David Parks [:handyman]]

Attachment: 2. Allow nsWindowsDllInterceptor constructor to behave like Init()

Trivial -- this makes dealing with the DLL interceptor easier in later patches.

Comment 13

[David Parks [:handyman]]

Attachment: 3. Add mechanism for automatically hooking DLL functions

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).

Comment 14

[David Parks [:handyman]]

Attachment: 4. Init/Shutdown parts of XPCOM needed in plugin process

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

Comment 15

[David Parks [:handyman]]

Attachment: 5. Add IpdlTuple for type-safely marshaling tuples

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.

Comment 16

[David Parks [:handyman]]

Attachment: 6. Start/stop new top-level brokering actors on their own threads

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.

Comment 17

[David Parks [:handyman]]

Attachment: 7. Add mechanism for automatically brokering DLL functions

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.

Comment 18

[David Parks [:handyman]]

Attachment: 8. Migrate some previously hooked functions to FunctionHook/Broker

Migrating hooks for existing functionality.  Note that this includes examples of BasicFunctionHook and FunctionBroker.

Comment 19

[David Parks [:handyman]]

Attachment: 9. Hook functions needed for SSL communication in NPAPI process

New APIs for SSL.  About 8 of these may not be needed but were part of the experiment.

Comment 20

[David Parks [:handyman]]

Attachment: 10. Add nsWindowsDllInterceptor tests for the new functions

Added DLL Interceptor tests.  This test still fails in Win 10 try builds.

Comment 21

[David Parks [:handyman]]

Attachment: 11. Update moz.build with new files

moz.build changes

Comment 22

[David Parks [:handyman]]

Attachment: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering

Rebase update.  See original comment 10.  This patch covers the missing cases mentioned there.

Comment 23

[David Parks [:handyman]]

Attachment: 2. Allow nsWindowsDllInterceptor constructor to behave like Init

Trivial -- this makes dealing with the DLL interceptor easier in later patches.

Comment 24

[David Parks [:handyman]]

Attachment: 3. Add mechanism for automatically hooking DLL functions

See original comment 13.

Comment 25

[David Parks [:handyman]]

Attachment: 4. Init/Shutdown parts of XPCOM needed in plugin process

See original comment 14.

Comment 26

[David Parks [:handyman]]

Attachment: 5. Add IpdlTuple for type-safely marshaling tuples

Original explanation in comment 15.

Comment 27

[David Parks [:handyman]]

Attachment: 6. Start/stop new top-level brokering actors on their own threads

Original comments on patch in comment 16.

Comment 28

[David Parks [:handyman]]

Attachment: 7. Add mechanism for automatically brokering DLL functions

Original comments in comment 17.

Comment 29

[David Parks [:handyman]]

Attachment: 8. Migrate some previously hooked functions to FunctionHook/Broker

@see comment 18

Comment 30

[David Parks [:handyman]]

Attachment: 9. Hook functions needed for SSL communication in NPAPI process

@see comment 19

Comment 31

[David Parks [:handyman]]

Attachment: 10. Add nsWindowsDllInterceptor tests for the new functions

Added DLL interceptor tests.  All tests are now green on try.

Comment 32

[David Parks [:handyman]]

Attachment: 11. Update moz.build with new files

moz.build changes

Comment 33

[David Parks [:handyman]]

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ae31499e4c654525f74dc3d492788aed762971

Comment 34

[Jim Mathies [:jimm]]

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.

Comment 35

[Eric Rahm [:erahm]]

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.

Comment 36

[Jim Mathies [:jimm]]

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?

Comment 37

[Jim Mathies [:jimm]]

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

Comment 38

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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

Comment 39

[Jim Mathies [:jimm]]

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

Comment 40

[David Parks [:handyman]]

Attachment: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering

Refreshing patch.

New tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7362ccbfc59ac7d5c2d0ee44f336d78cf23b9c42&selectedJob=146160154

Comment 41

[David Parks [:handyman]]

Attachment: 2. Allow nsWindowsDllInterceptor constructor to behave like Init (r=aklotz)

Refresh

Comment 42

[David Parks [:handyman]]

Attachment: 3. Add mechanism for automatically hooking DLL functions

> 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.

Comment 43

[Jim Mathies [:jimm]]

(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.

Comment 44

[David Parks [:handyman]]

Attachment: 4. Init/Shutdown parts of XPCOM needed in plugin process

> 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.

Comment 45

[David Parks [:handyman]]

Attachment: 5. Add IpdlTuple for type-safely marshaling tuples (r=jld)

> A couple of nits, but looks good otherwise.
All fixed.

Comment 46

[David Parks [:handyman]]

Attachment: 6. Start/stop new top-level brokering actors on their own threads

Refreshed patch -- see comment 16.

Comment 47

[David Parks [:handyman]]

Attachment: 7. Add mechanism for automatically brokering DLL functions (r=jimm)

> 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

Comment 48

[David Parks [:handyman]]

Attachment: 8. Migrate some previously hooked functions to FunctionHook/Broker (r=jimm)

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.

Comment 49

[David Parks [:handyman]]

Attachment: WIP 9. Hook functions needed for SSL communication in NPAPI process (r=jimm)

Refreshing patch but I haven't had a chance to address jimm's comments yet so WIP.

Comment 50

[David Parks [:handyman]]

Attachment: 10. Add nsWindowsDllInterceptor tests for the new functions (r=aklotz)

Comment 51

[David Parks [:handyman]]

Attachment: 11. Update moz.build with new files (r=jimm)

Comment 52

[Eric Rahm [:erahm]]

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.

Comment 53

[David Parks [:handyman]]

Comment on attachment 8931063 [details] [diff] [review]
6. Start/stop new top-level brokering actors on their own threads

See comment 16

Comment 54

[Gabor Krizsanits (INACTIVE)]

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

Comment 55

[Jim Mathies [:jimm]]

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

Comment 56

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 57

[David Parks [:handyman]]

Attachment: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering (r=aklotz)

Fixed nits and rebased.

Comment 58

[David Parks [:handyman]]

Attachment: 2. Allow nsWindowsDllInterceptor constructor to behave like Init (r=aklotz)

Comment 59

[David Parks [:handyman]]

Attachment: 3. Add mechanism for automatically hooking DLL functions

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.

Comment 60

[David Parks [:handyman]]

Attachment: 4. Init/Shutdown parts of XPCOM needed in plugin process (r=erahm)

Comment 61

[David Parks [:handyman]]

Attachment: 5. Add IpdlTuple for type-safely marshaling tuples (r=jld)

Comment 62

[David Parks [:handyman]]

Attachment: 6. Start/stop new top-level brokering actors on their own threads

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

Comment 63

[David Parks [:handyman]]

Attachment: 7. Add mechanism for automatically brokering DLL functions (r=jimm)

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.)

Comment 64

[David Parks [:handyman]]

Attachment: 8. Migrate some previously hooked functions to FunctionHook/Broker (r=jimm)

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

Comment 65

[David Parks [:handyman]]

Attachment: 9. Hook functions needed for SSL communication in NPAPI process (r=jimm)

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

Comment 66

[David Parks [:handyman]]

Attachment: 10. Add nsWindowsDllInterceptor tests for the new functions (r=aklotz)

Comment 67

[David Parks [:handyman]]

Attachment: 11. Update moz.build with new files (r=jimm)

Comment 68

[David Parks [:handyman]]

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

Comment 69

[David Parks [:handyman]]

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.

Comment 70

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 71

[David Parks [:handyman]]

Attachment: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering (r=aklotz)

Refreshing patches

Comment 72

[David Parks [:handyman]]

Attachment: 2. Allow nsWindowsDllInterceptor constructor to behave like Init (r=aklotz)

Comment 73

[David Parks [:handyman]]

Attachment: 3. Add mechanism for automatically hooking DLL functions

See comment 59 for notes on this version of the patch.

Comment 74

[David Parks [:handyman]]

Attachment: 4. Init/Shutdown parts of XPCOM needed in plugin process (r=erahm)

Comment 75

[David Parks [:handyman]]

Attachment: 5. Add IpdlTuple for type-safely marshaling tuples (r=jld)

Comment 76

[David Parks [:handyman]]

Attachment: 6. Start/stop new top-level brokering actors on their own threads (r=jld)

Comment 77

[David Parks [:handyman]]

Attachment: 7. Add mechanism for automatically brokering DLL functions (r=jimm)

Looking for feedback on some mild changes -- see comment 63.

Comment 78

[David Parks [:handyman]]

Attachment: 8. Migrate some previously hooked functions to FunctionHook/Broker (r=jimm)

See comment 64.

Comment 79

[David Parks [:handyman]]

Attachment: 9. Hook functions needed for SSL communication in NPAPI process (r=jimm)

See comment 65.

Comment 80

[David Parks [:handyman]]

Attachment: 10. Add nsWindowsDllInterceptor tests for the new functions (r=aklotz)

Comment 81

[David Parks [:handyman]]

Attachment: 11. Update moz.build with new files (r=jimm)

Comment 82

[Jim Mathies [:jimm]]

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.

Comment 83

[David Parks [:handyman]]

Attachment: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering (r=aklotz)

Final refresh

Comment 84

[David Parks [:handyman]]

Attachment: 2. Allow nsWindowsDllInterceptor constructor to behave like Init (r=aklotz)

Comment 85

[David Parks [:handyman]]

Attachment: 3. Add mechanism for automatically hooking DLL functions (r=jimm)

Comment 86

[David Parks [:handyman]]

Attachment: 4. Init/Shutdown parts of XPCOM needed in plugin process (r=erahm)

Comment 87

[David Parks [:handyman]]

Attachment: 5. Add IpdlTuple for type-safely marshaling tuples (r=jld)

Comment 88

[David Parks [:handyman]]

Attachment: 6. Start/stop new top-level brokering actors on their own threads (r=jld)

Comment 89

[David Parks [:handyman]]

Attachment: 7. Add mechanism for automatically brokering DLL functions (r=jimm)

Comment 90

[David Parks [:handyman]]

Attachment: 8. Migrate some previously hooked functions to FunctionHook/Broker (r=jimm)

Comment 91

[David Parks [:handyman]]

Attachment: 9. Hook functions needed for SSL communication in NPAPI process (r=jimm)

Comment 92

[David Parks [:handyman]]

Attachment: 10. Add nsWindowsDllInterceptor tests for the new functions (r=aklotz)

Comment 93

[David Parks [:handyman]]

Attachment: 11. Update moz.build with new files (r=jimm)

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

Comment 94

[Nathan Froyd [:froydnj]]

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.

Comment 95

[Pulsebot]

Pushed by [e-mail]:
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

Comment 96

[Pulsebot]

Backout by [e-mail]:
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

Comment 98

[Andreea Pavel [:apavel]]

Backed out for failing mochitest at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:318

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cc3ec37e0dbe422223a699ff76b94cbc11b28b45

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=155642255&repo=mozilla-inbound&lineNumber=6341

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6159c8eb544245f0a3ed8766608202ee72530101

Comment 99

[Intermittent Failures Robot]

6 failures in 788 pushes (0.008 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 6

Platform breakdown:
* linux64: 3
* linux64-qr: 2
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1382251&startday=2018-01-08&endday=2018-01-14&tree=all

Comment 100

[David Parks [:handyman]]

Attachment: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering (r=aklotz)

Oops -- was just a missing #ifdef.

Tests look good now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c479e6ec15fca68fbb6c46b5a3967fb0add06ff

Comment 101

[David Parks [:handyman]]

Attachment: 2. Allow nsWindowsDllInterceptor constructor to behave like Init (r=aklotz)

Comment 102

[David Parks [:handyman]]

Attachment: 3. Add mechanism for automatically hooking DLL functions (r=jimm)

Comment 103

[David Parks [:handyman]]

Attachment: 4. Init/Shutdown parts of XPCOM needed in plugin process (r=erahm)

Comment 104

[David Parks [:handyman]]

Attachment: 5. Add IpdlTuple for type-safely marshaling tuples (r=jld)

Comment 105

[David Parks [:handyman]]

Attachment: 6. Start/stop new top-level brokering actors on their own threads

Comment 106

[David Parks [:handyman]]

Attachment: 7. Add mechanism for automatically brokering DLL functions (r=jimm)

Comment 107

[David Parks [:handyman]]

Attachment: 8. Migrate some previously hooked functions to FunctionHook/Broker (r=jimm)

Comment 108

[David Parks [:handyman]]

Attachment: 9. Hook functions needed for SSL communication in NPAPI process (r=jimm)

Comment 109

[David Parks [:handyman]]

Attachment: 10. Add nsWindowsDllInterceptor tests for the new functions (r=aklotz)

Comment 110

[David Parks [:handyman]]

Attachment: 11. Update moz.build with new files (r=jimm)

Comment 111

[David Parks [:handyman]]

Trying again

Comment 112

[Pulsebot]

Pushed by [e-mail]:
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

Comment 113

[Dorel Luca [:dluca]]

This bug created build bustage on Windows-mingw32 Debug.

https://treeherder.mozilla.org/logviewer.html#?job_id=155645054&repo=mozilla-inbound&lineNumber=31302

Comment 114

[Dorel Luca [:dluca]]

Backed out 11 changesets (bug 1382251) for clipbloard failures, mingw32 bustage

https://treeherder.mozilla.org/logviewer.html#?job_id=157130611&repo=mozilla-inbound&lineNumber=31306

https://treeherder.mozilla.org/logviewer.html#?job_id=157131085&repo=mozilla-inbound&lineNumber=16258

https://hg.mozilla.org/integration/mozilla-inbound/rev/398fb8533bcb1ca48b5977ef04e3efa97b089214

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a5f447c3bde85b6240d7b029edeec6259529d303&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Also, dev shall check the push with their patches for more failures and it would be nice if the reviewer name is already included in the commit messages of the uploaded patches.

Comment 115

[Intermittent Failures Robot]

5 failures in 657 pushes (0.008 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 5

Platform breakdown:
* windows-mingw32-32: 5

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1382251&startday=2018-01-15&endday=2018-01-21&tree=all

Comment 116

[David Parks [:handyman]]

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.

Comment 117

[David Parks [:handyman]]

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84a6066234ad545cb7388a73d9d9785f7ed5cd87&selectedJob=158743372

Comment 118

[David Parks [:handyman]]

Attachment: 1. Add x64 opcodes to nsWindowsDllInterceptor needed for plugin process SSL brokering (r=aklotz)

Added reviewers to comments

Comment 119

[David Parks [:handyman]]

Attachment: 2. Allow nsWindowsDllInterceptor constructor to behave like Init (r=aklotz)

Comment 120

[David Parks [:handyman]]

Attachment: 3. Add mechanism for automatically hooking DLL functions (r=jimm)

Comment 121

[David Parks [:handyman]]

Attachment: 4. Init/Shutdown parts of XPCOM needed in plugin process (r=erahm)

Comment 122

[David Parks [:handyman]]

Attachment: 5. Add IpdlTuple for type-safely marshaling tuples (r=jld)

Comment 123

[David Parks [:handyman]]

Attachment: 6. Start/stop new top-level brokering actors on their own threads (r=jld)

Comment 124

[David Parks [:handyman]]

Attachment: 7. Add mechanism for automatically brokering DLL functions (r=jimm)

Comment 125

[David Parks [:handyman]]

Attachment: 8. Migrate some previously hooked functions to FunctionHook/Broker (r=jimm,froydnj)

(Belated gratitude to froydnj for stepping in here.)

Comment 126

[David Parks [:handyman]]

Attachment: 9. Hook functions needed for SSL communication in NPAPI process (r=jimm)

Comment 127

[David Parks [:handyman]]

Attachment: 10. Add nsWindowsDllInterceptor tests for the new functions (r=aklotz)

Comment 128

[David Parks [:handyman]]

Attachment: 11. Update moz.build with new files (r=jimm)

Comment 129

[David Parks [:handyman]]

crosses fingers

Comment 130

[Pulsebot]

Pushed by [e-mail]:
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

Comment 131

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d5ae6398dc52
https://hg.mozilla.org/mozilla-central/rev/2213e9ff5f51
https://hg.mozilla.org/mozilla-central/rev/5b5596661d5f
https://hg.mozilla.org/mozilla-central/rev/1aa70e4cb93e
https://hg.mozilla.org/mozilla-central/rev/1c0075f2885b
https://hg.mozilla.org/mozilla-central/rev/e442713e9946
https://hg.mozilla.org/mozilla-central/rev/1e37c536895f
https://hg.mozilla.org/mozilla-central/rev/0dd2318482b5
https://hg.mozilla.org/mozilla-central/rev/d66285664674
https://hg.mozilla.org/mozilla-central/rev/4db59285a8c8
https://hg.mozilla.org/mozilla-central/rev/d94c15bb4435