NoIssue - Refactor HTTP and HTTPS namespaces registration

src/plugin/PluginAppNamespaceClient.cpp

Index: src/plugin/PluginAppNamespaceClient.cpp
===================================================================
new file mode 100644
--- /dev/null
+++ b/src/plugin/PluginAppNamespaceClient.cpp
@@ -0,0 +1,93 @@
+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2015 Eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "PluginStdAfx.h"
+
+#include "PluginAppNamespaceClient.h"
+#include "PluginClient.h"
+#include "PluginWbPassThrough.h"
+
+
+typedef PassthroughAPP::CMetaFactory<PassthroughAPP::CComClassFactoryProtocol,WBPassthru> MetaFactory;

[Eric, 2015/05/16 21:00:48]

This definition could go in an anonymous namespace.

+
+
+CPluginAppNamespaceClient::CPluginAppNamespaceClient() : m_spCFHTTP(NULL),  m_spCFHTTPS(NULL)
+{
+  // Should only be called once
+  // We register mime filters here
+  // Register asynchronous protocol
+  CComPtr<IInternetSession> spSession;

[Eric, 2015/05/16 21:00:48]

We're also calling 'CoInternetGetSession' in the destructor, and we're using it
to unregister what we're registering here. It seems that the session ought to be
a member variable.

Also 'spSession' is reminiscent of Hungarian notation, which is in contradiction
to style policy. Might as well take the opportunity to rename it.

+  HRESULT hr = ::CoInternetGetSession(0, &spSession, 0);
+  if (FAILED(hr) || !spSession)
+  {
+    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SESSION, PLUGIN_ERROR_SESSION_GET_INTERNET_SESSION, "MimeClient::CoInternetGetSession failed");
+    return;
+  }
+
+  hr = MetaFactory::CreateInstance(CLSID_HttpProtocol, &m_spCFHTTP);

[Eric, 2015/05/16 21:00:48]

The invocation of 'CreateInstance' starts a memory leak. The pointer assigned to
'm_spCFHTTP' is allocated with 'new' (I read the code in "ProtocolCF.inl") but
not deallocated in the destructor. That's because 'CreateInstance' calls 'new'
internally; the caller of 'CreateInstance' has to call 'delete'.

The fact that the declaration of 'm_spCFHTTP' is 'CComPtr' does not affect this.
The destructor of 'CComPtr' calls 'Release', but that doesn't cause the object
to be deallocated.

This is arguably a design defect (or at least a documentation deficiency) in
PassthroughAPP. Nevertheless, its documentation does say that it "returns a
non-AddRef'ed pointer", so we should not naively assume that COM allocation is
being used here.

+  if (FAILED(hr) || !m_spCFHTTP)
+  {
+    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SESSION, PLUGIN_ERROR_SESSION_CREATE_HTTP_INSTANCE, "MimeClient::CreateInstance failed");
+    return;
+  }
+
+  hr = spSession->RegisterNameSpace(m_spCFHTTP, CLSID_HttpProtocol, L"http", 0, 0, 0);
+  if (FAILED(hr))
+  {
+    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SESSION, PLUGIN_ERROR_SESSION_REGISTER_HTTP_NAMESPACE, "MimeClient::RegisterNameSpace failed");
+    return;
+  }
+
+  hr = MetaFactory::CreateInstance(CLSID_HttpSProtocol, &m_spCFHTTPS);
+  if (FAILED(hr) || !m_spCFHTTPS)
+  {
+    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SESSION, PLUGIN_ERROR_SESSION_CREATE_HTTPS_INSTANCE, "MimeClient::CreateInstance failed");
+    return;

[Eric, 2015/05/16 21:00:48]

If the "http" handler registers correctly, but this one doesn't, we've left this
object in an inconsistent state.

The point here is this constructor masks errors. For the interim, we will still
be masking errors one way or another. We can make progress by masking the errors
when the constructor is called, rather than masking them in the constructor and
letting the object be constructed in an invalid state.

The upshot is that many of the 'return' statements here ought to throw instead.

+  }
+
+  hr = spSession->RegisterNameSpace(m_spCFHTTPS, CLSID_HttpSProtocol, L"https", 0, 0, 0);
+  if (FAILED(hr))
+  {
+    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SESSION, PLUGIN_ERROR_SESSION_REGISTER_HTTPS_NAMESPACE, "MimeClient::RegisterNameSpace failed");
+    return;
+  }
+
+}
+
+
+CPluginAppNamespaceClient::~CPluginAppNamespaceClient()
+{
+  CComPtr<IInternetSession> spSession;
+
+  ::CoInternetGetSession(0, &spSession, 0);
+  if (spSession)
+  {

[Eric, 2015/05/16 21:00:48]

We are manually unregistering both name spaces with identical code, just as we
are registering them above with identical code.

It would be better design to make each of these namespace an instance of a class
that manages the registration. Registration is a resource, and we're using RAII
when appropriate.

+    spSession->UnregisterNameSpace(m_spCFHTTP, L"http");
+    if (m_spCFHTTP != NULL)
+    {
+      m_spCFHTTP.Release();
+      m_spCFHTTP = NULL;
+    }
+    spSession->UnregisterNameSpace(m_spCFHTTPS, L"https");
+    if (m_spCFHTTPS != NULL)
+    {
+      m_spCFHTTPS.Release();
+      m_spCFHTTPS = NULL;
+    }
+
+  }
+}