Page MenuHomeWildfire Games

Alt + Tab during loading in fullscreen mode crash
ClosedPublic

Authored by Silier on May 16 2018, 10:26 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22314: Fix a crash on some system when Alt-tabbing during game setup.
Summary

This bug is related to D1212. There is special case that was not covered and it is kind of rare but not impossible situation.

  1. start a map
  2. during loading do alt + tab and stay out of window
  3. after game is loaded and simulation tries to start it crashes
Test Plan

If you could not reproduce D1212 you probably can not this one.

0. patch

  1. start a map
  2. during loading do alt + tab
  3. game is started

Should not crash due to alt + tab

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6984
Build 11425: Vulcan BuildJenkins
Build 11424: arc lint + arc unit

Event Timeline

Silier created this revision.May 16 2018, 10:26 PM
Stan added reviewers: Restricted Owners Package, Restricted Owners Package.May 16 2018, 10:31 PM
Stan added a subscriber: Stan.
Stan added inline comments.
source/ps/Game.cpp
319 ↗(On Diff #6559)

Maybe "some graphic cards" instead of "some graphic card families".
I think the comment could be improved but I'm not sure how.

Vulcan added a subscriber: Vulcan.May 16 2018, 11:19 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/501/display/redirect

wraitii added inline comments.
source/ps/Game.cpp
319 ↗(On Diff #6559)

Agreed, this should instead state something like "Do not render if not in focus, as that triggers a difficult-to-reproduce crash on some graphic cards. See D121 and D1495 for discussion."

I'd also like the comment on https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/main.cpp;21821$411 changed to something similar.

elexis added a subscriber: elexis.May 17 2018, 11:09 AM

So should this not become an early return in Render() rather than having the condition twice (if one needs fixing the other will be forgotton) and possibly missing in a third place?

Silier planned changes to this revision.May 17 2018, 11:38 AM
In D1495#61322, @elexis wrote:

So should this not become an early return in Render() rather than having the condition twice (if one needs fixing the other will be forgotton) and possibly missing in a third place?

sure, it could and will :)

Silier updated this revision to Diff 6569.May 18 2018, 5:33 PM

moving condition into render function and comment change

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/509/display/redirect

vladislavbelov added inline comments.
source/ps/GameSetup/GameSetup.cpp
197

Do we need these links? I have doubts, because these diffs don't describe the problem, only mention, that the problem presents. The comments does the same thing. But if someone want more information, links can be found easily through the blame.

ok, so @wraitii and @vladislavbelov make decision. I do not care what exactly will be in comment.

wraitii added inline comments.May 19 2018, 10:32 PM
source/ps/GameSetup/GameSetup.cpp
197

I suppose that's fair enough. I like having to not go through blame. Don't care that much tbh.

Stan added a comment.Jan 3 2019, 1:03 AM

@wraitii @vladislavbelov Can you accept/request changes on this patch ?

Silier updated this revision to Diff 7559.Mar 17 2019, 12:39 PM

ready to go

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/main.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1101/display/redirect

Stan added a comment.Mar 17 2019, 3:33 PM

Need to bump those copyrights :)

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1107/display/redirect

wraitii accepted this revision.May 28 2019, 2:53 PM

The reason why this happens is that CGame::ReallyStartGame calls Render() to start loading assets.

I think moving the checks inside the Render() function is a good move.

Now what I wondered is why SDL_GL_SwapWindow(g_VideoMode.GetWindow()); is in Main and not in render, and it happens to have been there from rP5 (!!!). At this point it just feels like an oversight, so I suggest moving this inside Render() and we can remove this if in main().

This revision is now accepted and ready to land.May 28 2019, 2:53 PM
This revision was automatically updated to reflect the committed changes.