IssueTracker

Trying to reproduce this on my 4.2.2 (v17) Nexus 4. Added this drawable:

<transition xmlns:android="http://schemas.android.com/apk/res/android" >
    <item android:drawable="@drawable/test_drawable_blue"/>
    <item android:drawable="@drawable/test_drawable_green"/>
</transition>

where blue/green drawables look like this:

<shape
    xmlns:android="http://schemas.android.com/apk/res/android"
    android:shape="rectangle">
    <size
        android:width="@dimen/drawable_large_size"
        android:height="@dimen/drawable_small_size" />
    <solid
        android:color="@color/test_blue" />
</shape>

Then added this test:

    @Test
    public void testMutateTransitionDrawable() {
        Drawable drawable = ResourcesCompat.getDrawable(mResources,
                R.drawable.test_transition_drawable, null);
        assertTrue(drawable instanceof TransitionDrawable);

        Drawable mutated = drawable.mutate();
        assertTrue(drawable instanceof TransitionDrawable);
        assertTrue(mutated instanceof TransitionDrawable);
    }

It passes on the device. Going to also try on other earlier devices a bit later in the day once they are charged.

Also not reproducible on Galaxy Nexus running 4.0.1 (v14), 4.0.4 (v15) and 4.2.2 (v17)

https://android-review.googlesource.com/c/platform/frameworks/support/+/960032 with the test. Closing as not reproducible. If this is still happening for you, we'd need to see either a test (same as in this CL) or a small, standalone sample project that reproduces this issue on v17 (or other) devices.

Puzzling, indeed.
What release(s) are you seeing this on? There are different code paths pre- and post-API 24, would be good to know which one you are seeing it on.

ok, thanks, looking into it (again!) now

I can't repro the issue yet. If I remove my synchronization code, I can reproduce it immediately (basically, calling isTrackingEnabled while JankStats is issuing frame data on another thread causes the problem). But synchronizing those code blocks on the window object handles that problem. I can't see what else might be happening that would trigger the bug. If anyone has more information on how your app interacts with JankStats (creation, multiple instances, enabling/disabling... anything), it would help.

Hmm, I was just able to force it to happen by doing two things:
- I sent the data directly on the internal thread that receives FrameMetrics info (instead of sending it through the executor, which is the case in alpha02)
- In the OnFrameListener code, I changed the value of isTrackingEnabled

This combination makes it possible to change the delegates list (as a result of isTrackingEnabled) while the list is being iterated (in the FrmaeMetrics listener, which is sending out the frame data to the listeners). Because they are happening on the same thread, the synchronization block doesn't prevent both of these from happening concurrently; it's just a nested synchronization on the same item in the same thread.

This case is pretty interesting because an upcoming change will remove the Executor from the JankStats constructor, moving to a model where data is sent directly on the internal thread, thus skipping the allocations that currently take place.
But this means anything that happens on that thread to change the delegators list (as above) will trip over this problem.
So I'll work on this. But in the meantime:
Does the above match what your app is doing?
For one thing, are you changing isTrackingEnabled in onFrameListener callback?
And what are you using for an Executor? Your version of the library still uses an Executor, but maybe you are setting it up in an unexpected way.

okay, more progress. There are simple ways to create an Executor that runs on the calling thread (I assume that's what you are doing in your app?).
Then it's just a matter of modifying the list of listeners in the listener callback. (again, I assume that is what you are doing?)

I am working on a fix (not sure yet what form it will take).
In the meantime, if you want to fix your app before the fix is available, avoid calling JankStats.isTrackingEnabled in your listener; that's the root of the problem, because it is modifying the listener list while that list is being processed.
(If any of my assumptions are not correct, please give me more details about what your app is actually doing; this is the only thing I can come up with so far which would cause this exception)

Thanks for the information. Can you also tell me how you create the Executor required to create the JankStats instance?

I'm not able to reproduce the error as described in #12. That approach to instantiation and logging is essentially what is done in the JankLoggingActivity test app in the metrics-integration-tests module. The lifecycle callbacks are slightly different there, but I changed the code to use LifecycleCallbacks (onActivityStarted/Paused/Resumed) instead and still don't see the issue. I also tried using an in-thread Executor (as mentioned in my comments above) and still didn't get the problem to occur.

I need to fix the other issue I found anyway, since changing tracking in a listener is definitely broken, but I can't tell what else could be happening besides that that is causing the problem with your app.

A couple of things that would help here:
- What kind of executor are you creating?
- What release(s) are you seeing the exception on?
- Do you see the exception every time you run the app? Is there anything in particular that triggers it (like going to the background or coming back to the foreground)?

Project: platform/frameworks/support
Branch: androidx-main

commit f6e347fa7bbb4ea5a688d3a0af192b2595db2fae
Author: Chet Haase <chet@google.com>
Date:   Fri Jul 01 10:11:09 2022

    Fix ConcurrentModificationException bug
   
    It is possible to add/remove items from the delegator list (in one code
    path) while iterating through those items (in an unrelated code path). This
    can lead to crashes.
   
    The fix was to send all delegator lists through a single, common object.
    Iteration and modifications only happen in a synchronized block on that object.
    To prevent reentrant issues (where a callback on the thread doing the
    iteration can add/remove items and thus succeed in both synchronized blocks),
    adds/removed are gated to avoid having them happen during iteration, postponing
    those operations until the outer iteration is complete.
   
    Bug: 236612357
    Test: Added test to JankStatsTest to verify bug and fix
    RelNote: "Fixed ConcurrentModificationException bug where the list of
    listener delegates was being modified while it was also being iterated through
    to send per-frame data."
   
    Change-Id: Ib769386f18e51dc6b58c935b42c5b8566c644abc

M       metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi24Impl.kt
M       metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi16Impl.kt
M       metrics/metrics-performance/src/androidTest/java/androidx/metrics/performance/test/JankStatsTest.kt

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

I'm still unable to reproduce the issue as described in the above comments (eg, #16). The fix submitted in #18 addresses another issue that is possible to run into, and it's possible that that fix solidified the concurrency logic in general (it definitely simplified it and resolved the nested issue described in #10). You could try running with the latest snapshot which includes this fix, or wait for alpha03 (not yet scheduled, so at least 3 weeks out).

If you are still seeing the issue with the above change, please reopen. Anything you can tell me about the situation would help; I need to reproduce the problem to make any progress on it. (including, for example, the Android release(s) you are seeing the issue on; that's what I was asking for in #15. But mostly I need steps to reproduce).