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

GCAdapter: Defer initialization until MainWindow::InitControllers #10502

Merged
merged 1 commit into from Mar 15, 2022

Conversation

Pokechu22
Copy link
Contributor

If libusb fails to initialize, an assertion fails, but if that happens before the main window is created, then Dolphin just dies. Now, the panic alert is properly shown and the user can ignore it.

Note that the shutdown still happens in UICommon:

void Shutdown()
{
GCAdapter::Shutdown();
WiimoteReal::Shutdown();
Common::Log::LogManager::Shutdown();
Discord::Shutdown();
SConfig::Shutdown();
Config::Shutdown();
}

(Wiimote::Shutdown is called in MainWindow::ShutdownControllers, but WiimoteReal::Shutdown is in UICommon, so I think it's fine to have it organized like this... though I'm not fully sure.)

@leoetlino
Copy link
Member

leoetlino commented Mar 8, 2022

I believe this would break GCAdapter for DolphinNoGUI, since GCAdapter::Init is only called from DolphinQt with this change.

Edit: This is probably why FifoCI fails.

@Pokechu22 Pokechu22 marked this pull request as draft March 9, 2022 22:03
@Pokechu22
Copy link
Contributor Author

Pokechu22 commented Mar 10, 2022

Changed by adding a call in MainNoGUI.cpp. But now I'm not sure if this works on android (though I don't know if it's even possible to use a GC adapter on android, though that's tangential to whether GCAdapter is initialized on it).

Note that this means that dolphin-emu-nogui does still suffer from the same abort() call if libusb fails to initialize. I don't think it ever registers a custom panic handler, though, so deferring initialization wouldn't change this.

@JosJuice
Copy link
Member

GCAdapter is fully supported on Android. Please add a call inside Java_org_dolphinemu_dolphinemu_NativeLibrary_Initialize in MainAndroid.cpp.

If libusb fails to initialize, an assertion fails, but if that happens before the main window is created, then Dolphin just dies.  Now, the panic alert is properly shown and the user can ignore it.
@Pokechu22 Pokechu22 marked this pull request as ready for review March 10, 2022 18:37
@leoetlino leoetlino merged commit e10967e into dolphin-emu:master Mar 15, 2022
10 checks passed
LittleCoaks added a commit to ProjectRio/ProjectRio that referenced this pull request Mar 18, 2022
Undo this PR dolphin-emu#10502

This way causing Rio to try and read from the GC adapter after closing, which caused exceptions to be thrown.
@pizuz
Copy link

pizuz commented Apr 18, 2022

EDIT: Sorry, this post was pretty confusing. I was referring to mainline Dolphin from the beginning.

Hi,
dunno if whatever was fixed in ProjectRio, might be also present in Dolphin mainline. Anyway, I'm getting a freeze on macOS since this PR. To be more specific: As soon as I press the close button, Dolphin freezes. I made a core dump, attached below:

dolphin_crash.txt

Shall I file a separate bug report?

@Pokechu22
Copy link
Contributor Author

If you can reproduce on mainline dolphin, please do file a bug report. (Here's the version with this change, and the version immediately before without this change).

I don't know anything about ProjectRio, but it looks like they attempt to revert this commit: ProjectRio/ProjectRio@ef33152 (and have included that for a month). I'm not sure if their reversion is done correctly. If you can't reproduce it on mainline dolphin, this might be an issue with their reversion (or maybe it's fixed in their latest version with that reversion).

@pizuz
Copy link

pizuz commented Apr 18, 2022

I just saw that GitHub message. I have no idea what Project Rio is. This definitely affects mainline dolphin and I already bisected the issue.

Running and closing mainline Dolphin with the GCAdapter attached, produces the freeze starting this PR. Doing the same without it attached doesn't. Bug report filed here:

https://bugs.dolphin-emu.org/issues/12892

#endif
s_libusb_context.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be moved after the next line? (Reset())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't directly use the libusb context after this point, but I'm not knowledgeable enough about libusb to say whether that would make a difference (and whether s_handle is still valid).

Copy link
Contributor

Choose a reason for hiding this comment

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

The reported "crash" isn't a crash, it's a deadlock:
GCAdapter::Reset() is hung in a join() call for thread 0xe61f56.
thread 0xe61f56 is in poll() (either Read or Write threadfunc). Read/Write wrap libusb_interrupt_transfer.

LibusbUtils::Context::Impl::~Impl will have already killed it's EventThread and called libusb_interrupt_event_handler and libusb_exit. So..i think that's probably the problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pizuz can you also try this change? (See my first comment)

Copy link

Choose a reason for hiding this comment

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

This produces a segfault:

dolphin_segfault.txt

I'll try your new PR next.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that might have been fixed by one of the recent commits to libusb. so if you compile new PR yourself, make sure to git submodule update to include that part of PR

Copy link

Choose a reason for hiding this comment

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

I commented there. I tried the buildbot build of that PR.

@pizuz pizuz mentioned this pull request Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants