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

[Peripherals] Fix segfault by adding some synchronization to peripheral add-ons #11824

Merged
merged 6 commits into from
Mar 24, 2017

Conversation

garbear
Copy link
Member

@garbear garbear commented Mar 7, 2017

This fixes a segfault at exit when peripheral add-ons misbehave. In particular, if the add-on hangs in another thread, then it can be destroyed while still in use. This was causing a segfault on exit.

Another segfault (or possibly the same one) has been fixed by moving g_peripherals to ServiceManager. Before, it tried to release resources when the static object was deconstructed after the app shut down.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@garbear garbear closed this Mar 7, 2017
@garbear garbear deleted the input-fixes branch March 7, 2017 18:28
@garbear garbear restored the input-fixes branch March 8, 2017 16:13
@garbear garbear deleted the input-fixes branch March 8, 2017 16:13
@garbear garbear restored the input-fixes branch March 8, 2017 16:13
@garbear garbear reopened this Mar 8, 2017
The command to power off controllers should be called from inside an exit
callback. This callback is issued before shutdown begins, so power-off no
longer depends on systems that have already been deinitialized.
@garbear garbear changed the title [Peripherals] Fix segfaults during exit [Peripherals] Fix segfault by adding some synchronization to peripheral add-ons Mar 8, 2017
@garbear
Copy link
Member Author

garbear commented Mar 8, 2017

I've pushed a new commit and updated the PR description. It turns out, there were only 2 segfaults at work here, one from misbehaving peripheral add-ons, and one due to g_peripherals being a static variable and trying to release resources that had already disappeared.

@MartijnKaijser
Copy link
Member

Does this also affect v17?
Jenkins build this please

@garbear
Copy link
Member Author

garbear commented Mar 9, 2017

Yes, Kodi is still crashing on the Steam Link, so without this Kodi crashes on exit 100% of the time.

@garbear
Copy link
Member Author

garbear commented Mar 9, 2017

This almost concludes my Krypton backports. The only other bugs I care to fix are the iMON incompatibility and better Shield controller support. If we're gearing up for 17.1 release I'm fine waiting for 17.2 or 18.0.

@garbear
Copy link
Member Author

garbear commented Mar 10, 2017

Hold off on merging. The shutdown stuff still isn't happening in the right order.

@garbear garbear force-pushed the input-fixes branch 2 times, most recently from 825607b to c4a8867 Compare March 10, 2017 17:55
@garbear
Copy link
Member Author

garbear commented Mar 10, 2017

lol, peripherals shutdown was 1 line off. Also pushed a commit for a third segfault that arose from starting/stopping things in the proper order.

jenkins build this please

@garbear
Copy link
Member Author

garbear commented Mar 10, 2017

jenkins build this please

{
assert(m_peripheralManager != nullptr);

This comment was marked as spam.

This comment was marked as spam.

@garbear
Copy link
Member Author

garbear commented Mar 12, 2017

updated with @thebrid's comment. jenkins build this please

@garbear
Copy link
Member Author

garbear commented Mar 12, 2017

Build errors look unrelated, but both OSX and linux64 failed, so again we try. Jenkins build this please.

@garbear
Copy link
Member Author

garbear commented Mar 13, 2017

let's try again. jenkins build and merge

@wsnipex
Copy link
Member

wsnipex commented Mar 22, 2017

jenkins build this please

@garbear
Copy link
Member Author

garbear commented Mar 22, 2017

There's an error with the test cases. Moving around the order of services on startup typically breaks things. I'll address this when I get a chance. Until then, this problem only manifests with broken peripheral add-ons, which is only peripheral.joystick on the Steam Link (and I work around it). So not critical.

@garbear
Copy link
Member Author

garbear commented Mar 24, 2017

test cases fixed! jenkins build this please

@garbear garbear merged commit cb344f0 into xbmc:master Mar 24, 2017
@garbear garbear deleted the input-fixes branch March 24, 2017 04:01
@Rechi Rechi added the v18 Leia label Mar 24, 2017
@Rechi Rechi added this to the L 18.0-alpha1 milestone Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Input Type: Fix non-breaking change which fixes an issue v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants