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

[droid] fix crashing on addon installation #8278

Merged
merged 2 commits into from Oct 25, 2015

Conversation

Projects
None yet
9 participants
@wsnipex
Copy link
Member

wsnipex commented Oct 22, 2015

Crossguid uses a jni based implementation on droid that is crashing all over and corrupting the stack.
This PR switches droid to use libuuid instead, just like linux.
Since our libuuid in depends doesn't cross compile on droid, we switch to an easier one from e2fsprogs.
Only tested on droid..

@Montellese, @koying ping

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 22, 2015

jenkins build this please

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Oct 22, 2015

Do we have any clue why it's crashing? Is something wrong with the JNI calls that could be patched / sent upstream?

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 22, 2015

no idea, it crashes inside system libs(don't remember the exact file atm) and gdb doesn't catch it either.

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 22, 2015

jni issues will typically show up in the android log file. You have to back track from there, gdb will be useless as android pops the world out from under it.

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Oct 23, 2015

Yup would be nice to get the adb log about the JNI issue. I took a quick look at the implementation and although I haven't done any JNI in a while it looks ok to me. But there are a lot of things that one can mess up with JNI.

@koying

This comment has been minimized.

Copy link
Contributor

koying commented Oct 23, 2015

We got 2 different minidumps (via breakpad) and both showed stack corruption after the "newguid" call.
I don't see anything wrong with the jni either, but the crash is very random for me (if iI ever got it).

Am i correct that uuids are actually created quite continuously? Looks like it only happens when installing addons...

@koying

This comment has been minimized.

Copy link
Contributor

koying commented Oct 23, 2015

I''ll try with reimplementing crossguid code with our own jni lib this weekend, just in case...

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 23, 2015

Isn't it nicer to use a native implementation instead of going through java anyway? Seems >= api 21 even ships a uuid.h now.

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Oct 23, 2015

@koying: UUIDs are generated by the event log mechanism so everytime a new event is generated a UUID is generated. An event is generated on startup, when installing/uninstalling/enabling/disabling addons etc. But if it would crash on every call it should never make it past startup.

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 23, 2015

@MrMC there is absolutely zero in the logcat, no trace at all.
anyway, it crashes here: https://github.com/graeme-hill/crossguid/blob/master/guid.cpp#L246

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 23, 2015

sorry for the spam. this is the correct line: https://github.com/graeme-hill/crossguid/blob/master/guid.cpp#L254

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Oct 23, 2015

What I noticed is that it didn't happen (well some occurrences) on the nvidia shield which is 64bit and did happen on my phone which is 32bit where it happened 100%. Maybe it's a small detail that could give some indication

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Oct 23, 2015

Might be a compatibility issue with Java's long data type then.
@wsnipex: Can you try http://android-developers.blogspot.ch/2011/07/debugging-android-jni-with-checkjni.html and see if you still don't get any JNI related errors in adb logcat?

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 23, 2015

It happens on shield as well. this one is from the shield with checkjni on:
http://pastebin.com/w9LAnaUa

@koying

This comment has been minimized.

Copy link
Contributor

koying commented Oct 23, 2015

Isn't it nicer to use a native implementation instead of going through java anyway? Seems >= api 21 even ships a uuid.h now.

Ah misread the PR. Yeah, if libuuid is the same for all platforms, I'm definitely +1 on using a native implementation rather than a jni one.

@wsnipex wsnipex added the Type: Fix label Oct 23, 2015

@koying

This comment has been minimized.

Copy link
Contributor

koying commented Oct 23, 2015

ping @Memphiz and @popcornmix because it affects depend

@Memphiz

This comment has been minimized.

Copy link
Member

Memphiz commented Oct 23, 2015

Looks good to me in general. Maybe a comment in that Makefile why we download the e2fsprog tarball when we want to build libuuid for clearing this up a bit.

@wsnipex wsnipex force-pushed the wsnipex:droid-uuid branch from e4deb58 to 94faa07 Oct 23, 2015

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 23, 2015

An explicit comment in the makefile that it only installs libuuid would be nice for some future coder that wonders why e2fsprogs was in depends. It sort of hints to that now. But then I tend to comment the hell out of things like this where something special is being done.

Also, any reason for the change in depends Makefile code convention ?

cd $(PLATFORM)/lib/uuid ; $(MAKE) -j1

vs

$(MAKE) -j1 -C $(PLATFORM)/lib/uuid

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 23, 2015

yeah, it makes it work

@popcornmix

This comment has been minimized.

Copy link
Member

popcornmix commented Oct 23, 2015

Doesn't seem to break my build on Pi so no objections here.

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 24, 2015

jenkins build this please.

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 24, 2015

jenkins build this please

@wsnipex

This comment has been minimized.

Copy link
Member Author

wsnipex commented Oct 25, 2015

good to go?

@mkortstiege mkortstiege added this to the Jarvis 16.0-alpha4 milestone Oct 25, 2015

mkortstiege added a commit that referenced this pull request Oct 25, 2015

Merge pull request #8278 from wsnipex/droid-uuid
[droid] fix crashing on addon installation

@mkortstiege mkortstiege merged commit 0c47ea4 into xbmc:master Oct 25, 2015

1 check failed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.