Fail Firestore client if we can't establish SSL connection #508

schmidt-sebastian · 2019-06-10T18:31:30Z

We should hard-fail the Firestore client if we are running on an old Android device with no SSL ciphers.

I manually verified that this works with the WatchStream, the WriteStream and Transactions. Stacktrace looks as follows:

  java.lang.RuntimeException: Internal error in Firestore (19.0.2).
        at com.google.firebase.firestore.util.AsyncQueue.lambda$panic$5(AsyncQueue.java:379)
        at com.google.firebase.firestore.util.AsyncQueue$$Lambda$5.run(AsyncQueue.java)
        at android.os.Handler.handleCallback(Handler.java:733)
        at android.os.Handler.dispatchMessage(Handler.java:95)
        at android.os.Looper.loop(Looper.java:136)
        at android.app.ActivityThread.main(ActivityThread.java:5017)
        at java.lang.reflect.Method.invokeNative(Native Method)
        at java.lang.reflect.Method.invoke(Method.java:515)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:779)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:595)
        at dalvik.system.NativeStart.main(Native Method)
     Caused by: java.lang.IllegalStateException: Could not establish a secure connection to the Firestore backend. This is likely a problem with your app, rather than with Firestore itself. See https://bit.ly/2XFpdma for instructions on how to enable TLS on Android 4.x devices.
        at com.google.firebase.firestore.remote.RemoteStore.handleWatchStreamClose(RemoteStore.java:475)
        at com.google.firebase.firestore.remote.RemoteStore.access$200(RemoteStore.java:53)
        at com.google.firebase.firestore.remote.RemoteStore$1.onClose(RemoteStore.java:188)
        at com.google.firebase.firestore.remote.AbstractStream.close(AbstractStream.java:324)
        at com.google.firebase.firestore.remote.AbstractStream.handleServerClose(AbstractStream.java:378)
        at com.google.firebase.firestore.remote.AbstractStream$StreamObserver.lambda$onClose$3(AbstractStream.java:149)
        at com.google.firebase.firestore.remote.AbstractStream$StreamObserver$$Lambda$4.run(AbstractStream.java)
        at com.google.firebase.firestore.remote.AbstractStream$CloseGuardedRunner.run(AbstractStream.java:65)
        at com.google.firebase.firestore.remote.AbstractStream$StreamObserver.onClose(AbstractStream.java:135)
        at com.google.firebase.firestore.remote.FirestoreChannel$1.onClose(FirestoreChannel.java:131)

rsgowman

The code lgtm. But are we able to not raise an Internal error in this case? As discussed in standup, those usually imply a bug in our code, and this situation isn't quite that.

schmidt-sebastian · 2019-06-10T22:56:34Z

The code lgtm. But are we able to not raise an Internal error in this case? As discussed in standup, those usually imply a bug in our code, and this situation isn't quite that.

We can special-case it here but I personally don't think it's worth it (especially since I don't want to do this for all IllegalStateExceptions): https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java#L378

I can be convinced otherwise, but I was hoping that the error message itself addressed the concerns from standup.

schmidt-sebastian · 2019-06-10T23:06:33Z

We could also throw on the UI thread, but I reckon that is pretty frowned upon. @samtstern ?

schmidt-sebastian · 2019-06-11T04:15:50Z

I updated this PR to crash the UI thread instead. That removes the first part of the stacktrace in the description.

Note that I also included EHOSTUNREACH since that is the exception I am getting on my home Wifi.

schmidt-sebastian · 2019-06-11T04:16:59Z

@samtstern Is this a sane thing to do? https://github.com/firebase/firebase-android-sdk/pull/508/files#diff-eaa0a4df27b2101ab36a779a4c14f01fR218

samtstern · 2019-06-11T16:23:18Z

@schmidt-sebastian crashing the user's app is very rude especially when:

This is not a checked exception so they can't catch it (without being exceptionally smart)
It's not dependent on developer input, so we can't rightly call it developer error
There is no proactive checkSsl() method that you could guard your Firestore initialization with

So I'd say it's not reasonable for us to crash the main thread. Better to just fail every single Firestore call or something like that.

wilhuff · 2019-06-11T16:33:48Z

This is dependent on developer input. Namely, if they want to ship for non-GMS devices prior to Android 5.0, they need to build with Conscript. If there were a way to fail at build time we'd do it.

samtstern · 2019-06-11T16:51:32Z

@wilhuff I can see that angle, but a lot of times your app "ends up" on those devices against your will. Especially in the markets where non-GMS devices are more prevalent.

wilhuff · 2019-06-11T17:08:39Z

I don't think failing every method is useful:

Developers don't plan for or test for this failure so it's unlikely they're going to handle this in a way that's right
Additionally, it's likely that since they haven't planned for this kind of failure, their app will do unpredictable things as a result.
Without a crash it's unlikely they're actually going to get any telemetry of the failed state

We don't have any other kind of developer error that fails like this, so implementing error-every-method is pretty expensive.

schmidt-sebastian · 2019-06-11T17:14:40Z

I think I am going to defer the final decision on this to the API council. To me, this is akin to a developer not shipping/declaring a required dependency which would also crash the JVM.

In the meantime, I will make the check a little tighter and only throw on < API level 21.

rsgowman · 2019-06-12T15:42:48Z

I'd still prefer to avoid surfacing this as an Internal error; I like being able to unconditionally say that all Internal errors represent a bug in our code.

Another option might be to introduce a NotOurFaultException (usage: throw new NotOurFaultException(/*cause=*/new IllegalStateException("badness"));), which the function here https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/util/AsyncQueue.java#L378 could explicitly check for and carefully unwrap. It's a pretty gross miss-use of exceptions though.

Extending IllegalArgumentException (SslDependencyMissingException?) and then explicitly checking for it could work. (Doesn't solve the generic case, and would involve us introducing at least one new class... more if there are other cases like this in the future.)

Plumbing a boolean (ourFault=true) through to the above function would work too... but would be a pain. Not worth it, IMO.

I also don't like parsing the IllegalArgumentException message.

I like your idea of just crashing the UI thread. If you can get everyone on board with that, it seems simplest.

(Code still lgtm.)

schmidt-sebastian · 2019-06-19T17:05:24Z

/retest

schmidt-sebastian · 2019-06-19T17:06:01Z

@vkryachko approved this for merging in an internal API discussion thread.

schmidt-sebastian · 2019-06-19T17:12:41Z

Assigning to @rsgowman for approval.

PromanSEW · 2019-06-19T18:55:45Z

firebase-firestore/CHANGELOG.md

@@ -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


Typo: crahses -> crashes

Fixed. Thanks :)

rsgowman · 2019-06-19T20:01:57Z

firebase-firestore/CHANGELOG.md

-  SSL Ciphers during a connection attempt.
+- [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 


Another typo: Conscrypt. :)

This was already fixed: 080792b

vkryachko

It also might make sense to create an integ test for this, not sure how feasible it will be to implement, but you have that option as you can request that the test runs on an old device, e.g. here's how you can control what device(s) tests run on:

firebase-android-sdk/transport/transport-runtime/transport-runtime.gradle

Lines 24 to 33 in 44d0645

    
           device 'model=walleye,version=28' // Pixel2 
        
           device 'model=walleye,version=27' // Pixel2 
        
           device 'model=victara,version=19' // Moto X 
        
           device 'model=Nexus4,version=22' 
        
           device 'model=Nexus7,version=21' 
        
           device 'model=Nexus4,version=19' 
        
           device 'model=starqlteue,version=26' // Galaxy S9 
        
           device 'model=m0,version=18' // Galaxy S3 
        
           device 'model=hero2lte,version=23' // Galaxy S7 
        
           device 'model=htc_m8,version=19' // HTC One (M8)

vkryachko · 2019-06-19T20:08:17Z

firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java

@@ -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) {


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.

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.

schmidt-sebastian · 2019-06-19T20:45:26Z

I am going to merge this without Integration tests. I have verified it manually and a "council of elders" has determined that the benefit of having an Integration tests that verifies that we are crashing does not outweigh its cost.

(*) Still looking for members.

Fail Firestore client if we can't establish SSL connection

393bbd5

googlebot added the cla: yes Override cla label Jun 10, 2019

google-oss-bot added the size/M label Jun 10, 2019

schmidt-sebastian requested a review from rsgowman June 10, 2019 18:31

schmidt-sebastian assigned rsgowman Jun 10, 2019

rsgowman reviewed Jun 10, 2019

View reviewed changes

rsgowman assigned schmidt-sebastian and unassigned rsgowman Jun 10, 2019

schmidt-sebastian assigned rsgowman and unassigned schmidt-sebastian Jun 10, 2019

Crash UI Thread instead

48363ca

schmidt-sebastian assigned samtstern Jun 11, 2019

Unused import

de30749

schmidt-sebastian added 2 commits June 11, 2019 10:15

Merge

a4ff139

Add API Level check

4535883

rsgowman removed their assignment Jun 12, 2019

schmidt-sebastian added 2 commits June 19, 2019 10:10

Update CHANGELOG.md

b863ed2

Remove empty line

b188eae

schmidt-sebastian assigned rsgowman and unassigned samtstern Jun 19, 2019

schmidt-sebastian added 4 commits June 19, 2019 10:18

Merge branch 'master' into mrschmidt-sslerror

288ec66

Using AndroidX

d355738

Fix typo

080792b

Update CHANGELOG.md

da32187

PromanSEW reviewed Jun 19, 2019

View reviewed changes

schmidt-sebastian added 2 commits June 19, 2019 12:57

Update CHANGELOG.md

fc58b41

Merge branch 'master' into mrschmidt-sslerror

b096ac4

rsgowman approved these changes Jun 19, 2019

View reviewed changes

vkryachko approved these changes Jun 19, 2019

View reviewed changes

schmidt-sebastian merged commit 9421586 into master Jun 19, 2019

schmidt-sebastian deleted the mrschmidt-sslerror branch July 3, 2019 15:46

firebase locked and limited conversation to collaborators Oct 10, 2019

	device 'model=walleye,version=28' // Pixel2
	device 'model=walleye,version=27' // Pixel2
	device 'model=victara,version=19' // Moto X
	device 'model=Nexus4,version=22'
	device 'model=Nexus7,version=21'
	device 'model=Nexus4,version=19'
	device 'model=starqlteue,version=26' // Galaxy S9
	device 'model=m0,version=18' // Galaxy S3
	device 'model=hero2lte,version=23' // Galaxy S7
	device 'model=htc_m8,version=19' // HTC One (M8)

Fail Firestore client if we can't establish SSL connection #508

Fail Firestore client if we can't establish SSL connection #508

Uh oh!

Conversation

schmidt-sebastian commented Jun 10, 2019

Uh oh!

rsgowman left a comment

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Jun 10, 2019

Uh oh!

schmidt-sebastian commented Jun 10, 2019

Uh oh!

schmidt-sebastian commented Jun 11, 2019

Uh oh!

schmidt-sebastian commented Jun 11, 2019

Uh oh!

samtstern commented Jun 11, 2019

Uh oh!

wilhuff commented Jun 11, 2019

Uh oh!

samtstern commented Jun 11, 2019

Uh oh!

wilhuff commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian commented Jun 11, 2019

Uh oh!

rsgowman commented Jun 12, 2019

Uh oh!

schmidt-sebastian commented Jun 19, 2019

Uh oh!

schmidt-sebastian commented Jun 19, 2019

Uh oh!

schmidt-sebastian commented Jun 19, 2019

Uh oh!

PromanSEW Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsgowman Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

vkryachko left a comment

Choose a reason for hiding this comment

Uh oh!

vkryachko Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

vkryachko Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Jun 19, 2019

Uh oh!

Uh oh!

wilhuff commented Jun 11, 2019 •

edited

Loading

schmidt-sebastian Jun 19, 2019 •

edited

Loading