RecyclerView ChildHelper.removeView re-enters removeViewIfHidden, causing crash [292114537]

Verified

Bug

adexe s nau

Status Update

No update yet.

Description

da...@google.com

created issue #1

Jul 20, 2023 06:28PM

I will create a Test to reproduce the bug.

Comments

da...@google.com <da...@google.com> #2Jul 20, 2023 06:29PM

Reassigned to da...@google.com.

Hi Ed, Thank you so much for these suggestions. I've been reviewing them and merging them in. Hopefully it should be live. I've included a thank you note too in the article.

Message last modified on Jul 21, 2023 02:12AM

ap...@google.com <ap...@google.com> #3Jul 21, 2023 09:45PM

Great! Thanks a lot, I'll look for the live updates soon!

da...@google.com <da...@google.com> #4Jul 21, 2023 09:52PM

Submitted a leanback test shows the crash steps. "setSelectedPosition(0)" removes the the item "6" that is currently sliding into the RecyclerView, it causes re-entering ChildHelper.removeViewIfHidden("6") in the middle of ChildHelper.removeView("6"). After that, ChildHelper is in a bad state for the getChildCount(), the following "setSelectedPosition(2)" throws NullPointerException.

https://cs.android.com/androidx/platform/frameworks/support/+/d0866d321132bd36f828e1e39a49ca7dad83fd2c:leanback/leanback/src/androidTest/java/androidx/leanback/widget/GridWidgetTest.java;l=2252

        // showing 0 1(selected) 2 3 4 5(peek)
        setSelectedPosition(1);

        // swap 5 and 6, showing 0 1(selected) 2 3 4 6(slide in) 5(slide out)
        mActivityTestRule.runOnUiThread(() ->
                adapter.setItems(TestPresenter.generateItems(new int[]{0, 1, 2, 3, 4, 6, 5, 7, 8}),
                        TestPresenter.DIFF_CALLBACK));

        // Wait a little bit for the ItemAnimation to be started
        waitForItemAnimationStart();

        // Scroll to 0 to remove "6", this can break ChildHelper and scroll to 2 may failed to find
        // the View and crash.
        setSelectedPosition(0);
        setSelectedPosition(2);

ap...@google.com <ap...@google.com> #5Aug 10, 2023 08:26PM

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

commit 7b1d8693ce84c2e99a0972311c0abe9850315d22
Author: Dake Gu <dake@google.com>
Date: Fri Jul 21 12:02:27 2023

Fix ChildHelper removeView re-enter removeViewIfHidden

If ItemAnimation is running, a layout pass removeAndRecycleView
the animating view, the following may happen:

- Initially the ChildHelper binary bits are 1000, 4th view
is hidden to LM.
- LM tries to remove the 3rd view.
- ChildHelper.removeView modifies the binary bits (
1000 -> 100 since it's going to remove 3rd View)
- ChildHelper.removeView calls Callback.removeViewAt(2)
- Callback.removeViewAt calls dispatchViewDetached
- custom adapter dispatchViewDettached clears ViewPropertyAnimator
- ItemAnimation stops
- ChildHelper.removeViewIfHidden is called
- Everything falls apart since the ChildHelper is already
in removeView and state was changed half way. Since the
bits was changed to 100, ChildHelper incorrectly thinks
the 3rd View is hidden and calls removeViewAt(2) second
time.

It appears TV apps using Presenter are all clearing the
ViewPropertyAnimator during dispatchViewAttached due to it
wants to clear the "transient state" which prevents a view
being recycled in scroll pass. It not only stops
the custom animations, but also stops the ItemAnimation.
See the original CL that added the animate().cancel() call:

https://android.googlesource.com/platform/frameworks/support/+/5358b0ca6ed795892bd097fdf15d41fb6b1a03d2

Interstingly, there was a simlar bug fix and unit test for
removing a "fade-in" view in scroll pass back in 2015:

https://android.googlesource.com/platform/frameworks/support/+/95ff60ba852e21922aa606b5117f6a0c4f4f33d3
In 2015, predictive layout wasn't supported in leanback.
Reproduce step of this bug needs predictive layout (slide
an item into the RV). So no wonder we didn't find this crash
in 2015. This bug was first reported in 2017, after predictive
animation support was added in leanback.

Since we haven't document how the app should behave in
dispatchViewDetached, even we documented, it's very likely
developer may ignore. The better fix will be making the
recyclerview more defensive.
So the CL adds a re-entering check within ChildHelper by
tracking the calling of removeView/removeViewAt/removeViewIfHidden.

Bug: 241014260
Bug: 292114537
Bug: 157105352

Test: Created recyclerview tests based on the fake LayoutManager:
removeSlideInViewLeftToSlideOutViewAndCancelAnimationInOnDetach
removeSlideInViewLeftToSlideOutView

Test: A leanback regression test running on the real layoutmanager
was already merged (but disabled pending on the fix)

https://android-review.googlesource.com/#/q/Icf8fd1373f16d5544bc14db5721a670ba7e5b7ec

Change-Id: I42f22b0e805db12ee4cd478b3beb6b9d1a86cdaa

M recyclerview/recyclerview/src/androidTest/java/androidx/recyclerview/widget/BaseRecyclerViewAnimationsTest.java
M recyclerview/recyclerview/src/androidTest/java/androidx/recyclerview/widget/BaseRecyclerViewInstrumentationTest.java
M recyclerview/recyclerview/src/androidTest/java/androidx/recyclerview/widget/RecyclerViewAnimationsTest.java
M recyclerview/recyclerview/src/main/java/androidx/recyclerview/widget/ChildHelper.java

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

da...@google.com <da...@google.com> #6Aug 10, 2023 08:29PM

Reassigned to ry...@google.com.

Waiting for next recyclerview release.

ry...@google.com <ry...@google.com> #7Aug 10, 2023 09:55PM

Marked as fixed.

Will be released with next alpha release, barring anything unexpected.

pr...@google.com <pr...@google.com> #8Sep 6, 2023 07:55PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.leanback:leanback:1.2.0-alpha03

da...@google.com <da...@google.com> #9Sep 25, 2023 07:37PM

Re #8
No, the fix is in recyclerview, not in leanback. Recyclerview hasn't released a alpha or beta yet with this fix.

da...@google.com <da...@google.com> #10Oct 7, 2023 03:28AM

Recyclerview 1.3.2 release is currently being prepared to include the fix.

pr...@google.com <pr...@google.com> #11Oct 18, 2023 08:42PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.recyclerview:recyclerview:1.4.0-alpha01
androidx.recyclerview:recyclerview:1.3.2

da...@google.com <da...@google.com> Oct 26, 2023 03:02PM

Verified by da...@google.com.

ap...@google.com <ap...@google.com> #12Oct 28, 2023 09:24PM

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

commit f87647a9de4c5b4308b2a6ebe299441f74f8d740
Author: Dake Gu <dake@google.com>
Date: Fri Oct 27 10:18:25 2023

leanback: update recyclerview requirement to 1.3.2

This is to include the important recyclerview fix that is
common in leanback apps.

framework fragment Tests are regenerated.

Bug: 292114537
Test: ./gradlew leanback:leanback:connectedCheck --info --daemon -Pandroid.testInstrumentationRunnerArguments.class=androidx.leanback.widget.GridWidgetTest#testCrashOnRVChildHelperBug292114537
Change-Id: I2c3a0f7ae43f72bd6a1dbbe30c269148f824a885

M leanback/leanback-grid/build.gradle
M leanback/leanback-grid/src/main/java/androidx/leanback/widget/GridLayoutManager.java
M leanback/leanback-preference/build.gradle
M leanback/leanback/build.gradle
M leanback/leanback/src/androidTest/generatev4.py
M leanback/leanback/src/androidTest/java/androidx/leanback/app/BrowseFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/BrowseSupportFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/DetailsFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/DetailsTestFragment.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/GuidedStepFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/GuidedStepTestFragment.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/RowsFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/RowsSupportFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/SearchFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/SearchSupportFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/VerticalGridFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/app/VerticalGridSupportFragmentTest.java
M leanback/leanback/src/androidTest/java/androidx/leanback/widget/GridWidgetTest.java

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