-
Notifications
You must be signed in to change notification settings - Fork 615
Fail Firestore client if we can't establish SSL connection #508
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
Changes from 6 commits
393bbd5
48363ca
de30749
a4ff139
4535883
b863ed2
b188eae
288ec66
d355738
080792b
da32187
fc58b41
b096ac4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,10 @@ | |
- [feature] Added `clearPersistence()`, which clears the persistent storage | ||
including pending writes and cached documents. This is intended to help | ||
write reliable tests (https://github.com/firebase/firebase-js-sdk/issues/449). | ||
|
||
- [changed] Instead of failing silently, Firestore now crahses the client app | ||
if it fails to load SSL Ciphers. To avoid these crashes, you must bundle | ||
Conscrupt if you support non-GMSCore devices on Android KitKat or JellyBean | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another typo: Conscrypt. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was already fixed: 080792b |
||
(see https://github.com/grpc/grpc-java/blob/master/SECURITY.md#tls-on-android). | ||
|
||
# 19.0.1 | ||
- [fixed] Fixed an issue that prevented schema migrations for clients with | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,9 @@ | |
|
||
package com.google.firebase.firestore.util; | ||
|
||
import androidx.annotation.Nullable; | ||
import android.os.Handler; | ||
import android.os.Looper; | ||
import android.support.annotation.Nullable; | ||
import com.google.android.gms.tasks.Continuation; | ||
import com.google.cloud.datastore.core.number.NumberComparisonHelper; | ||
import com.google.firebase.firestore.FieldPath; | ||
|
@@ -211,4 +213,13 @@ public static String toDebugString(ByteString bytes) { | |
public static String typeName(@Nullable Object obj) { | ||
return obj == null ? "null" : obj.getClass().getName(); | ||
} | ||
|
||
/** Raises an exception on Android's UI Thread and crashes the end user's app. */ | ||
public static void crashMainThread(RuntimeException exception) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nit: the exception's stacktrace is constructed when the exception is created, not when it is thrown. So the throw site(below) will not be part of the resulting stacktrace, make sure it is ok and won't cause confusion as I am not familiar enough with your codebase to make such a call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Stacktrace reflects the callsite for this function, which is either AbstractStream or FirestoreChannel. To me, those are the correct places and I prefer this over crashing from Util. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sgtm |
||
new Handler(Looper.getMainLooper()) | ||
.post( | ||
() -> { | ||
throw exception; | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: crahses -> crashes
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks :)