(1.2.0-rc03) Fragment destruction will call onViewCreated on child Fragment in specific situation [146290333]

Fixed

Bug

Status Update

No update yet.

Description

mr...@gmail.com

created issue #1

Dec 16, 2019 05:37AM

Component used: Fragment
Version used: 1.2.0-rc03
Devices/Android versions reproduced on:
- Pixel 3 API 27 Emulator
- Pixel 3 API 29 Emulator
- LG G7 ThinQ API 28

Sample code:

https://github.com/eneim/Fragment-RC3-Bug

How to use:
- Make sure Fragment F1 is using layout fragment_f1_bad. Use the code to start the MainActivity, produce a configuration change (eg: rotate the device) --> it will crash due to an Exception is thrown in a check code.
- Changing F1 layout to fragment_f1_good and it will be fine.

Description:

- Saying I have a Fragment F1 in my Activity, defined either by xml (fragment tag or FragmentContainerView) or manual adding at runtime.
- F1's layout is defined in an xml with a 'fragment' tag whose name is a Fragment F2.
- The app starts normally as usual.
- If a configuration change happens, the destruction of F1 will call F2's onViewCreated.

In the sample app, I put a check code which checks if the Activity is destroyed or not, and in this case it will crash the app.

The expected behavior should be: the destruction of F1 should not call F2's onViewCreated.

This issue doesn't occur on 1.2.0-rc03 IF I use `FragmentContainerView` instead of `fragment` to add F2.
This issue cannot be reproduced on 1.2.0-rc02, 1.2.0-rc01 and (I believe) earlier versions.

Please find a call stack in attached screenshot (taken from a debug session on Pixel 3 API 29 Emulator).

Screen Shot 2019-12-16 at 14.33.43.png

253 KB

View

Download

Comments

be...@citymapper.com <be...@citymapper.com> #2Dec 17, 2019 05:56PM

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

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

il...@google.com <il...@google.com> #3Dec 17, 2019 09:41PM

Seems like this is already fixed in master as a side effect of https://issuetracker.google.com/issues/145769287. Can you confirm this is fixed for you if you use the latest snapshot?

You'd want to add the snapshot repository to your repositories block:

maven {
  url "https://ci.android/com/builds/submitted/6080026/androidx_snapshot/latest/repository/"
}

then switch to androidx.fragment:fragment-ktx:1.3.0-SNAPSHOT.

We'll look into cherry picking the changes into Fragment 1.2.0

be...@citymapper.com <be...@citymapper.com> #4Dec 18, 2019 09:44AM

I can confirm that the issue appears to be fixed in that snapshot. Thanks!

il...@google.com <il...@google.com> #5Dec 18, 2019 08:41PM

Marked as fixed, reassigned to il...@google.com.

Great, thanks for confirming.

While there isn't any specific changes needed for this issue (as the issue was already fixed before this bug was filed), I do want to explain what exactly is going to happen from a release process:

First, we'll be cherry picking in the changes needed into a Fragment 1.2.0-rc05 release (it was too late to hit rc04, which already went out earlier today). Namely, these two changes:

https://android-review.googlesource.com/1178750/
https://android-review.googlesource.com/1183079/ (this was the fix for the bug mentioned in comment#3, FWIW)

Secondly, we've added additional testing (in an unrelated, but fortuitously timed effort) specifically to cover this case to ensure this doesn't happen again in https://android-review.googlesource.com/1193030

mr...@gmail.com <mr...@gmail.com> #6Dec 20, 2019 01:27AM

I could confirm the behavior on both my device and emulator. Thanks for the update and it is also good to see addition check here.

I'm curious why you need to limit the check to Build.VERSION.SDK_INT >= 17?

il...@google.com <il...@google.com> #7Jan 6, 2020 06:28PM

For the test in https://android-review.googlesource.com/1193030, isDestroyed() only exists on API 17 and higher: https://developer.android.com/reference/android/app/Activity.html#isDestroyed()

Issue 146290333