Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cross-Plattform Crash Reporting #4455

Merged
merged 28 commits into from
Nov 23, 2022
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ vscode-cpptools/*
*.xcodeproj/
MozillaVPN.vcxproj*
*.autosave
# Visual Studio stuff.
CMakeSettings.json

# Qt Creator
CMakeLists.txt.user
Expand Down
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,6 @@
path = 3rdparty/glean
url = https://github.com/mozilla/glean.git
branch = main
[submodule "3rdparty/sentry"]
path = 3rdparty/sentry
url = https://github.com/getsentry/sentry-native/
1 change: 1 addition & 0 deletions 3rdparty/sentry
Submodule sentry added at 87e67a
30 changes: 30 additions & 0 deletions LICENSE.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ the terms of the MIT Public License (*MIT*), a copy of which is available
[available here](https://opensource.org/licenses/MIT) and a copy is provided
below.

strseb marked this conversation as resolved.
Show resolved Hide resolved
The project relies on [sentry-native](https://github.com/getsentry/sentry-native), under
the terms of the MIT Public License (*MIT*), a copy of which is available
[available here](https://opensource.org/licenses/MIT) and a copy is provided
below.

Finally, this project uses EC25519 and CHACHA-POLY implementations from
[HACL\*](https://hacl-star.github.io/), available under the terms of the MIT
Public License (*MIT*) and copyright (c) 2016-2020 INRIA, CMU, and Microsoft
Expand Down Expand Up @@ -1437,3 +1442,28 @@ distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.


The MIT License (MIT) - sentry-native
=================================

Copyright (c) 2019 Sentry (https://sentry.io) and individual contributors.
All rights reserved.

Permission is hereby granted, free of charge, to any person obtaining a copy of
this software and associated documentation files (the "Software"), to deal in
the Software without restriction, including without limitation the rights to
use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
of the Software, and to permit persons to whom the Software is furnished to do
so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
4 changes: 3 additions & 1 deletion src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ target_link_libraries(mozillavpn PUBLIC ${CMAKE_DL_LIBS})
target_link_libraries(mozillavpn PRIVATE glean lottie nebula translations)

include(cmake/sources.cmake)
include(cmake/sentry.cmake)

if(${BUILD_DUMMY})
set(MVPN_PLATFORM_NAME "dummy")
Expand All @@ -58,7 +59,8 @@ if(NOT ${CMAKE_SYSTEM_NAME} STREQUAL "Emscripten")
target_link_libraries(mozillavpn PRIVATE crashreporter vpnglean -ldl)
if (WIN32)
target_link_libraries(mozillavpn PRIVATE ntdll)
set_target_properties(mozillavpn PROPERTIES LINK_FLAGS "/SUBSYSTEM:CONSOLE")
endif()
endif()

qt_finalize_target(mozillavpn)
qt_finalize_target(mozillavpn)
90 changes: 90 additions & 0 deletions src/cmake/sentry.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.


# This CMAKE File will Integrate Sentry into MVPN


# Defines which OS builds can include sentry. Check src/cmake Lists for all values of MVPN_PLATFORM_NAME
set(SENTRY_SUPPORTED_OS "Windows" "Darwin" "Android")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no iOS and Linux?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not tested so far, as i don't have a build machine setuped yet 😅.
For iOS I want to wait for it to be moved to cmake before i start that.

Copy link
Contributor

@brizental brizental Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, totally get that. Let's file tickets for the iOS and Linux work though, so we can keep track of it.

set(EXTERNAL_INSTALL_LOCATION ${CMAKE_BINARY_DIR}/external)
include(ExternalProject)



LIST(FIND SENTRY_SUPPORTED_OS ${CMAKE_SYSTEM_NAME} _SUPPORTED)

if( ${_SUPPORTED} GREATER -1 )
message("Building sentry for ${CMAKE_SYSTEM_NAME}")
target_compile_definitions(mozillavpn PRIVATE SENTRY_ENABLED)
# Sentry support is given
target_sources(mozillavpn PRIVATE
sentry/sentryadapter.cpp
sentry/sentryadapter.h
)

# Configure Linking and Compile
if(APPLE)
include(cmake/osxtools.cmake)
# Let sentry.h know we are using a static build
target_compile_definitions(mozillavpn PRIVATE SENTRY_BUILD_STATIC)
# Let mozilla-vpn know we need to provide the upload client
target_compile_definitions(mozillavpn PRIVATE SENTRY_NONE_TRANSPORT)
# Compile Static for apple and link to libsentry.a
target_link_libraries(mozillavpn PUBLIC libsentry.a)
target_link_libraries(mozillavpn PUBLIC breakpad_client.a)
# We are using breakpad as a backend - in process stackwalking is never the best option ... however!
# this is super easy to link against and we do not need another binary shipped with the client.
SET(SENTRY_ARGS -DSENTRY_BACKEND=breakpad -DSENTRY_BUILD_SHARED_LIBS=false -DSENTRY_TRANSPORT=none)
endif()
if(WIN32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we use an elseif here? Also for the Android conditional.

# Let sentry.h know we are using a static build
target_compile_definitions(mozillavpn PRIVATE SENTRY_BUILD_STATIC)
# Link against static sentry + breakpad + the stack unwind utils
target_link_libraries(mozillavpn PUBLIC sentry.lib)
target_link_libraries(mozillavpn PUBLIC breakpad_client.lib)
target_link_libraries(mozillavpn PUBLIC dbghelp.lib)
# Windows will use the winhttp transport btw
SET(SENTRY_ARGS -DSENTRY_BUILD_SHARED_LIBS=false -DSENTRY_BACKEND=breakpad -DCMAKE_BUILD_TYPE=Release)
endif()

if(ANDROID)
# Let mozilla-vpn know we need to provide the upload client
target_compile_definitions(mozillavpn PRIVATE SENTRY_NONE_TRANSPORT)

target_link_libraries(mozillavpn PUBLIC libsentry.a)
target_link_libraries(mozillavpn PUBLIC libunwindstack.a)
# We can only use inproc as crash backend.
SET(SENTRY_ARGS -DSENTRY_BUILD_SHARED_LIBS=false
-DANDROID_PLATFORM=21
-DCMAKE_SYSTEM_NAME=Android
-DANDROID_ABI=${ANDROID_ABI}
-DCMAKE_ANDROID_NDK=${ANDROID_NDK_ROOT}
-DCMAKE_TOOLCHAIN_FILE=${ANDROID_NDK_ROOT}/build/cmake/android.toolchain.cmake
-DSENTRY_BACKEND=inproc
)
endif()

include(ExternalProject)
ExternalProject_Add(sentry
SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}../../3rdparty/sentry
GIT_REPOSITORY https://github.com/getsentry/sentry-native/
GIT_TAG 0.5.0
CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${EXTERNAL_INSTALL_LOCATION} ${SENTRY_ARGS}
)

target_include_directories(mozillavpn PUBLIC ${EXTERNAL_INSTALL_LOCATION}/include)
target_link_directories( mozillavpn PUBLIC ${EXTERNAL_INSTALL_LOCATION}/lib)
add_dependencies(mozillavpn sentry)
else()
# Sentry is not supported on this Plattform.
message("Sentry supported OS -> ${SENTRY_SUPPORTED_OS}")
message("Cannot build sentry for ${CMAKE_SYSTEM_NAME}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd remove this message. It's redudant with the previous one and it looks like an error.

endif()

# Add Sources that will be required anyway
target_sources(mozillavpn PRIVATE
tasks/sentry/tasksentry.cpp
tasks/sentry/tasksentry.h
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: newline.

4 changes: 0 additions & 4 deletions src/commands/commandui.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
#endif

#ifdef MVPN_WINDOWS
# include "crashreporter/crashclient.h"
# include "eventlistener.h"
# include "platforms/windows/windowsstartatbootwatcher.h"
# include "platforms/windows/windowsappimageprovider.h"
Expand Down Expand Up @@ -179,9 +178,6 @@ int CommandUI::run(QStringList& tokens) {
std::cerr.clear();
}
# endif

CrashClient::instance().start(CommandLineParser::argc(),
CommandLineParser::argv());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: What are we removing here? What is the crash client? How is this related to the Sentry changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crash client is a mini qt application we're shipping in the client. It shows the "ohno it's crashed, wanna upload?" prompt. It's currently only running under windows.
This line will make the crash client register itself for "structured exception handling", - which we don't want as sentry will do that.
On that note, I currently don't want to rip that whole thing out.... yet. We might still want to create the crash-ui process, in the sentry onCrash handler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, yeah my question was a precursor to me asking if you wanted to also just remove the whole thing. Thanks for clarifying!

#endif

#ifdef MVPN_DEBUG
Expand Down
7 changes: 7 additions & 0 deletions src/constants.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,13 @@ CONSTEXPR(uint32_t, controllerPeriodicStateRecorderMsec, 10800000, 60000, 0)
#define PRODBETAEXPR(type, functionName, prod, beta) \
inline type functionName() { return inProduction() ? prod : beta; }

// TODO: Get a mozilla one - this is bastis project :)
constexpr const char* SENTRY_DER =
"https://[email protected]/"
"6719480";
constexpr const char* SENTRY_ENVELOPE_INGESTION =
"https://o1396220.ingest.sentry.io/api/6719480/envelope/";

constexpr const char* API_PRODUCTION_URL = "https://vpn.mozilla.org";
constexpr const char* API_STAGING_URL =
"https://stage-vpn.guardian.nonprod.cloudops.mozgcp.net";
Expand Down
8 changes: 8 additions & 0 deletions src/featureslist.h
Original file line number Diff line number Diff line change
Expand Up @@ -240,3 +240,11 @@ FEATURE_SIMPLE(gleanRust, // Feature ID
FeatureCallback_true, // Can be flipped off
QStringList(), // feature dependencies
FeatureCallback_false)

FEATURE_SIMPLE(sentry, // Feature ID
"Sentry Crash Report SDK", // Feature name
"2.13.0", // released
FeatureCallback_false, // Can be flipped on
FeatureCallback_false, // Can be flipped off
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not allow flipping it on and off?

QStringList(), // feature dependencies
FeatureCallback_inStaging)
brizental marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 14 additions & 0 deletions src/mozillavpn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@
#include "urlopener.h"
#include "websocket/websockethandler.h"

#ifdef SENTRY_ENABLED
#include "sentry/sentryadapter.h"
#endif

#ifdef MVPN_IOS
# include "platforms/ios/iosdatamigration.h"
# include "platforms/ios/iosutils.h"
Expand Down Expand Up @@ -994,6 +998,9 @@ void MozillaVPN::mainWindowLoaded() {
m_gleanTimer.start(Constants::gleanTimeoutMsec());
m_gleanTimer.setSingleShot(false);
#endif
#ifdef SENTRY_ENABLED
SentryAdapter::instance()->init();
#endif
}

void MozillaVPN::telemetryPolicyCompleted() {
Expand Down Expand Up @@ -1603,6 +1610,13 @@ void MozillaVPN::exitForUnrecoverableError(const QString& reason) {

void MozillaVPN::crashTest() {
logger.debug() << "Crashing Application";

unsigned char* test = NULL;
test[1000] = 'a'; //<< here it should crash

// Interestingly this does not cause a "Signal" but a VC runtime exception
// and more interestingly, neither breakpad nor crashpad are catchting this on
// windows...
char* text = new char[100];
delete[] text;
delete[] text;
Expand Down
13 changes: 13 additions & 0 deletions src/networkrequest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,19 @@ NetworkRequest* NetworkRequest::createForFxaSessionDestroy(
return r;
}

// static
NetworkRequest* NetworkRequest::createForSentry(Task* parent,
const QByteArray& envelope) {
NetworkRequest* r = new NetworkRequest(parent, 200, false);
QUrl url(Constants::SENTRY_ENVELOPE_INGESTION);
r->m_request.setUrl(url);
r->m_request.setHeader(QNetworkRequest::ContentTypeHeader,
"application/x-sentry-envelope");
r->m_request.setRawHeader("dsn", Constants::SENTRY_DER);
r->postRequest(envelope);
return r;
}

NetworkRequest* NetworkRequest::createForProducts(Task* parent) {
Q_ASSERT(parent);

Expand Down
3 changes: 3 additions & 0 deletions src/networkrequest.h
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ class NetworkRequest final : public QObject {

static NetworkRequest* createForFxaSessionDestroy(
Task* parent, const QByteArray& sessionToken);

static NetworkRequest* createForSentry(Task* parent,
const QByteArray& envelope);

static NetworkRequest* createForProducts(Task* parent);

Expand Down
Loading