Run a single FilterEngine instance in a separate process

Shared/AdblockPlusClient.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #include "PluginFilter.h"
 #include "PluginClientFactory.h"
 #include "PluginDictionary.h"
 #include "PluginHttpRequest.h"
 #include "PluginMutex.h"
 #include "PluginClass.h"
 #include "PluginUtil.h"
 
 #include "AdblockPlusClient.h"
 
 namespace
 {
-  // TODO: pipeName, bufferSize, AutoHandle, ReadMessage, WriteMessage, MarshalStrings and UnmarshalStrings are
+  // TODO: GetUserName, pipeName, bufferSize, AutoHandle, ReadMessage, WriteMessage, MarshalStrings and UnmarshalStrings are
   //       duplicated in AdblockPlusEngine. We should find a way to reuse them.
 
-  const std::wstring pipeName = L"\\\\.\\pipe\\adblockplusengine";
+  std::wstring GetUserName()
+  {
+    const DWORD maxLength = UNLEN + 1;
+    std::auto_ptr<wchar_t> buffer(new wchar_t[maxLength]);
+    DWORD length = maxLength;
+    if (!::GetUserName(buffer.get(), &length))
+    {
+      std::stringstream stream;
+      stream << "Failed to get the current user's name (Error code: " << GetLastError() << ")";
+      throw std::runtime_error("Failed to get the current user's name");
+    }
+    return std::wstring(buffer.get(), length);
+  }
+
+  const std::wstring pipeName = L"\\\\.\\pipe\\adblockplusengine_" + GetUserName();
   const int bufferSize = 1024;
 
   class AutoHandle
   {
   public:
     AutoHandle()
     {
     }
 
     AutoHandle(HANDLE handle) : handle(handle)
     {
     }
 
     ~AutoHandle()
     {
       CloseHandle(handle);
     }
 
     HANDLE get()
     {
       return handle;
     }
 
   private:
     HANDLE handle;
-    static int references;
 
     AutoHandle(const AutoHandle& autoHandle);
     AutoHandle& operator=(const AutoHandle& autoHandle);
   };
 
   std::string MarshalStrings(const std::vector<std::string>& strings)
   {
     // TODO: This is some pretty hacky marshalling, replace it with something more robust
     std::string marshalledStrings;
     for (std::vector<std::string>::const_iterator it = strings.begin(); it != strings.end(); it++)
       marshalledStrings += *it + ';';
     return marshalledStrings;
   }
 
   std::vector<std::string> UnmarshalStrings(const std::string& message)
   {
     std::stringstream stream(message);
     std::vector<std::string> strings;
     std::string string;
     while (std::getline(stream, string, ';'))
         strings.push_back(string);
     return strings;
   }
 
   std::string ReadMessage(HANDLE pipe)
   {
     std::stringstream stream;
     std::auto_ptr<char> buffer(new char[bufferSize]);
-    bool hasError;
-    do
+    bool doneReading = false;
+    while (!doneReading)
     {
       DWORD bytesRead;
-      hasError = !ReadFile(pipe, buffer.get(), bufferSize * sizeof(char), &bytesRead, 0);
-      if (hasError && GetLastError() != ERROR_MORE_DATA)
+      if (ReadFile(pipe, buffer.get(), bufferSize * sizeof(char), &bytesRead, 0))
+        doneReading = true;
+      else if (GetLastError() != ERROR_MORE_DATA)
       {
         std::stringstream stream;
         stream << "Error reading from pipe: " << GetLastError();
         throw std::runtime_error(stream.str());
       }
       stream << std::string(buffer.get(), bytesRead);
-    } while (hasError);
+    }
     return stream.str();
   }
 
   void WriteMessage(HANDLE pipe, const std::string& message)
   {
     DWORD bytesWritten;
     if (!WriteFile(pipe, message.c_str(), message.length(), &bytesWritten, 0)) 
       throw std::runtime_error("Failed to write to pipe");
   }
 
   HANDLE OpenPipe(const std::wstring& name)
   {
     if (WaitNamedPipe(name.c_str(), 5000))
       return CreateFile(name.c_str(), GENERIC_READ | GENERIC_WRITE, 0, 0, OPEN_EXISTING, 0, 0);
     return INVALID_HANDLE_VALUE;
   }
 
   void SpawnAdblockPlusEngine()
   {
     std::wstring engineExecutablePath = DllDir() + L"AdblockPlusEngine.exe";
     STARTUPINFO startupInfo = {};
     PROCESS_INFORMATION processInformation = {};
 
     HANDLE token;
     OpenProcessToken(GetCurrentProcess(), TOKEN_DUPLICATE | TOKEN_ADJUST_DEFAULT | TOKEN_QUERY | TOKEN_ASSIGN_PRIMARY, &token);
     HANDLE newToken;
     DuplicateTokenEx(token, 0, 0, SecurityImpersonation, TokenPrimary, &newToken);
 
     if (!CreateProcessAsUser(newToken, 0, const_cast<wchar_t*>(engineExecutablePath.c_str()), 0, 0, 0, 0, 0, 0,

[Wladimir Palant, 2013/05/16 11:52:16]

You are leaking the handles created here: "Handles in PROCESS_INFORMATION must
be closed with CloseHandle when they are no longer needed."

[Felix Dahlke, 2013/05/23 12:31:20]

Done.

                              &startupInfo, &processInformation))
     {
       DWORD error = GetLastError();
       throw std::runtime_error("Failed to start Adblock Plus Engine");
     }
+
+    CloseHandle(processInformation.hProcess);
+    CloseHandle(processInformation.hThread);
   }
 
   HANDLE OpenAdblockPlusEnginePipe()
   {
     try
     {
       HANDLE pipe = OpenPipe(pipeName);
       if (pipe == INVALID_HANDLE_VALUE)
       {
         SpawnAdblockPlusEngine();

[Wladimir Palant, 2013/05/16 11:52:16]

I can't say that I like the logic here, timing assumptions and such...

We need something more in line with the example under
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365592%28v=vs.85%29...:

* Call CreateFile() immediately - if it succeeds then we can just work with the
pipe.
* If we get INVALID_HANDLE_VALUE then the engine is probably not running, start
it. If it's running but not initialized yet - no problem, the engine will exit
if there is a previous instance.
* After starting then engine we need to call WaitNamedPipe() and wait
indefinitely - no assumptions about how long it takes to initialize the engine.
We can occasionally call GetExitCodeProcess() for the engine process - if it
returned with an error code then we probably don't want to wait any longer.

[Oleksandr, 2013/05/16 13:23:22]

The documentation says "If no instances of the specified named pipe exist, the
WaitNamedPipe function returns immediately, regardless of the time-out value."

My understanding is that this is the reason why the approach you've described
won't work. The pipe has to be created, before WaitNamedPipe, since that just
waits for ConnectNamedPipe

On 2013/05/16 11:52:16, Wladimir Palant wrote:

[Felix Dahlke, 2013/05/17 09:11:36]

Yes, Oleksandr is right. We are using WaitNamedPipe when opening it, but here we
need to wait a while for the spawned engine to create the pipe.

[Wladimir Palant, 2013/05/17 10:35:02]

Ok, I wonder whether we can wait for pipe creation somehow... And we probably
want the engine to create it sooner but only start waiting for connections once
everything is initialized.

 
-        int timeout = 5000;
+        int timeout = 10000;
         while ((pipe = OpenPipe(pipeName)) == INVALID_HANDLE_VALUE)
         {
           const int step = 10;
           Sleep(step);
           timeout -= step;
           if (timeout <= 0)
             throw std::runtime_error("Unable to open Adblock Plus Engine pipe");
         }
       }
 
@@ skipping 134 matching lines @@
   cbData = 50;
   status = RegQueryValueEx(hKey, L"Version", NULL, &type, (BYTE*)version, &cbData);
   if (status != 0)
   {
     return 0;
   }
   RegCloseKey(hKey);
   return (int)(version[0] - 48);
 }
 
-std::string CallAdblockPlusEngineProcedure(const std::string& name, const std::vector<std::string>& args)

[Wladimir Palant, 2013/05/16 11:52:16]

Why combine the parameters here? It's a private helper anyway, the callers can
simply create the complete vector that needs to be sent.

[Wladimir Palant, 2013/05/23 14:25:01]

This comment doesn't seem to be addressed.

[Felix Dahlke, 2013/05/23 19:10:44]

Oops, seems I missed this. Addressed now, you're right it's not necessary
anymore (used to marshal things a bit differently before).

+std::string CallAdblockPlusEngineProcedure(const std::vector<std::string>& args)
 {
   AutoHandle pipe(OpenAdblockPlusEnginePipe());
-  std::vector<std::string> strings;
-  strings.push_back(name);
-  for (std::vector<std::string>::const_iterator it = args.begin(); it != args.end(); it++)
-    strings.push_back(*it);
-  WriteMessage(pipe.get(), MarshalStrings(strings));
+  WriteMessage(pipe.get(), MarshalStrings(args));
   return ReadMessage(pipe.get());
 }
 
 bool CAdblockPlusClient::Matches(const std::string& url, const std::string& contentType, const std::string& domain)
 {
   std::vector<std::string> args;
+  args.push_back("Matches");
   args.push_back(url);
   args.push_back(contentType);
   args.push_back(domain);
 
   try
   {
-    std::string response = CallAdblockPlusEngineProcedure("Matches", args);
+    std::string response = CallAdblockPlusEngineProcedure(args);
     return response == "1";
   }
   catch (const std::exception& e)
   {
     DEBUG_GENERAL(e.what());
     return false;
   }
 }
 
 std::vector<std::string> CAdblockPlusClient::GetElementHidingSelectors(std::string domain)
 {
   std::vector<std::string> args;
+  args.push_back("GetElementHidingSelectors");
   args.push_back(domain);
 
   try
   {
-    std::string response = CallAdblockPlusEngineProcedure("GetElementHidingSelectors", args);
+    std::string response = CallAdblockPlusEngineProcedure(args);
     return UnmarshalStrings(response);
   }
   catch (const std::exception& e)
   {
     DEBUG_GENERAL(e.what());
     return std::vector<std::string>();
   }
 }
 
 std::vector<AdblockPlus::SubscriptionPtr> CAdblockPlusClient::FetchAvailableSubscriptions()
@@ skipping 18 matching lines @@
 {
   //TODO: implement this
   return std::vector<AdblockPlus::SubscriptionPtr>();
 }
 
 AdblockPlus::SubscriptionPtr CAdblockPlusClient::GetSubscription(std::string url)
 {
   //TODO: imlement this
   return AdblockPlus::SubscriptionPtr();
 }