Skip to content

Fix ButtonClicker random crash issue [ the crash happens on 64 bit gpg sdk ] #20

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.

Sign up for GitHub

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

Closed
ggfan wants to merge 3 commits into from
Closed

Fix ButtonClicker random crash issue [ the crash happens on 64 bit gpg sdk ] #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6afd45
fixing ButtongClicker 64 bit crash problem
ggfan Aug 31, 2015
58e7062
adding check to gpg login when UI is not ready
ggfan Aug 31, 2015
98d0da4
addressing code review issue
ggfan Sep 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions samples-android/ButtonClicker/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.google.example.games.bc3"
package="com.google.example.games.bc"
android:versionCode="1"
android:versionName="1.0" >

<uses-sdk
android:minSdkVersion="11"
android:targetSdkVersion="19" />
android:targetSdkVersion="22" />

<uses-feature android:glEsVersion="0x00020000" >
</uses-feature>
Expand Down Expand Up @@ -47,4 +47,4 @@
</intent-filter>
</activity>
</application>
</manifest>
</manifest>
5 changes: 3 additions & 2 deletions samples-android/ButtonClicker/jni/Application.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
APP_PLATFORM := android-9
Copy link
Contributor

@claywilkinson claywilkinson Aug 31, 2015

Choose a reason for hiding this comment

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

can we keep this at 9?

Copy link
Contributor Author

@ggfan ggfan Sep 1, 2015

Choose a reason for hiding this comment

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

yes, sure

#APP_ABI := all
APP_ABI := armeabi-v7a
#,arm64-v8a

#APP_ABI:= arm64-v8a
#APP_ABI := all

APP_STL := c++_static

Expand Down
27 changes: 14 additions & 13 deletions samples-android/ButtonClicker/jni/ButtonClickerNativeActivity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,14 @@ void Engine::InitGooglePlayGameServices() {
*
*/
void Engine::OnAuthActionStarted(gpg::AuthOperation op) {
startup_mutex_.lock();
if (!initialized_resources_) return;

ndk_helper::JNIHelper::GetInstance()->RunOnUiThread([this, op]() {
EnableUI(false);
authorizing_ = true;
if (op == gpg::AuthOperation::SIGN_IN) {
Copy link
Contributor

@claywilkinson claywilkinson Aug 31, 2015

Choose a reason for hiding this comment

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

can you keep the log messages? They help diagnose issues which there always are with authentication.

Copy link
Contributor Author

@ggfan ggfan Aug 31, 2015

Choose a reason for hiding this comment

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

removed a couple of LOG messages from sample to make it clean. when UI is up, it is displayed UI page at the bottom in status_text. so we could make it a little clean in logcat

Copy link
Contributor

@claywilkinson claywilkinson Sep 2, 2015

Choose a reason for hiding this comment

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

But when there is a problem, we ask for a log so we can see what is going on - so now we can't see what is going on. Please put them back.

LOGI("Signing in to GPG");
status_text_->SetAttribute("Text", "Signing In...");
} else {
LOGI("Signing out from GPG");
status_text_->SetAttribute("Text", "Signing Out...");
}
});
Expand All @@ -135,17 +134,19 @@ void Engine::OnAuthActionFinished(gpg::AuthOperation op,
}
});
}

if (!initialized_resources_) return;


if (!initialized_resources_) {
startup_mutex_.unlock();
return;
}
ndk_helper::JNIHelper::GetInstance()->RunOnUiThread([this, status]() {
EnableUI(true);
authorizing_ = false;
button_sign_in_->SetAttribute(
"Text", gpg::IsSuccess(status) ? "Sign Out" : "Sign In");

status_text_->SetAttribute(
"Text", gpg::IsSuccess(status) ? "Signed In" : "Signed Out");
startup_mutex_.unlock();
});
}

Expand Down Expand Up @@ -560,8 +561,14 @@ void Engine::LeaveGame() {
* invoking jui_helper functions to create java UIs
*/
void Engine::InitUI() {
// The window initialization
jui_helper::JUIWindow::Init(app_->activity, JUIHELPER_CLASS_NAME);

// Show toast with app label
ndk_helper::JNIHelper::GetInstance()->RunOnUiThread([]() {
if(NULL == jui_helper::JUIWindow::GetInstance()->GetContext()) {
return;
}
jui_helper::JUIToast toast(
ndk_helper::JNIHelper::GetInstance()->GetAppLabel());
toast.Show();
Expand All @@ -572,9 +579,6 @@ void Engine::InitUI() {
// UIs.
//

// The window initialization
jui_helper::JUIWindow::Init(app_->activity, JUIHELPER_CLASS_NAME);

//
// Buttons
//
Expand All @@ -586,7 +590,6 @@ void Engine::InitUI() {
jui_helper::LAYOUT_PARAMETER_TRUE);
button_sign_in_->SetCallback([this](jui_helper::JUIView *view,
const int32_t message) {
LOGI("button_sign_in_ click: %d", message);
if (message == jui_helper::JUICALLBACK_BUTTON_UP) {
if (service_->IsAuthorized()) {
service_->SignOut();
Expand Down Expand Up @@ -647,7 +650,6 @@ void Engine::InitUI() {
// Init play game services
InitGooglePlayGameServices();

if (authorizing_) EnableUI(false);
return;
}

Expand All @@ -669,7 +671,6 @@ void Engine::SetParameters(jui_helper::JUIButton *button,
* Enable/Disable management UI
*/
void Engine::EnableUI(bool enable) {
LOGI("Updating UI:%d", enable);
ndk_helper::JNIHelper::GetInstance()->RunOnUiThread([this, enable]() {
button_sign_in_->SetAttribute("Enabled", enable);

Expand Down
2 changes: 1 addition & 1 deletion samples-android/ButtonClicker/jni/ButtonClickerNativeActivity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,13 @@ class Engine : public gpg::IRealTimeEventListener {
// in OnRoomStatusChanged()
int32_t score_counter_; // Score counter of local player
bool playing_; // Am I playing a game?
bool authorizing_; // Am I signing in to gpg service?
std::string self_id_; // Local player's ID
double start_time_; // Game start time

// synchronization primitive to synchronize
// UIThread, Timer thread and gpg callback thread
mutable std::mutex mutex_;
mutable std::mutex startup_mutex_;
Copy link
Contributor

@claywilkinson claywilkinson Aug 31, 2015

Choose a reason for hiding this comment

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

can you add a comment on why 2 mutex are needed? It seems like 1 should be ok?

Copy link
Contributor Author

@ggfan ggfan Aug 31, 2015

Choose a reason for hiding this comment

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

this one is just for start up code: taking conservative way not to make a big change to the existing code.


// Renderer of a teapot
TeapotRenderer renderer_;
Expand Down
4 changes: 2 additions & 2 deletions samples-android/ButtonClicker/jni/ButtonClickerNativeActivity_Engine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
Engine::Engine()
: initialized_resources_(false),
has_focus_(false),
authorizing_(false),
app_(nullptr),
dialog_(nullptr),
textViewFPS_(nullptr),
Expand Down Expand Up @@ -61,10 +60,12 @@ void Engine::UnloadResources() { renderer_.Unload(); }
*/
int Engine::InitDisplay(const int32_t cmd) {
if (!initialized_resources_) {
startup_mutex_.lock();
gl_context_->Init(app_->window);
InitUI();
LoadResources();
initialized_resources_ = true;
startup_mutex_.unlock();
Copy link
Contributor

@claywilkinson claywilkinson Aug 31, 2015

Choose a reason for hiding this comment

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

can you catch exceptions to make sure the mutex gets unlocked?

Copy link
Contributor Author

@ggfan ggfan Sep 1, 2015

Choose a reason for hiding this comment

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

this is good point. if there is exception in native code, it crashes and there is not any catcher here. I think java code handles lot of exceptions; in side native code, I do not see too much yet.

} else {
// initialize OpenGL ES and EGL
if (EGL_SUCCESS != gl_context_->Resume(app_->window)) {
Expand Down Expand Up @@ -192,7 +193,6 @@ int32_t Engine::HandleInput(android_app *app, AInputEvent *event) {
*/
void Engine::HandleCmd(struct android_app *app, int32_t cmd) {
Engine *eng = (Engine *)app->userData;
LOGI("message %d", cmd);
switch (cmd) {
case APP_CMD_SAVE_STATE:
break;
Expand Down
2 changes: 1 addition & 1 deletion samples-android/build_sample.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ trap cleanup EXIT
# Copy the lib project and run "android update lib-project" on it.
# This requires a target, which apparently needs to be android-10.
cp -r ${lib_project} ${private_lib}
${android_tool} update lib-project --path ${private_lib} --target android-10
${android_tool} update lib-project --path ${private_lib} --target android-22

#
# At last, build!
Expand Down