-
Notifications
You must be signed in to change notification settings - Fork 119
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
[XB1] Release DX extended resources before Cobalt exit #1524
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1524 +/- ##
==========================================
- Coverage 57.77% 57.76% -0.02%
==========================================
Files 1915 1915
Lines 95127 95129 +2
==========================================
- Hits 54960 54950 -10
- Misses 40167 40179 +12
|
f90f5a0
to
71e71be
Compare
ec296e6
to
c922d06
Compare
@@ -410,6 +410,10 @@ bool Application::DispatchAndDelete(Application::Event* event) { | |||
} | |||
|
|||
bool Application::HandleEventAndUpdateState(Application::Event* event) { | |||
// Don't accept any events in kStateStopped state | |||
if (state_ == kStateStopped) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaidokert is this ok to reject all events if the state is stopped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaidokert is this ok to reject all events if the state is stopped?
When application goes to state_ == kStateStopped it means that the button "Close YT" was pressed. It is the last event. Almost last, because the next, really last event is "Button Up" event. It does nothing, because system reacts on "Button Down" event. But application can be already destroyed when last "Button Up" goes. It is the reason I ignore all events in kStateStopped state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to confirm with @kaidokert since this will affect all platforms. Is there any platform (or even 3rd party) that might expect events even in the stopped state?
I'm very hesitant to change things at the starboard/shared level since it could affect all other platforms, especially 3rd party platforms that we don't have direct visibility into.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologize for late review. @jellefoks is the best expert in the lifecycle changes actually, i'd leave it for his throughts. According to the lifecycle diagram we never leave the STOPPED state so it doesn't look too risky to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that you have seen happen, and what goes wrong when the event is not dropped here? I'm wondering, if this is an actual bug, if this is the best place to fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we have never seen this happen. We had an increased crash rate on the Xbox One platform that we were unable to reproduce. That’s why we reviewed all the code for the potential root of that problem. In this case we suspected that in some cases during the app exit a crash report may be generated. As @victorpasoshnikov wrote, now we know that this never happens, so this fix is not required.
When I run this locally with a debugger attached, I see that on application exit SbSystemBreakIntoDebugger gets hit, looking like a crash. To users that shouldn't matter since they're quitting the app anyway, but I'll need to test this against Microsoft's crash reporting to make sure we aren't going to have a crash reported for every app launch. |
I investigated these SbSystemBreakIntoDebugger calls. |
b/299726093 Change-Id: Ic47bbf2f454c98a79a9203d0d040ab7a6c2dbdc2
b/299726093 Change-Id: I632e0d46496e402678a02d4e9d5139d194d45758
5f2b258
to
e8c8607
Compare
@@ -410,6 +410,10 @@ bool Application::DispatchAndDelete(Application::Event* event) { | |||
} | |||
|
|||
bool Application::HandleEventAndUpdateState(Application::Event* event) { | |||
// Don't accept any events in kStateStopped state | |||
if (state_ == kStateStopped) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to confirm with @kaidokert since this will affect all platforms. Is there any platform (or even 3rd party) that might expect events even in the stopped state?
I'm very hesitant to change things at the starboard/shared level since it could affect all other platforms, especially 3rd party platforms that we don't have direct visibility into.
// But CoreApplication::Run never reterns in UWP apps. | ||
// To avoid hang in starboard::Thread::Join() and SbSystemBreakIntoDebugger() | ||
// in starboard::Thread::~Thread() override Join() in CoreApplicationThread | ||
// It does nothing but prevents SbSystemBreakIntoDebugger() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to find a solution that doesn't deviate from common::Thread if possible.
I'm also seeing it hang when we call thread.Join(), but I wasn't able to tell why exactly. I saw it got to starboard\shared\win32\condition_variable_wait.cc (28)
before hanging while other threads continued to run. For instance, analog_thumbstick_thread_ was still taking input in this state, so the ApplicationUwp singleton wasn't destroyed yet. Should that happen prior to the thread join? I'll try to look into this more as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the fact that extended resources are not released in exit doesn't increase crash statistics. I looked on my solution with more attention and found that in time of CoreApplication::Exit() call there are dozens alive threads in Cobalt. That is we aren't able to resolve this issue in classic Windows desktop mode without a huge changing in code. I think CoreApplication::Exit() just kills the Cobalt process. So in my opinion we can ignore unreleased extended resources and leave things as is.
@amurovanyi closed the 299726093 issue as obsolete. So I close the PR too |
b/299726093
Change-Id: Ic47bbf2f454c98a79a9203d0d040ab7a6c2dbdc2