Delete google_breakpad::ExceptionHandler before calling _exit()

While debugging tdf#129712 on Windows, I saw this sequence:

1. nullptr was dereferenced (the reason for tdf#129712).
2. ExceptionHandler::HandleException was called (in
workdir/UnpackedTarball/breakpad/src/client/windows/handler/exception_handler.cc).
3. It called ExceptionHandler::WriteMinidumpOnHandlerThread.
4. Minidump was created in ExceptionHandler::ExceptionHandlerThreadMain.
5. Document Recovery dialog was shown in Desktop::Exception (in
desktop/source/app/app.cxx).
6. After closing dialog, _exit() was called in Desktop::Exception.
7. All threads except main were terminated.
8. Another access violation was thrown in the "minimal CRT cleanup".
9. ExceptionHandler::HandleException called again.
10. ExceptionHandler::WriteMinidumpOnHandlerThread hung on WaitForSingleObject
because handler thread that should release the semaphore was terminated
already at step 7.

The process had to be killed manually.

This change destroys the breakpad handler at the start of Desktop::Exception,
which de-registers itself (on Windows it uses SetUnhandledExceptionFilter).
Other than preventing the hang, the rationale also is that keeping the handler
after first minidump creation is wrong: even if the second minidump creation
succeeded, uploading it to crashdump server would give not the actual problem,
but some unrelated stack.

Change-Id: If12d0c7db519693f733b5ab3b8a288cef800a149
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/86104
Reviewed-by: Markus Mohrhard <markus.mohrhard@googlemail.com>
Tested-by: Mike Kaganski <mike.kaganski@collabora.com>
(cherry picked from commit 12b5892cf9c78dd917f2e50672cd250478e6c7d6)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/89690
Tested-by: Thorsten Behrens <Thorsten.Behrens@CIB.de>
Reviewed-by: Thorsten Behrens <Thorsten.Behrens@CIB.de>

1 files changed, 4 insertions, 2 deletions

diff --git a/include/desktop/crashreport.hxx b/include/desktop/crashreport.hxx
index 585c0af5e1a9..8235cff03501 100644
--- a/include/desktop/crashreport.hxx
+++ b/include/desktop/crashreport.hxx
@@ -18,6 +18,7 @@
 #include <config_features.h>
 
 // vector not sort the entries
+#include <memory>
 #include <vector>
 #include <string>
 
@@ -46,7 +47,8 @@ public:
 #if HAVE_FEATURE_BREAKPAD
     static void addKeyValue(const OUString& rKey, const OUString& rValue, tAddKeyHandling AddKeyHandling);
 
-    static void storeExceptionHandler(google_breakpad::ExceptionHandler* pExceptionHandler);
+    static void installExceptionHandler();
+    static void removeExceptionHandler();
 
     static bool crashReportInfoExists();
 
@@ -71,7 +73,7 @@ private:
     typedef std::vector<mpair> vmaKeyValues;
     static vmaKeyValues maKeyValues; // used to temporarily save entries before the old info has been uploaded
 
-    static google_breakpad::ExceptionHandler* mpExceptionHandler;
+    static std::unique_ptr<google_breakpad::ExceptionHandler> mpExceptionHandler;
 
     static std::string getIniFileName();
     static void writeCommonInfo();