Skip to content

Fix GetPlatformAppByName() crash on KitKat when the app was not found#429

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

Merged
merged 5 commits into from
May 19, 2021

Conversation

dconeybe
Copy link
Contributor

This PR fixes a bug in GetPlatformAppByName() if an exception is thrown by the Java method call. The issue is that if an exception is thrown then the return value from CallStaticObjectMethod() is NULL... except on KitKat, where it appears to return garbage instead. This garbage value was being returned and the caller was incorrectly treating it as a valid object reference. The fix is to check if an exception was thrown and explicitly returning NULL in that case, instead of relying on the return value from CallStaticObjectMethod() being NULL.

This bug surfaced in the integration test FirestoreIntegrationTest.CanPageThroughItems which was crashing on KitKat. The root cause was that it was using a custom app name and the call to GetPlatformAppByName() was returning garbage in that case.

static jobject GetPlatformAppByName(JNIEnv* jni_env, constchar* name) {
jobject platform_app;
if (app_common::IsDefaultAppName(name)) {
platform_app = jni_env->CallStaticObjectMethod(
app::GetClass(), app::GetMethodId(app::kGetInstance));
} else {
jobject name_string = jni_env->NewStringUTF(name);
platform_app = jni_env->CallStaticObjectMethod(
app::GetClass(), app::GetMethodId(app::kGetInstanceByName),
name_string);
jni_env->DeleteLocalRef(name_string);
}
jni_env->ExceptionCheck();
jni_env->ExceptionClear();
return platform_app;
}

TEST_F(FirestoreIntegrationTest, CanPageThroughItems) {
CollectionReference collection =
Collection({{"a", {{"v", FieldValue::String("a")}}},
{"b", {{"v", FieldValue::String("b")}}},
{"c", {{"v", FieldValue::String("c")}}},
{"d", {{"v", FieldValue::String("d")}}},
{"e", {{"v", FieldValue::String("e")}}},
{"f", {{"v", FieldValue::String("f")}}}});

…xceptionCheck() and return NULL if an exception was thrown.
@dconeybedconeybe self-assigned this May 17, 2021
@dconeybedconeybe added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label May 17, 2021
@github-actionsgithub-actionsbot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels May 17, 2021
@github-actionsgithub-actionsbot added the tests: failed This PR's integration tests failed. label May 17, 2021
@dconeybedconeybe requested review from stewartmiles and alexames and removed request for stewartmilesMay 17, 2021 18:58
@dconeybedconeybe assigned alexames and unassigned dconeybeMay 17, 2021
@firebase-workflow-triggerfirebase-workflow-triggerbot removed the tests: in-progress This PR's integration tests are in progress. label May 17, 2021
@dconeybedconeybe added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label May 18, 2021
@github-actionsgithub-actionsbot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). tests: failed This PR's integration tests failed. labels May 18, 2021
@firebase-workflow-triggerfirebase-workflow-triggerbot removed the tests: in-progress This PR's integration tests are in progress. label May 18, 2021
@github-actionsgithub-actionsbot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests: failed This PR's integration tests failed. labels May 19, 2021
@firebase-workflow-triggerfirebase-workflow-triggerbot removed the tests: in-progress This PR's integration tests are in progress. label May 19, 2021
@github-actionsgithub-actionsbot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests: succeeded This PR's integration tests succeeded. labels May 19, 2021
@firebase-workflow-triggerfirebase-workflow-triggerbot removed the tests: in-progress This PR's integration tests are in progress. label May 19, 2021
…stCrash to pick up #430, which will fix some of the integration test failures by setting the minSdkVersion of the messaging test to 16 (was 26).
@dconeybedconeybe added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label May 19, 2021
@github-actionsgithub-actionsbot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). tests: succeeded This PR's integration tests succeeded. labels May 19, 2021
@github-actions
Copy link

github-actionsbot commented May 19, 2021

❌  Integration test FAILED

Requested by @dconeybe on commit 57d29a5
Last updated: Wed May 19 12:33:28 PDT 2021
View integration test results

Platform (Build Config)Build failuresTest failures (Test Devices)
Android (built on Linux)firestore (Nexus10+19)
installations (Pixel2+28, flame+29)
messaging (Nexus10+19, Pixel2+28)
Android (built on MacOS)firestore (Nexus10+19)
messaging (Nexus10+19, Pixel2+28)
Android (built on Windows)firestore (Nexus10+19)
messaging (Nexus10+19, Pixel2+28)

@github-actionsgithub-actionsbot added the tests: failed This PR's integration tests failed. label May 19, 2021
@firebase-workflow-triggerfirebase-workflow-triggerbot removed the tests: in-progress This PR's integration tests are in progress. label May 19, 2021
@dconeybedconeybe removed the tests: failed This PR's integration tests failed. label May 19, 2021
@dconeybe
Copy link
ContributorAuthor

Note: I'm going to ignore the test failures because they are all flakes that could not possibly be triggered by this PR.

@dconeybedconeybe merged commit d5b719c into mainMay 19, 2021
@dconeybedconeybe deleted the dconeybe/FixFirestoreCanPageThroughItemsTestCrash branch May 19, 2021 19:38
@firebasefirebase locked and limited conversation to collaborators Jun 19, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2 participants
@dconeybe@alexames
close