Provide an API to group modifiers under one InspectorInfo [191017532]

Fixed

Bug

Status Update

No update yet.

Description

lp...@google.com

created issue #1

Jun 14, 2021 01:41PM

Currently we use Modifier.composed in many places to 'wrap' modifiers with a new InspectorInfo, while ignoring any infos used internally in the implementation detail of the modifiers.

For example, Modifier.selectable currently looks like:

composed(
    factory = {
        Modifier.clickable(
            enabled = enabled,
            role = role,
            interactionSource = interactionSource,
            indication = indication,
            onClick = onClick
        ).semantics {
            this.selected = selected
        }
    },
    inspectorInfo = debugInspectorInfo {
        name = "selectable"
        properties["selected"] = selected
        properties["enabled"] = enabled
        properties["role"] = role
        properties["interactionSource"] = interactionSource
        properties["indication"] = indication
        properties["onClick"] = onClick
    }
)

In this case using composed is important, as we want to hide the clickable's InspectorInfo, and replace it with a more meaningful one for selectable.

However, using Modifier.composed like this when there are no Composable functions being called in the implementation is expensive - it means that the modifier can never be skipped, and this can cause performance degradation. As a result, this shouldn't be composed, and we will need a new specific API to ignore the internal info.

Comments

lp...@google.com <lp...@google.com> #2Jun 14, 2021 01:42PM

A couple of questions:

1. Have you saw crash in real device or only in simulators?
2. Do you use dynamic feature for language ID?

ap...@google.com <ap...@google.com> #3Jun 17, 2021 12:41AM

Tested on Android 12 Emulator with custom executor, but cannot repro this issue.

jl...@google.com <jl...@google.com> Aug 10, 2021 10:34PM

Accepted by jl...@google.com.

ap...@google.com <ap...@google.com> #4Aug 20, 2021 10:31PM

Marked as fixed.

Second crash in the description is from a real device. Experienced it myself on two different Xiaomi phones, plus lots of crashes from users in the Google Play console.
Dynamic features are not used in the application.

As a wild guess, I have downgraded build tools from 31.0.0 to 30.0.3, compileSdk from 31 to 30, and moved all work with Language ID to the service in a separate process (just to be sure that crash can kill secondary process instead of main). This combination is in beta for 2 days by now and I don't see any SIGSEGV crashes.

lp...@google.com <lp...@google.com> #5Aug 21, 2021 07:09PM

Status: Assigned (reopened)

Hmm, I feel the crash might be something related to separate/secondary process.

I also changed compileSdk and targetSDK to 31 but still cannot repro this issue.

ap...@google.com <ap...@google.com> #6Aug 26, 2021 07:13PM

On the contrary, there was no separate process before, when crashes started.

In the new build (with the aforementioned changes) I can see SIGSEGV crash, but only one instead of dozens and it has a bit different backtrace:

signal 11 (SIGSEGV), code 1 (SEGV_MAPERR)
liblanguage_id_jni.so (offset 0x11e000)

backtrace:
  #00  pc 000000000003c7c0  /data/app/azagroup.reedy-mF7zTu2bv_ELlbFArwNgqA==/split_config.arm64_v8a.apk!lib/arm64-v8a/liblanguage_id_jni.so (offset 0x11e000)
  #00  pc 000000000003b960  /data/app/azagroup.reedy-mF7zTu2bv_ELlbFArwNgqA==/split_config.arm64_v8a.apk!lib/arm64-v8a/liblanguage_id_jni.so (offset 0x11e000)
  #00  pc 000000000003bb48  /data/app/azagroup.reedy-mF7zTu2bv_ELlbFArwNgqA==/split_config.arm64_v8a.apk!lib/arm64-v8a/liblanguage_id_jni.so (offset 0x11e000)
  #00  pc 000000000003bafc  /data/app/azagroup.reedy-mF7zTu2bv_ELlbFArwNgqA==/split_config.arm64_v8a.apk!lib/arm64-v8a/liblanguage_id_jni.so (offset 0x11e000)
  #00  pc 0000000000036c98  /data/app/azagroup.reedy-mF7zTu2bv_ELlbFArwNgqA==/split_config.arm64_v8a.apk!lib/arm64-v8a/liblanguage_id_jni.so (offset 0x11e000)
  #00  pc 0000000000032714  /data/app/azagroup.reedy-mF7zTu2bv_ELlbFArwNgqA==/split_config.arm64_v8a.apk!lib/arm64-v8a/liblanguage_id_jni.so (offset 0x11e000)
  #00  pc 0000000000031cac  /data/app/azagroup.reedy-mF7zTu2bv_ELlbFArwNgqA==/split_config.arm64_v8a.apk!lib/arm64-v8a/liblanguage_id_jni.so (offset 0x11e000)
  #00  pc 0000000000057438  /data/app/azagroup.reedy-mF7zTu2bv_ELlbFArwNgqA==/oat/arm64/base.odex (offset 0x57000)

ap...@google.com <ap...@google.com> #7Aug 26, 2021 07:13PM

FYI, ML Kit launched a new language ID SDK in the latest release, which uses a new language ID model. https://developers.google.com/ml-kit/release-notes#august_11_2021

Could you try the new SDK version(17.0.0) to check if you can still repro this native crash? Thanks!

ap...@google.com <ap...@google.com> #8Aug 26, 2021 07:13PM

Thank you, I'll try it and check.

jl...@google.com <jl...@google.com> #9Aug 30, 2021 10:40PM