"Emulator" has unnecessary thread-unsafe public method in a multi-threaded lock [348598513]

Fixed

Bug

Status Update

No update yet.

Description

ky...@bytedance.com

created issue #1

Jun 21, 2024 10:39PM

AutoLock appears to be a custom implementation of std::unique_lock/lock_guard. It does, however, a method named isLocked(). There is only one use of the method at the moment: https://cs.android.com/android/platform/superproject/+/emu-master-dev:hardware/google/aemu/base/include/aemu/base/async/RecurrentTask.h;l=120

I believe, that has room for improvement. Firstly, even if we checked isLock() in general, as isLock() is not thread-safe, there is no guarantee (in general) that the same thread can then lock it. It is only useful when there is only one thread that locks/unlocks the given AutoLock object and the thread wants know whether the lock is acquired or not by itself. I don't think that's useful. I think the method being public, it has a good chance to confuse external developers.

My belief is that the AutoLock should be simply replaced with std::unique_lock and/or std::lock_guard, as many uses of the class do not call lock or unlock. And, isLocked() should be removed. The only call site should be implemented without that.

Aside from all mentioned above, I am trying to compile the code with --enable-thread-safety-checks, and the first code that blocked the compilation on Linux was the use of isLocked().

Comments

ky...@bytedance.com <ky...@bytedance.com> #2Jun 21, 2024 10:39PM

alpha01? That's so old! :D

We've found the issue in alpha10 and are working on a fix.

rk...@google.com <rk...@google.com> Jun 22, 2024 03:58AM

Assigned to rk...@google.com.

rk...@google.com <rk...@google.com> #3Jun 23, 2024 04:49AM

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

commit 37ed64d4de87bd4a4365f5c0dfee7b33c8043013
Author: Daniel Santiago Rivera <danysantiago@google.com>
Date: Wed Jan 23 15:46:00 2019

Add missing import of parent Directions class in generated Safe Args.

When generating a parent direction action method in a child directions
class,correctly import the parent direction if its located in another
package. Both JavaPoet and KotlinPoet generate import statements but the
NavWriters were incorrectly creating the class name of the parent type,
specifically it missed the package name which caused the import to be
missing.

Bug: 123307342
Test: ./gradlew navigation:navigation-safe-args-gradle-plugin:test \
navigation:navigation-safe-args-generator:test

Change-Id: Ibeda7a7ec6f31bb1c860dfe1f5cd2a7e7ea6d47d

M navigation/safe-args-generator/src/main/kotlin/androidx/navigation/safe/args/generator/java/JavaNavWriter.kt
M navigation/safe-args-generator/src/main/kotlin/androidx/navigation/safe/args/generator/kotlin/KotlinNavWriter.kt
M navigation/safe-args-generator/src/tests/kotlin/androidx/navigation/safe/args/generator/NavGeneratorTest.kt
M navigation/safe-args-generator/src/tests/kotlin/androidx/navigation/safe/args/generator/NavParserTest.kt
M navigation/safe-args-generator/src/tests/test-data/expected/nav_generator_test/java/nested/LoginFragmentDirections.java
M navigation/safe-args-generator/src/tests/test-data/expected/nav_generator_test/java/nested/RegisterFragmentDirections.java
M navigation/safe-args-generator/src/tests/test-data/expected/nav_generator_test/java/nested_overridden_action/InnerSettingsFragmentDirections.java
M navigation/safe-args-generator/src/tests/test-data/expected/nav_generator_test/java/nested_overridden_action/SettingsFragmentDirections.java
M navigation/safe-args-generator/src/tests/test-data/expected/nav_generator_test/kotlin/nested/LoginFragmentDirections.kt
M navigation/safe-args-generator/src/tests/test-data/expected/nav_generator_test/kotlin/nested/RegisterFragmentDirections.kt
M navigation/safe-args-generator/src/tests/test-data/expected/nav_generator_test/kotlin/nested_overridden_action/InnerSettingsFragmentDirections.kt
M navigation/safe-args-generator/src/tests/test-data/expected/nav_generator_test/kotlin/nested_overridden_action/SettingsFragmentDirections.kt
M navigation/safe-args-generator/src/tests/test-data/nested_login_test.xml
M navigation/safe-args-gradle-plugin/src/test/test-data/app-project-kotlin/src/main/res/navigation/nav_nested_test.xml
A navigation/safe-args-gradle-plugin/src/test/test-data/app-project/src/main/res/navigation/nav_nested_test.xml

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

https://goto.google.com/android-sha1/37ed64d4de87bd4a4365f5c0dfee7b33c8043013

rk...@google.com <rk...@google.com> #4Jul 2, 2024 08:08PM

Hi, I'm still having the issue with 1.0.0-alpha11. Generated classes for fragments in nested navigation throws error for global actions.

rk...@google.com <rk...@google.com> Sep 14, 2024 04:59PM

Marked as fixed.

Issue 348598513