Skip to content

Fix crash by disabling Flipper on API 22 and below #7428

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

Merged
merged 6 commits into from
Oct 21, 2022
Merged

Fix crash by disabling Flipper on API 22 and below #7428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ada6c58
Disable Flipper on API 21 and below
jonnyandrew Oct 21, 2022
617e2be
Add changelog entry
jonnyandrew Oct 21, 2022
f96f766
Update changelog
jonnyandrew Oct 21, 2022
5425622
Refactor
jonnyandrew Oct 21, 2022
1080740
Address review comments
jonnyandrew Oct 21, 2022
8d2315d
Add jonnyandrew to PR sign-off allowlist
jonnyandrew Oct 21, 2022
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
1 change: 1 addition & 0 deletions changelog.d/7428.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix crash by disabling Flipper on Android API 22 and below - only affects debug version of the application.
1 change: 1 addition & 0 deletions tools/danger/dangerfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ const allowList = [
"Florian14",
"ganfra",
"jmartinesp",
"jonnyandrew",
"langleyd",
"MadLittleMods",
"manuroe",
Expand Down
56 changes: 36 additions & 20 deletions vector-app/src/debug/java/im/vector/app/flipper/VectorFlipperProxy.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package im.vector.app.flipper

import android.content.Context
import android.os.Build
import com.facebook.flipper.android.AndroidFlipperClient
import com.facebook.flipper.android.utils.FlipperUtils
import com.facebook.flipper.plugins.crashreporter.CrashReporterPlugin
Expand All @@ -31,6 +32,7 @@ import com.kgurgul.flipper.RealmDatabaseDriver
import com.kgurgul.flipper.RealmDatabaseProvider
import im.vector.app.core.debug.FlipperProxy
import io.realm.RealmConfiguration
import okhttp3.Interceptor
import org.matrix.android.sdk.api.Matrix
import javax.inject.Inject
import javax.inject.Singleton
Expand All @@ -41,29 +43,43 @@ class VectorFlipperProxy @Inject constructor(
) : FlipperProxy {
private val networkFlipperPlugin = NetworkFlipperPlugin()

private val isEnabled: Boolean
get() {
// https://github.com./facebook/flipper/issues/3572
if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.LOLLIPOP_MR1) {
return false
}

return FlipperUtils.shouldEnableFlipper(context)
}
Copy link
Member

Choose a reason for hiding this comment

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

Why this is not done above the line SoLoader.init(context, false)?

Also I would also update the fun networkInterceptor() to return null if Build.VERSION.SDK_INT <= Build.VERSION_CODES.LOLLIPOP_MR1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it here to keep this logic at the same point as the existing check (on the line immediately after this one).

In the docs SoLoader.init() is always called regardless of whether Flipper is enabled but I'm not sure if there's any reason for that.

The same is true of the networkInterceptor() but I'd agree, it should return null if Flipper is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

5425622


override fun init(matrix: Matrix) {
if (!isEnabled) return

SoLoader.init(context, false)

if (FlipperUtils.shouldEnableFlipper(context)) {
val client = AndroidFlipperClient.getInstance(context)
client.addPlugin(CrashReporterPlugin.getInstance())
client.addPlugin(SharedPreferencesFlipperPlugin(context))
client.addPlugin(InspectorFlipperPlugin(context, DescriptorMapping.withDefaults()))
client.addPlugin(networkFlipperPlugin)
client.addPlugin(
DatabasesFlipperPlugin(
RealmDatabaseDriver(
context = context,
realmDatabaseProvider = object : RealmDatabaseProvider {
override fun getRealmConfigurations(): List<RealmConfiguration> {
return matrix.debugService().getAllRealmConfigurations()
}
})
)
)
client.start()
}
val client = AndroidFlipperClient.getInstance(context)
client.addPlugin(CrashReporterPlugin.getInstance())
client.addPlugin(SharedPreferencesFlipperPlugin(context))
client.addPlugin(InspectorFlipperPlugin(context, DescriptorMapping.withDefaults()))
client.addPlugin(networkFlipperPlugin)
client.addPlugin(
DatabasesFlipperPlugin(
RealmDatabaseDriver(
context = context,
realmDatabaseProvider = object : RealmDatabaseProvider {
override fun getRealmConfigurations(): List<RealmConfiguration> {
return matrix.debugService().getAllRealmConfigurations()
}
})
)
)
client.start()
}

override fun networkInterceptor() = FlipperOkhttpInterceptor(networkFlipperPlugin)
override fun networkInterceptor(): Interceptor? {
if (!isEnabled) return null

return FlipperOkhttpInterceptor(networkFlipperPlugin)
}
}