Bundled SQLite crashes due to thread safe mode [340949940]

Fixed

Bug

Status Update

No update yet.

Description

ib...@gmail.com

created issue #1

May 16, 2024 05:02AM

Component used: bundled androidx.sqlite:sqlite-bundled
Version used: 2.5.0-alpha02
Devices/Android versions reproduced on: Pixel 6

I tried to migrate from

https://github.com/requery/sqlite-android to this library and I noticed that my queries started to cause app crashes from time to time. Here is the Logcat output:

2024-05-16 11:41:26.367 30919-30967 libc org.btcmap.debug A Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x8000004300003b in tid 30967 (DefaultDispatch), pid 30919 (rg.btcmap.debug)

2024-05-16 11:41:26.662 31039-31039 DEBUG pid-31039 A Cmdline: org.btcmap.debug
2024-05-16 11:41:26.662 31039-31039 DEBUG pid-31039 A pid: 30919, tid: 30967, name: DefaultDispatch >>> org.btcmap.debug <<<
2024-05-16 11:41:26.662 31039-31039 DEBUG pid-31039 A #00 pc 00000000000cb30c /data/app/~~98VGkyL4jH13WZ5B1K-MmQ==/org.btcmap.debug-MmWOzd6cam3KXcL8NYi-uA==/base.apk!libsqliteJni.so (offset 0x930000)

The app itself is open source and it doesn't need any extra setup, you can easily reproduce it by launching it in the emulator or on a real device. Usually it crashes before moving the map, but it will crash for sure if you start moving the map, since it triggers a few queries.

https://github.com/teambtcmap/btcmap-android/commit/1d60da058a10ed1efaff307af4bcf3e5378fc2a2

It's possible that I messed up a few queries during that transition, but I don't think it should cause crashes, and it can surely be more specific on where it's failing and why

Comments

da...@google.com <da...@google.com> #2May 16, 2024 12:34PM

The issue is reproducible with core-ktx 1.2.0 and 1.3.0-rc01.

ib...@gmail.com <ib...@gmail.com> #3May 16, 2024 01:01PM

The Typeface.weight is not a weight of the underlying font file. It is a display style. On older APIs, the display style is adjusted if the Typeface is created from single font. However, after moving to CustomFallbackBuilder, that adjustment is removed since it can crate Typeface from multiple style font files.

Looks like it is good to set display style by ResourcesCompat.getFont for backward compatibility.

da...@google.com <da...@google.com> #4May 16, 2024 01:52PM

Hi Nona,

Can you please schedule a release after you merge the fix?

ib...@gmail.com <ib...@gmail.com> #5May 17, 2024 12:04PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 3d6aa2e9b3243dcc4de1f54bd8d40339bd69cb05
Author: Seigo Nonaka <nona@google.com>
Date: Wed May 27 17:38:05 2020

Adjust the Typeface display style with the style of given font

This behavir is implicitly done by Typeface.Builder and
Typeface.createXXX function but not to be done by
Typeface.CustomFallbackBuilder since it is designed to be working
with multiple font files which has different style.

Looks like the style argument is ignored on older API implementation.

Bug: 156853883
Bug: 152023266
Test: ResourcesCompatTest#testGetFont_adjustDisplayStyle passes on 29
Test: ./gradlew core:core:connectedAndroidTest on API 29, 28, 23
Change-Id: I3a377c339a7aed50973cf11df86ddf0069f4ec25

A core/core/src/androidTest/assets/fonts/thin_italic.ttf
A core/core/src/androidTest/assets/fonts/thin_italic.ttx
M core/core/src/androidTest/java/androidx/core/content/res/ResourcesCompatTest.java
A core/core/src/androidTest/res/font/thin_italic.ttf
M core/core/src/main/java/androidx/core/graphics/TypefaceCompatApi29Impl.java

https://android-review.googlesource.com/1318947

da...@google.com <da...@google.com> #6May 30, 2024 03:49PM

The following changes were cherrypicked through Coastguard Release Track: (https://android-build.googleplex.com/coastguard/dashboard/6222780589080576/#/history/all) Changes: aosp/1318947

ap...@google.com <ap...@google.com> #7May 31, 2024 08:37PM

Any way I can tell what version this will land in?

da...@google.com <da...@google.com> May 31, 2024 08:38PM

Marked as fixed.

ap...@google.com <ap...@google.com> #8Jun 3, 2024 05:52PM

should be in 1.5.0-alpha1 https://developer.android.com/jetpack/androidx/releases/core#1.5.0-alpha01

pr...@google.com <pr...@google.com> #9Jun 12, 2024 10:45PM

Great—works as expected, thanks!

ib...@gmail.com <ib...@gmail.com> #10Sep 23, 2024 01:52PM

I just tested this new flag and it looks like the issue still persists. It's easily reproducible since the repo is open source:

https://github.com/teambtcmap/btcmap-android/commit/9fe6be3604067ae1a1bd8b9c576b014023bff6ec

So default open() passes SQLITE_OPEN_READWRITE or SQLITE_OPEN_CREATE flags, and I changed it to SQLITE_OPEN_READWRITE or SQLITE_OPEN_CREATE or SQLITE_OPEN_FULLMUTEX

The database works fine after that change, but when I tried to disable the global connection lock (connLock variable) in the same file, the app started to crash with "android.database.SQLException: Error code: 21, message: no row"

android.database.SQLException: Error code: 21, message: no row
    at androidx.sqlite.driver.bundled.BundledSQLiteStatementKt.nativeGetLong(Native Method)
    at androidx.sqlite.driver.bundled.BundledSQLiteStatementKt.access$nativeGetLong(BundledSQLiteStatement.jvmAndroid.kt:1)
    at androidx.sqlite.driver.bundled.BundledSQLiteStatement.getLong(BundledSQLiteStatement.jvmAndroid.kt:70)
    at element.ElementQueries$selectClusters$1.invoke(ElementQueries.kt:271)

da...@google.com <da...@google.com> #11Sep 25, 2024 03:31PM

Status: Assigned (reopened)

The flags are working as expected, but looks like the issue is in our JNI implementation, specifically we use sqlite3_errcode to get the 'last result code' and that is dangerous since it is a connection level API and if the connection is being used concurrently the 'last result code' of a statement might be from a different statement being executed, in other words we need to localize the 'last result code' per statement. I'll work on a fix. Again, I appreciate the feedback and you trying out the driver.

Message last modified on Sep 25, 2024 03:31PM

ap...@google.com <ap...@google.com> #12Sep 26, 2024 07:06PM

Project: platform/frameworks/support
Branch: androidx-main
Author: Daniel Santiago Rivera <danysantiago@google.com>
Link: https://android-review.googlesource.com/3282515

Use sqlite3_stmt_busy to check if there are rows in a statement.

Expand for full commit details

Use sqlite3_stmt_busy to check if there are rows in a statement.

Instead of using 'sqlite3_errcode(db) == SQLITE_ROW' to check if a statement is in a row and ready to read columns, use sqlite3_stmt_busy as that API is local to the statement and avoids the risk of sqlite3_errcode that works on a database connection returning a result code that is irrelevant to the statement when the connection is being used concurrently.

Bug: 340949940
Test: BaseBundledConformanceTest.openWithFullMutexFlag
Change-Id: If02039b90c6415a7532ae94f1c99e1182b546db8

Files:

M sqlite/integration-tests/driver-conformance-test/src/commonTest/kotlin/androidx/sqlite/driver/test/BaseBundledConformanceTest.kt
M sqlite/sqlite-bundled/src/jvmAndroidMain/jni/sqlite_bindings.cpp
M sqlite/sqlite-framework/src/nativeMain/kotlin/androidx/sqlite/driver/NativeSQLiteStatement.kt

Hash: 5885a89d74a47b73d2e05abefc979279b85c14fe
Date: Wed Sep 25 12:13:07 2024

da...@google.com <da...@google.com> Sep 26, 2024 07:08PM