Fixed
Status Update
No update yet.
Comments
ch...@google.com
The issue is reproducible with core-ktx 1.2.0 and 1.3.0-rc01.
zg...@gmail.com
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.
ch...@google.com
Hi Nona,
Can you please schedule a release after you merge the fix?
zg...@gmail.com
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
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
zg...@gmail.com
Any way I can tell what version this will land in?
ch...@google.com
Great—works as expected, thanks!
ch...@google.com
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.
- 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.
ch...@google.com
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)
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)
zg...@gmail.com
Thank you for spending so much time on this issue。Let me talk about an operation path on my side，I create a jankstats in the onactivitystarted method of each activity(mJankStats = JankStats.createAndTrack(activity.window,executor,jankFrameListener)), and then set istrackingenabled = true in the onactivityresumed method; Set istrackingenabled = false in onactivitypaused method; In addition, istrackingenabled has not been operated in other places; And then I was at jankstats Onframelistener collects framedurationuinanos of each frame to record the time consumption of each frame, and then reports about 60 frames once.
ch...@google.com
Thanks for the information. Can you also tell me how you create the Executor required to create the JankStats instance?
ch...@google.com
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.
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.
ch...@google.com
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)?
- 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)?
zg...@gmail.com
- Dispatchers.Default.asExecutor()
- alpha02 version
- It's an occasional crash. I didn't reproduce it, but users of the online version will appear. According to the analysis of the online crash log, it seems that some are in the background and some are in the foreground, which may happen in the front and back switching
zg...@gmail.com
Is there any progress on this issue？
ap...@google.com
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
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
ch...@google.com
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).
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).
Description
`androidx.metrics` version used: 1.0.0-alpha01
Devices/Android versions reproduced on: multiple android versions 11, 10, 12, 9
When you encountered this bug:
• Issue is reported on production. App is crashing with ConcurrentModificationException. We are initialising JankStat on
every fragment to measure the jank frames for that fragment.
This issue is already fixed:
When the new version of JankStats would be released with the fix. For now is there any workaround to suppress this crash?