Rework crash reporting UI

* Encourage users running dev code to submit reports
* Change from "Disable" to "Enable"
* Always initialize Crashpad and just prevent uploads if disabled
* Fix menu not being in sync
* Add documentation
* Keep track of whether Crashpad initialized successfully

interface/src/CrashHandler.h

@@ -21,5 +21,7 @@ Q_DECLARE_LOGGING_CATEGORY(crash_handler)
 bool startCrashHandler(std::string appPath);
 void setCrashAnnotation(std::string name, std::string value);
 void startCrashHookMonitor(QCoreApplication* app);
+void setCrashReportingEnabled(bool value);
+
 
 #endif // hifi_CrashHandler_h

interface/src/CrashHandler_Breakpad.cpp

@@ -81,6 +81,11 @@ void setCrashAnnotation(std::string name, std::string value) {
     flushAnnotations();
 }
 
+void setCrashReportingEnabled(bool value) {
+    qCritical() << "Can't set crash reporting status on Breakpad.";
+    qCritical() << "Breakpad is deprecated and needs replacing!";
+}
+
 void startCrashHookMonitor(QCoreApplication* app) {
 }
 

interface/src/CrashHandler_Crashpad.cpp

@@ -43,6 +43,8 @@ Q_LOGGING_CATEGORY(crash_handler, "overte.crash_handler")
 #include <BuildInfo.h>
 #include <FingerprintUtils.h>
 #include <UUID.h>
+#include <UserActivityLogger.h>
+
 
 static const std::string BACKTRACE_URL{ CMAKE_BACKTRACE_URL };
 static const std::string BACKTRACE_TOKEN{ CMAKE_BACKTRACE_TOKEN };
@@ -134,6 +136,9 @@ bool SpinLockLocker::relock(int msecs /* = -1 */ ) {
 // ------------------------------------------------------------------------------------------------
 
 crashpad::CrashpadClient* client{ nullptr };
+std::unique_ptr< crashpad::CrashReportDatabase > crashpadDatabase;
+
+
 SpinLock crashpadAnnotationsProtect;
 crashpad::SimpleStringDictionary* crashpadAnnotations{ nullptr };
 
@@ -409,14 +414,20 @@ bool startCrashHandler(std::string appPath) {
     base::FilePath handler(handlerPath);
 
     qCDebug(crash_handler) << "Opening crashpad database" << QString::fromStdString(crashpadDbPath);
-    auto database = crashpad::CrashReportDatabase::Initialize(db);
-    if (database == nullptr || database->GetSettings() == nullptr) {
+    crashpadDatabase = crashpad::CrashReportDatabase::Initialize(db);
+    if (crashpadDatabase == nullptr || crashpadDatabase->GetSettings() == nullptr) {
         qCCritical(crash_handler) << "Failed to open crashpad database" << QString::fromStdString(crashpadDbPath);
         return false;
     }
 
     // Enable automated uploads.
-    database->GetSettings()->SetUploadsEnabled(true);
+    QObject::connect(&UserActivityLogger::getInstance(), &UserActivityLogger::crashReportingEnabledChanged, []() {
+        auto &ual = UserActivityLogger::getInstance();
+        setCrashReportingEnabled(ual.isCrashReportingEnabled());
+    });
+
+    crashpadDatabase->GetSettings()->SetUploadsEnabled(UserActivityLogger::getInstance().isCrashReportingEnabled());
+
 
     if (!client->StartHandler(handler, db, db, BACKTRACE_URL, annotations, arguments, true, true)) {
         qCCritical(crash_handler) << "Failed to start crashpad handler";
@@ -432,6 +443,36 @@ bool startCrashHandler(std::string appPath) {
     return true;
 }
 
+void setCrashReportingEnabled(bool enabled) {
+    auto settings = crashpadDatabase->GetSettings();
+    settings->SetUploadsEnabled(enabled);
+
+    if (enabled) {
+        // Enabled now, was disabled before
+        //
+        // Reports are generated while uploads are disabled, so if we didn't do this we could
+        // send something the user doesn't want sent. So if reporting was disabled then enabled,
+        // remove any pending reports.
+        qCDebug(crash_handler) << "Removing any pending crash reports";
+
+        std::vector<crashpad::CrashReportDatabase::Report> pendingReports;
+        crashpad::CrashReportDatabase::OperationStatus status;
+
+        status = crashpadDatabase->GetPendingReports(&pendingReports);
+
+        if (status != crashpad::CrashReportDatabase::kNoError) {
+            qCWarning(crash_handler) << "Failed to get pending reports";
+        } else {
+            for (const auto& report : pendingReports) {
+                qCDebug(crash_handler) << "Deleted crash report" << QString::fromStdString(report.uuid.ToString());
+                crashpadDatabase->DeleteReport(report.uuid);
+            }
+        }
+    }
+
+    qCInfo(crash_handler) << "Crashpad uploads " << (enabled ? QString("enabled") : QString("disabled"));
+}
+
 void setCrashAnnotation(std::string name, std::string value) {
     if (client) {
         SpinLockLocker guard(crashpadAnnotationsProtect);

interface/src/CrashHandler_None.cpp

@@ -31,4 +31,7 @@ void setCrashAnnotation(std::string name, std::string value) {
 void startCrashHookMonitor(QCoreApplication* app) {
 }
 
+void setCrashReportingEnabled(bool value) {
+
+}
 #endif

interface/src/CrashRecoveryHandler.cpp

@@ -27,15 +27,22 @@
 #include "Menu.h"
 
 
+
 #include <RunningMarker.h>
 #include <SettingHandle.h>
 #include <SettingHelpers.h>
 #include <SettingManager.h>
 #include <DependencyManager.h>
 #include <UserActivityLogger.h>
+#include <BuildInfo.h>
+
+ static const QString BACKTRACE_URL{ CMAKE_BACKTRACE_URL };
 
 
 bool CrashRecoveryHandler::checkForResetSettings(bool wasLikelyCrash, bool suppressPrompt) {
+    Setting::Handle<bool> crashReportingAsked { "CrashReportingAsked", false };
+
+
     Settings settings;
     settings.beginGroup("Developer");
     QVariant displayCrashOptions = settings.value(MenuOption::DisplayCrashOptions);
@@ -47,6 +54,7 @@ bool CrashRecoveryHandler::checkForResetSettings(bool wasLikelyCrash, bool suppr
 
     // If option does not exist in Interface.ini so assume default behavior.
     bool displaySettingsResetOnCrash = !displayCrashOptions.isValid() || displayCrashOptions.toBool();
+    bool userPrompted = false;
 
     if (suppressPrompt) {
         return wasLikelyCrash;
@@ -54,18 +62,106 @@ bool CrashRecoveryHandler::checkForResetSettings(bool wasLikelyCrash, bool suppr
 
     if (wasLikelyCrash || askToResetSettings) {
         if (displaySettingsResetOnCrash || askToResetSettings) {
+            userPrompted = true;
             Action action = promptUserForAction(wasLikelyCrash);
             if (action != DO_NOTHING) {
                 handleCrash(action);
             }
         }
     }
+
+    if (!userPrompted) {
+        // Both dialogs share a purpose -- if we already showed the full dialog, no need to show this one.
+        if (BuildInfo::BUILD_TYPE != BuildInfo::BuildType::Stable) {
+            // We didn't crash but are running a development build -- we'd like reports if possible.
+            if (suggestCrashReporting()) {
+                // If suggestCrashReporting returns false, we didn't ask the user.
+                crashReportingAsked.set(true);
+            }
+        }
+
+    }
+
     return wasLikelyCrash;
 }
 
+
+bool CrashRecoveryHandler::suggestCrashReporting() {
+    QDialog crashDialog;
+
+    crashDialog.setWindowTitle("You are running an experimental version of Overte");
+
+    QVBoxLayout* layout = new QVBoxLayout;
+
+    QString explainText;
+    auto &ual = UserActivityLogger::getInstance();
+
+
+
+    switch(BuildInfo::BUILD_TYPE) {
+        case BuildInfo::BuildType::Dev:
+            explainText = "You're running a pre-release version. This is an official release, but the code\n"
+                        "is not yet considered to be fully stable. We'd highly appreciate it if you enabled\n"
+                        "crash reporting to help us test the upcoming release.";
+            break;
+        case BuildInfo::BuildType::PR:
+            // TODO: It would be nice to have here the PR number, and who submitted it. This would require GHA support.
+            explainText = "You're running a PR version. This is experimental code contributed by a third party\n"
+                        "and not yet part of the official code. We'd highly appreciate it if you enabled\n"
+                        "crash reporting to help us test this potential addition.";
+            break;
+        case BuildInfo::BuildType::Master:
+            explainText = "You're running a pre-release version. This is an official release, but the code\n"
+                        "is not yet considered to be fully stable. We'd highly appreciate it if you enabled\n"
+                        "crash reporting to help us test the upcoming release.";
+            break;
+        case BuildInfo::BuildType::Stable:
+            explainText = "You're running a stable version. This is an official release, and should perform\n"
+                          "correctly. Nevertheless, we'd highly appreciate it if you enabled crash reporting\n"
+                          "to help us catch any remaining problems.";
+            break;
+    }
+
+    if (!ual.isCrashMonitorStarted()) {
+        qWarning() << "Crash reporting not working, skipping suggestion to enable it.";
+        return false;
+    }
+
+    QLabel* explainLabel = new QLabel(explainText);
+
+    QLabel* crashReportLabel = new QLabel("Reports can only be seen by members of the Overte e.V. organization and\n"
+                                          "trusted developers, and will only be used for improving the code.");
+
+    QCheckBox* crashReportCheckbox = new QCheckBox("Enable automatic crash reporting");
+
+    crashReportCheckbox->setChecked(ual.isCrashReportingEnabled());
+    crashReportCheckbox->setEnabled(ual.isCrashMonitorStarted());
+
+    layout->addWidget(explainLabel);
+    layout->addSpacing(12);
+    layout->addWidget(crashReportLabel);
+    layout->addWidget(crashReportCheckbox);
+    layout->addSpacing(12);
+    layout->addStretch();
+
+    QDialogButtonBox* buttons = new QDialogButtonBox(QDialogButtonBox::Ok);
+    layout->addWidget(buttons);
+    crashDialog.connect(buttons, SIGNAL(accepted()), SLOT(accept()));
+
+    crashDialog.setLayout(layout);
+
+    crashDialog.exec();
+
+    ual.setCrashReportingEnabled(crashReportCheckbox->isChecked());
+
+    return true;
+}
+
 CrashRecoveryHandler::Action CrashRecoveryHandler::promptUserForAction(bool showCrashMessage) {
     QDialog crashDialog;
     QLabel* label;
+    auto &ual = UserActivityLogger::getInstance();
+
     if (showCrashMessage) {
         crashDialog.setWindowTitle("Interface Crashed Last Run");
         label = new QLabel("If you are having trouble starting would you like to reset your settings?");
@@ -81,17 +177,21 @@ CrashRecoveryHandler::Action CrashRecoveryHandler::promptUserForAction(bool show
     QRadioButton* option1 = new QRadioButton("Reset all my settings");
     QRadioButton* option2 = new QRadioButton("Reset my settings but keep essential info");
     QRadioButton* option3 = new QRadioButton("Continue with my current settings");
+    QLabel* crashReportLabel = nullptr;
 
-    QLabel* crashReportLabel = new QLabel("To help us with debugging, you can enable automatic crash reports.\n"
-                                          "They'll only be seen by members of the Overte e.V. organization and\n"
-                                          "trusted developers, and will only be used for improving the code.");
+    if (ual.isCrashMonitorStarted()) {
+        crashReportLabel = new QLabel("To help us with debugging, you can enable automatic crash reports.\n"
+                                      "They'll only be seen by members of the Overte e.V. organization and\n"
+                                      "trusted developers, and will only be used for improving the code.");
+    } else {
+        crashReportLabel = new QLabel("Unfortunately, crash reporting isn't built into this release.");
+    }
 
     QCheckBox* crashReportCheckbox = new QCheckBox("Enable automatic crash reporting");
 
-    auto &ual = UserActivityLogger::getInstance();
-    crashReportCheckbox->setChecked(ual.isCrashMonitorEnabled());
-
 
+    crashReportCheckbox->setChecked(ual.isCrashReportingEnabled());
+    crashReportCheckbox->setEnabled(ual.isCrashMonitorStarted());
 
     option3->setChecked(true);
     layout->addWidget(option1);
@@ -121,7 +221,7 @@ CrashRecoveryHandler::Action CrashRecoveryHandler::promptUserForAction(bool show
         }
     }
 
-    ual.crashMonitorDisable(!crashReportCheckbox->isChecked());
+    ual.setCrashReportingEnabled(crashReportCheckbox->isChecked());
 
     // Dialog cancelled or "do nothing" option chosen
     return CrashRecoveryHandler::DO_NOTHING;

interface/src/CrashRecoveryHandler.h

@@ -14,6 +14,11 @@
 
 #include <QString>
 
+
+
+
+
+
 class CrashRecoveryHandler {
 
 public:
@@ -26,8 +31,10 @@ private:
         DO_NOTHING
     };
 
+    static bool suggestCrashReporting();
     static Action promptUserForAction(bool showCrashMessage);
     static void handleCrash(Action action);
+
 };
 
 #endif // hifi_CrashRecoveryHandler_h

interface/src/Menu.cpp

@@ -368,7 +368,7 @@ Menu::Menu() {
     // Developer > Scripting > Verbose Logging
     addCheckableActionToQMenuAndActionHash(scriptingOptionsMenu, MenuOption::VerboseLogging, 0, false,
                                            qApp, SLOT(updateVerboseLogging()));
-                                           
+
    // Developer > Scripting > Enable Cachebusting of Script.require
    addCheckableActionToQMenuAndActionHash(scriptingOptionsMenu, MenuOption::CachebustRequire, 0, false,
                                           qApp, SLOT(setCachebustRequire()));
@@ -637,11 +637,12 @@ Menu::Menu() {
         &UserActivityLogger::getInstance(),
         SLOT(disable(bool)));
     addCheckableActionToQMenuAndActionHash(networkMenu,
-        MenuOption::DisableCrashLogger,
+        MenuOption::EnableCrashReporting,
         0,
-        true,
+        UserActivityLogger::getInstance().isCrashReportingEnabled(),
         &UserActivityLogger::getInstance(),
-        SLOT(crashMonitorDisable(bool)));
+        SLOT(setCrashReportingEnabled(bool)));
+
     addActionToQMenuAndActionHash(networkMenu, MenuOption::ShowDSConnectTable, 0,
         qApp, SLOT(loadDomainConnectionDialog()));
 

interface/src/Menu.h

@@ -90,7 +90,7 @@ namespace MenuOption {
     const QString DeleteAvatarEntitiesBookmark = "Delete Avatar Entities Bookmark";
     const QString DeleteBookmark = "Delete Bookmark...";
     const QString DisableActivityLogger = "Disable Activity Logger";
-    const QString DisableCrashLogger = "Disable Crash Reporter";
+    const QString EnableCrashReporting = "Enable Crash Reporting";
     const QString DisableEyelidAdjustment = "Disable Eyelid Adjustment";
     const QString DisableLightEntities = "Disable Light Entities";
     const QString DisplayCrashOptions = "Display Crash Options";
@@ -145,7 +145,7 @@ namespace MenuOption {
     const QString OctreeStats = "Entity Statistics";
     const QString OnePointCalibration = "1 Point Calibration";
     const QString OnlyDisplayTopTen = "Only Display Top Ten";
-    const QString OpenVrThreadedSubmit = "OpenVR Threaded Submit"; 
+    const QString OpenVrThreadedSubmit = "OpenVR Threaded Submit";
     const QString OutputMenu = "Display";
     const QString Overlays = "Show Overlays";
     const QString PackageModel = "Package Avatar as .fst...";

interface/src/main.cpp

@@ -421,11 +421,17 @@ int main(int argc, const char* argv[]) {
     }
     qDebug() << "UserActivityLogger is enabled:" << ual.isEnabled();
 
-    bool isCrashHandlerEnabled = ual.isCrashMonitorEnabled() || parser.isSet(forceCrashReportingOption);
-    qDebug() << "Crash handler logger is enabled:" << isCrashHandlerEnabled;
-    if (isCrashHandlerEnabled) {
-        auto crashHandlerStarted = startCrashHandler(argv[0]);
+    if (parser.isSet(forceCrashReportingOption)) {
+        qInfo() << "Crash reporting enabled on the command-line";
+        ual.setCrashReportingEnabled(true);
+    }
+
+    auto crashHandlerStarted = startCrashHandler(argv[0]);
+    if (crashHandlerStarted) {
+        ual.setCrashMonitorStarted(true);
         qDebug() << "Crash handler started:" << crashHandlerStarted;
+    } else {
+        qWarning() << "Crash handler failed to start";
     }
 
     const QString& applicationName = getInterfaceSharedMemoryName();

interface/src/ui/PreferencesDialog.cpp

@@ -238,9 +238,9 @@ void setupPreferences() {
         auto preference = new BrowsePreference(SNAPSHOTS, "Put my snapshots here", getter, setter);
         preferences->addPreference(preference);
     }
-    
+
     {
-        auto getter = []()->bool { 
+        auto getter = []()->bool {
             if (!DependencyManager::get<Snapshot>()->_snapshotNotifications.isSet()){
                 DependencyManager::get<Snapshot>()->_snapshotNotifications.set(true);
             }
@@ -278,7 +278,7 @@ void setupPreferences() {
         preference->setItems(items);
         preferences->addPreference(preference);
     }
-    
+
     {
         auto getter = []()->float { return SnapshotAnimated::snapshotAnimatedDuration.get(); };
         auto setter = [](float value) { SnapshotAnimated::snapshotAnimatedDuration.set(value); };
@@ -299,8 +299,8 @@ void setupPreferences() {
     }
 
     {
-        auto getter = []()->bool { return !Menu::getInstance()->isOptionChecked(MenuOption::DisableCrashLogger); };
-        auto setter = [](bool value) { Menu::getInstance()->setIsOptionChecked(MenuOption::DisableCrashLogger, !value); };
+        auto getter = []()->bool { return Menu::getInstance()->isOptionChecked(MenuOption::EnableCrashReporting); };
+        auto setter = [](bool value) { Menu::getInstance()->setIsOptionChecked(MenuOption::EnableCrashReporting, value); };
         preferences->addPreference(new CheckPreference("Privacy", "Send crashes - Overte uses information provided by your "
                                 "client to improve the product through crash reports. By allowing Overte to collect "
                                 "this information you are helping to improve the product. ", getter, setter));

libraries/networking/src/UserActivityLogger.cpp

@@ -34,10 +34,6 @@ void UserActivityLogger::disable(bool disable) {
     _disabled.set(disable);
 }
 
-void UserActivityLogger::crashMonitorDisable(bool disable) {
-    _crashMonitorDisabled.set(disable);
-}
-
 void UserActivityLogger::logAction(QString action, QJsonObject details, JSONCallbackParameters params) {
 //  qCDebug(networking).nospace() << ">>> UserActivityLogger::logAction(" << action << "," << QJsonDocument(details).toJson();
 // This logs what the UserActivityLogger would normally send to centralized servers.
@@ -86,6 +82,15 @@ void UserActivityLogger::requestError(QNetworkReply* errorReply) {
     qCDebug(networking) << errorReply->error() << "-" << errorReply->errorString();
 }
 
+void UserActivityLogger::setCrashReportingEnabled(bool enabled) {
+    bool old = _crashReportingEnabled.get();
+    _crashReportingEnabled.set(enabled);
+
+    if (old != enabled) {
+        emit crashReportingEnabledChanged();
+    }
+ }
+
 void UserActivityLogger::launch(QString applicationVersion, bool previousSessionCrashed, int previousSessionRuntime) {
     const QString ACTION_NAME = "launch";
     QJsonObject actionDetails;

libraries/networking/src/UserActivityLogger.h

@@ -27,40 +27,93 @@ const QString USER_ACTIVITY_URL = "/api/v1/user_activities";
 
 class UserActivityLogger : public QObject {
     Q_OBJECT
-    
+
 public:
     static UserActivityLogger& getInstance();
-    
+
 public slots:
     bool isEnabled() { return !_disabled.get(); }
     bool isDisabledSettingSet() const { return _disabled.isSet(); }
 
-    bool isCrashMonitorEnabled() { return !_crashMonitorDisabled.get(); }
-    bool isCrashMonitorDisabledSettingSet() const { return _crashMonitorDisabled.isSet(); }
+
+    /**
+     * @brief Whether the crash monitor has been successfully started
+     *
+     * Reasons for it failing to start include:
+     *
+     *  * Not having a crash reporter for the platform
+     *  * Crash reporter not being configured with reporting URLs (CMAKE_BACKTRACE_TOKEN and CMAKE_BACKTRACE_URL)
+     *  * Crash reporter is present and configured, but failed to initialize for some reason
+     *
+     * @return true Crash reporter is present, configured and working.
+     * @return false Crash reporter has not been started for one of the above reasons.
+     */
+    bool isCrashMonitorStarted() const { return _crashMonitorStarted; }
+
+
+    /**
+     * @brief Whether the crash monitor will report crashes if they occur
+     *
+     * This setting is independent of isCrashMonitorStarted() -- crash reporting may be enabled but fail to work
+     * due to the crash reporting component being missing or failing to initialize.
+     *
+     * @return true Crashes will be reported to CMAKE_BACKTRACE_URL
+     * @return false Crashes will not be reported
+     */
+    bool isCrashReportingEnabled() { return _crashReportingEnabled.get(); }
+
+    /**
+     * @brief Marks the crash monitor as started
+     *
+     * @warning Only to be used as part of the startup process
+     *
+     * @param started
+     */
+    void setCrashMonitorStarted(bool started) { _crashMonitorStarted = started; }
+
+    /**
+     * @brief Set whether we want to submit crash reports to the report server
+     *
+     * The report server is configured with CMAKE_BACKTRACE_URL.
+     * Emits crashReportingEnabledChanged signal.
+     *
+     * @param enabled Whether it's enabled.
+     */
+    void setCrashReportingEnabled(bool enabled);
 
     void disable(bool disable);
-    void crashMonitorDisable(bool disable);
     void logAction(QString action, QJsonObject details = QJsonObject(), JSONCallbackParameters params = JSONCallbackParameters());
-    
+
     void launch(QString applicationVersion, bool previousSessionCrashed, int previousSessionRuntime);
 
     void insufficientGLVersion(const QJsonObject& glData);
-    
+
     void changedDisplayName(QString displayName);
     void changedModel(QString typeOfModel, QString modelURL);
     void changedDomain(QString domainURL);
     void connectedDevice(QString typeOfDevice, QString deviceName);
     void loadedScript(QString scriptName);
     void wentTo(AddressManager::LookupTrigger trigger, QString destinationType, QString destinationName);
-    
+
+signals:
+
+    /**
+     * @brief The crash reporting setting has been changed.
+     *
+     * This signal is used by the crash reporter to enable/disable itself.
+     *
+     */
+    void crashReportingEnabledChanged();
+
 private slots:
     void requestError(QNetworkReply* errorReply);
-    
+
 private:
     UserActivityLogger();
     Setting::Handle<bool> _disabled { "UserActivityLoggerDisabled", true };
-    Setting::Handle<bool> _crashMonitorDisabled { "CrashMonitorDisabled2", true };
+    Setting::Handle<bool> _crashReportingEnabled { "CrashReportingEnabled", false };
 
+    bool _crashMonitorStarted {false};
     QElapsedTimer _timer;
 };