Fix GetPlatformAppByName() crash on KitKat when the app was not found by dconeybe · Pull Request #429 · firebase/firebase-cpp-sdk · GitHub

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.

firebase-cpp-sdk/app/src/app_android.cc

Lines 302 to 317 in 4a34edd

    
           static jobject GetPlatformAppByName(JNIEnv* jni_env, const char* 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; 
        
           }

firebase-cpp-sdk/firestore/integration_test_internal/src/cursor_test.cc

Lines 19 to 26 in 4a34edd

    
           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.

…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).

❌ 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 failures	Test 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)

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

Fix GetPlatformAppByName() to actually check the result of jni_env->E…

396d75a

…xceptionCheck() and return NULL if an exception was thrown.

dconeybe self-assigned this May 17, 2021

google-cla Bot added the cla: yes label May 17, 2021

dconeybe added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label May 17, 2021

github-actions Bot 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

Add release notes

4984f09

github-actions Bot added the tests: failed This PR's integration tests failed. label May 17, 2021

dconeybe requested review from alexames and stewartmiles and removed request for stewartmiles May 17, 2021 18:58

dconeybe assigned alexames and unassigned dconeybe May 17, 2021

dconeybe added 2 commits May 17, 2021 15:21

Fix a typo in release_build_files/readme.md

d762ce6

Fix a typo in the typo fix

391acc5

firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label May 17, 2021

dconeybe added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label May 18, 2021

alexames approved these changes May 18, 2021

View reviewed changes

firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label May 18, 2021

github-actions Bot 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-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label May 19, 2021

github-actions Bot 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-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label May 19, 2021

Merge branch 'main' at 94df73a into FixFirestoreCanPageThroughItemsTe…

5989e57

…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).

dconeybe added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label May 19, 2021

github-actions Bot 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 Bot added the tests: failed This PR's integration tests failed. label May 19, 2021

firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label May 19, 2021

dconeybe removed the tests: failed This PR's integration tests failed. label May 19, 2021

dconeybe merged commit d5b719c into main May 19, 2021

dconeybe deleted the dconeybe/FixFirestoreCanPageThroughItemsTestCrash branch May 19, 2021 19:38

firebase locked and limited conversation to collaborators Jun 19, 2021

Fix GetPlatformAppByName() crash on KitKat when the app was not found by dconeybe · Pull Request #429 · firebase/firebase-cpp-sdk · GitHub

Uh oh!

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

Fix GetPlatformAppByName() crash on KitKat when the app was not found#429
dconeybe merged 5 commits into
mainfrom
dconeybe/FixFirestoreCanPageThroughItemsTestCrash

dconeybe commented May 17, 2021

Uh oh!

github-actions Bot commented May 19, 2021 •
edited
Loading

Uh oh!

Uh oh!

dconeybe commented May 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	static jobject GetPlatformAppByName(JNIEnv* jni_env, const char* 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")}}}});

Fix GetPlatformAppByName() crash on KitKat when the app was not found by dconeybe · Pull Request #429 · firebase/firebase-cpp-sdk · GitHub

Uh oh!

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

Conversation

dconeybe commented May 17, 2021

Uh oh!

github-actions Bot commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Integration test FAILED

Uh oh!

dconeybe commented May 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented May 19, 2021 •
edited
Loading