Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

- Update Xbox toolchain to include `UseDebugLibraries` fix for Debug builds. ([#1302](https://github.com/getsentry/sentry-native/pull/1302))
- Fix GDK version selection for Xbox by propagating `XdkEditionTarget` to MSBuild. ([#1312](https://github.com/getsentry/sentry-native/pull/1312))
- Fix WinHTTP initialization timing to prevent error 12007 on Xbox platform. ([#1321](https://github.com/getsentry/sentry-native/pull/1321))

**Meta**:

Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ elseif(LINUX)
set(_SENTRY_PLATFORM_LIBS "dl" "rt")
elseif(WIN32)
if (XBOX)
set(_SENTRY_PLATFORM_LIBS "version")
set(_SENTRY_PLATFORM_LIBS "version" "xgameruntime")
else()
set(_SENTRY_PLATFORM_LIBS "dbghelp" "shlwapi" "version")
endif()
Expand Down
5 changes: 5 additions & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@ elseif(SENTRY_TRANSPORT_WINHTTP)
sentry_target_sources_cwd(sentry
transports/sentry_transport_winhttp.c
)
if (XBOX)
sentry_target_sources_cwd(sentry
transports/sentry_transport_xnetworking.cpp
)
endif()
elseif(SENTRY_TRANSPORT_NONE)
sentry_target_sources_cwd(sentry
transports/sentry_transport_none.c
Expand Down
65 changes: 47 additions & 18 deletions src/transports/sentry_transport_winhttp.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
#include <string.h>
#include <winhttp.h>

#ifdef SENTRY_PLATFORM_XBOX
HRESULT sentry__transport_ensure_network_initialized();
#endif // SENTRY_PLATFORM_XBOX

typedef struct {
sentry_dsn_t *dsn;
wchar_t *user_agent;
Expand Down Expand Up @@ -40,6 +44,31 @@ sentry__winhttp_bgworker_state_new(void)
return state;
}

static void
sentry__winhttp_session_start(void *_state)
{
winhttp_bgworker_state_t *state = _state;
if (state->proxy) {
state->session
= WinHttpOpen(state->user_agent, WINHTTP_ACCESS_TYPE_NAMED_PROXY,
state->proxy, WINHTTP_NO_PROXY_BYPASS, 0);
} else {
#if _WIN32_WINNT >= 0x0603
state->session = WinHttpOpen(state->user_agent,
WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY, WINHTTP_NO_PROXY_NAME,
WINHTTP_NO_PROXY_BYPASS, 0);
#endif
// On windows 8.0 or lower, WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY does
// not work on error we fallback to
// WINHTTP_ACCESS_TYPE_DEFAULT_PROXY
if (!state->session) {
state->session = WinHttpOpen(state->user_agent,
WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME,
WINHTTP_NO_PROXY_BYPASS, 0);
}
}
}

static void
sentry__winhttp_bgworker_state_free(void *_state)
{
Expand Down Expand Up @@ -118,28 +147,14 @@ sentry__winhttp_transport_start(
}
}

if (state->proxy) {
state->session
= WinHttpOpen(state->user_agent, WINHTTP_ACCESS_TYPE_NAMED_PROXY,
state->proxy, WINHTTP_NO_PROXY_BYPASS, 0);
} else {
#if _WIN32_WINNT >= 0x0603
state->session = WinHttpOpen(state->user_agent,
WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY, WINHTTP_NO_PROXY_NAME,
WINHTTP_NO_PROXY_BYPASS, 0);
#endif
// On windows 8.0 or lower, WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY does
// not work on error we fallback to WINHTTP_ACCESS_TYPE_DEFAULT_PROXY
if (!state->session) {
state->session = WinHttpOpen(state->user_agent,
WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME,
WINHTTP_NO_PROXY_BYPASS, 0);
}
}
#ifndef SENTRY_PLATFORM_XBOX
sentry__winhttp_session_start(state);

if (!state->session) {
SENTRY_WARN("`WinHttpOpen` failed");
return 1;
}
#endif // !SENTRY_PLATFORM_XBOX

return sentry__bgworker_start(bgworker);
}
Expand Down Expand Up @@ -187,6 +202,20 @@ sentry__winhttp_send_task(void *_envelope, void *_state)
sentry_envelope_t *envelope = (sentry_envelope_t *)_envelope;
winhttp_bgworker_state_t *state = (winhttp_bgworker_state_t *)_state;

#ifdef SENTRY_PLATFORM_XBOX
if (!state->session) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're only checking session once, however, the suspend/resume cycle would influence the connectivity. Do we need to ensure connection every time we make a request instead?

SENTRY_DEBUG(
"ensuring xbox network is initialized for WinHttp transport");
sentry__transport_ensure_network_initialized();
sentry__winhttp_session_start(state);
Comment on lines +209 to +210
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure - are both of these safe to call from a background thread?


if (!state->session) {
SENTRY_WARN("`WinHttpOpen` failed");
return;
}
}
#endif // SENTRY_PLATFORM_XBOX

uint64_t started = sentry__monotonic_time();

char *user_agent = sentry__string_from_wstr(state->user_agent);
Expand Down
64 changes: 64 additions & 0 deletions src/transports/sentry_transport_xnetworking.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#include "sentry_boot.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep the contents here in the *winhttp.c file instead of creating a new one. So far, the mapping is file=transport. This would break that by only extending what's needed for winhttp transport.

Copy link
Author

Choose a reason for hiding this comment

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

I've created dedicated file because xnetworking requires c++ compiler. Having that in mind, do you still think that it should be merged to winhttp.c?

Copy link
Author

Choose a reason for hiding this comment

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

@vaind ping

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it's probably good as is, I'll leave this up to the maintainers

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it may be worthwile to just move this PR to sentry-xbox repository. Then this being a cpp file would be a non-issue


#include <XNetworking.h>

static void
sentry__transport_network_connectivity_hint_changed_callback(_In_ void *context,
_In_ const XNetworkingConnectivityHint *connectivity_hint)
{
HANDLE network_initialized_event = static_cast<HANDLE>(context);
if (connectivity_hint->networkInitialized) {
(void)SetEvent(network_initialized_event);
}
}

extern "C" {

HRESULT
sentry__transport_ensure_network_initialized()
{
HRESULT hr = S_OK;
XNetworkingConnectivityHint connectivity_hint;
XTaskQueueHandle queue;

hr = XTaskQueueCreate(XTaskQueueDispatchMode::Immediate,
XTaskQueueDispatchMode::Immediate, &queue);
if (SUCCEEDED(hr)) {
// Use the new XNetworking APIs to check if the network is initialized.
hr = XNetworkingGetConnectivityHint(&connectivity_hint);
if (SUCCEEDED(hr)) {
if (!connectivity_hint.networkInitialized) {
// The network isn't initialized. Wait until the network becomes
// initialized.
HANDLE network_initialized_event
= CreateEvent(nullptr, TRUE, FALSE, nullptr);
if (network_initialized_event != nullptr) {
XTaskQueueRegistrationToken token;
hr = XNetworkingRegisterConnectivityHintChanged(queue,
network_initialized_event,
sentry__transport_network_connectivity_hint_changed_callback,
&token);
if (SUCCEEDED(hr)) {
DWORD result = WaitForSingleObjectEx(
network_initialized_event, INFINITE, FALSE);
Comment on lines +42 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

This may block indefinitely. Is that the right thing to do? I guess it's alright if we're on the background thread, but might be something to keep in mind in relation to #1316 - at the very least, add a TODO mentioning that issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to what we do for Switch in

#ifdef SENTRY_PLATFORM_NX
if (!sentry_nx_curl_connect(state->nx_state)) {
return; // TODO should we dump the envelope to disk?
}
#endif

if (result != WAIT_OBJECT_0) {
hr = HRESULT_FROM_WIN32(GetLastError());
}

XNetworkingUnregisterConnectivityHintChanged(
token, true);
}

CloseHandle(network_initialized_event);
} else {
hr = HRESULT_FROM_WIN32(GetLastError());
}
}
}

XTaskQueueCloseHandle(queue);
}

return hr;
}
}
Loading