Issue #1173 - Default behavior for catch-all blocks

src/plugin/PluginDebug.cpp

 /*
  * 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 "PluginDebug.h"
 #include "Exception.h"
 #include "PluginMutex.h"
 #include "PluginSettings.h"
 
 #if (defined ENABLE_DEBUG_INFO)
-struct EntryPointHandlers
-{
-  static void Unknown(std::string name)
-  {
-    CPluginDebug::Debug("Unknown exception in catch-all for entry point " + name);
-  }
-  static void Exception(std::exception& ex, std::string name)
-  {
-    CPluginDebug::DebugOrdinaryException(ex,
-      PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
-      "std::exception in catch-all for entry point " + name);
-  }
-  static void LogicError(std::logic_error& ex, std::string name)
-  {
-    CPluginDebug::DebugOrdinaryException(ex,
-      PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
-      "std::logic_error in catch-all for entry point " + name);
-  }
-  static void RuntimeError(std::runtime_error& ex, std::string name)
-  {
-    CPluginDebug::DebugOrdinaryException(ex,
-      PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
-      "std::runtime_error in catch-all for entry point " + name);
-  }
-  static void SystemError(std::system_error& ex, std::string name)
-  {
-    CPluginDebug::DebugSystemException(ex,
-      PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
-      "std::system_error in catch-all for entry point " + name);
-  }
-};
-#endif
-
-void EntryPointExceptionDefault(std::string name)

[Oleksandr, 2015/03/05 07:18:55]

I'd prefer 'DefaultExceptionHander' instead. This is not limited in any way to
only entry points.

[Eric, 2015/03/05 14:58:04]

As a small point of language, there's no need to use "Handler" (or "Behavior",
my previous name) as a component of these names. It's completely clear from
context what they're doing, and the extra name doesn't add anything.

I had named this function with "EntryPoint" because entry points are special and
deserve special behavior. They sit at the root of the call stack and must not
rethrow. The policy I'd like to see is that exceptions should be handled
explicitly before they reach the catch-all exception handler of an entry point,
and thus that any exception that _is_ caught there is evidence of missing logic
elsewhere. At the start, this could be as simple as a magic string in the debug
log, but there should be some distinction. Using a separate function also
provides a distinct location to place a breakpoint.

To address your point, I'll add an unadorned 'ExceptionDefault' that can be used
elsewhere. Its log message won't have a special logging annotation. Personally,
I think that the default for non-root exceptions should be to rethrow, in
addition to logging, rather than only logging and thus suppressing the
exception. I've not done that in the present change set, since it's slightly
premature. We can simply add a 'throw' statement with no operand after
'ExceptionDefault' for now to get this behavior.

+namespace {
+  // VS 2012 does not have support for variadic templates, which would eliminate the need for an argument class.
+  struct LogHandlersArguments
+  {
+    bool isEntryPoint;
+    std::string name;
+  };
+
+  std::string DefaultLogMessage(const LogHandlersArguments& x, const std::string &exceptionType)
+  {
+    std::string s = x.isEntryPoint ? "ENTRY POINT '" : "'";
+    s += x.name;
+    s += "' caught default ";
+    s += exceptionType;
+    return s;
+  }
+
+  struct LogHandlers
+  {
+    static void Unknown(const LogHandlersArguments& args)
+    {
+      CPluginDebug::Debug(DefaultLogMessage(args, "Unknown exception"));
+    }
+    static void Exception(const std::exception& ex, LogHandlersArguments& args)
+    {
+      CPluginDebug::DebugOrdinaryException(ex,
+        PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+        DefaultLogMessage(args, "std::exception"));
+    }
+    static void LogicError(const std::logic_error& ex, LogHandlersArguments& args)
+    {
+      CPluginDebug::DebugOrdinaryException(ex,
+        PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+        DefaultLogMessage(args, "std::logic_error"));
+    }
+    static void RuntimeError(const std::runtime_error& ex, LogHandlersArguments& args)
+    {
+      CPluginDebug::DebugOrdinaryException(ex,
+        PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+        DefaultLogMessage(args, "std::runtime_error"));
+    }
+    static void SystemError(const std::system_error& ex, LogHandlersArguments& args)
+    {
+      CPluginDebug::DebugSystemException(ex,
+        PLUGIN_ERROR_ENTRY_POINT, PLUGIN_ERROR_ENTRY_POINT_CATCHALL_EXCEPTION,
+        DefaultLogMessage(args, "std::std::system_error"));
+    }
+  };
+}
+#endif
+
+void EntryPointExceptionDefault(const std::string& name)
 {
 #if (defined ENABLE_DEBUG_INFO)
-  CatchAllVoid<EntryPointHandlers>::Handler(name);
-#endif
-}
-
+  // VS 2012 does not have support for brace initializer lists; otherwise this could be a single line
+  LogHandlersArguments x;
+  x.isEntryPoint = true;
+  x.name = name;
+  CatchAllVoid<LogHandlers>::Handler(x);
+#endif
+}
+
+void ExceptionDefault(const std::string& name)
+{
+#if (defined ENABLE_DEBUG_INFO)
+  LogHandlersArguments x;
+  x.isEntryPoint = false;
+  x.name = name;
+  CatchAllVoid<LogHandlers>::Handler(x);
+#endif
+}
 
 class CPluginDebugLock : public CPluginMutex
 {
 
 private:
 
   static CComAutoCriticalSection s_criticalSectionDebugLock;
 
 public:
 
@@ skipping 14 matching lines @@
 {
   std::string message = description + ", " + ex.code().message() + ", " + ex.what();
   DEBUG_ERROR_LOG(ex.code().value(), errorId, errorSubid, message);
 }
 
 void CPluginDebug::DebugOrdinaryException(const std::exception& ex, int errorId, int errorSubid, const std::string& description)
 {
   std::string message = description + ", " + ex.what();
   DEBUG_ERROR_LOG(0, errorId, errorSubid, message);
 }
-
-
 
 #ifdef ENABLE_DEBUG_INFO
 
 void DebugLegacy(const CString& text, DWORD dwProcessId, DWORD dwThreadId)
 {
 #ifdef USE_CONSOLE
   CONSOLE("%s", CT2A(text.GetString(), CP_UTF8));
 #endif
 
   if (CPluginSettings::HasInstance())
@@ skipping 87 matching lines @@
 {
   CString errorCodeText;
   errorCodeText.Format(L"%u (0x%8.8x)", errorCode, errorCode);
 
   CString finalError = error + L". error=" + errorCodeText;
 
 #ifdef ENABLE_DEBUG_ERROR
   DebugLegacy(finalError, dwProcessId, dwThreadId);
 #endif
 
-  DEBUG_SELFTEST(L"********************************************************************************\n" + finalError + "\ n********************************************************************************")
+  DEBUG_SELFTEST(L"********************************************************************************\n" + finalError + "\n********************************************************************************")
 }
 
 void CPluginDebug::DebugErrorCode(DWORD errorCode, const std::string& error, DWORD processId, DWORD threadId)
 {
   DebugErrorCodeLegacy(errorCode, CString(error.c_str()), processId, threadId);
 }
 
 #endif
 
 // ============================================================================
@@ skipping 80 matching lines @@
     srcTrunc = srcTrunc.Left(67) + L"..." + srcTrunc.Right(30);
   }
 
   CString blocking;
   blocking.Format(L"Ignored  %-12s  %s  %s", ToCString(type), domain.empty()? L"-" : ToCString(domain), srcTrunc);
 
   DebugResultLegacy(blocking);
 }
 
 #endif // ENABLE_DEBUG_RESULT_IGNORED