Skip to content

Call FragmentStrictMode listeners on main thread of host #131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
simonschiller wants to merge 5 commits into from
Closed

Call FragmentStrictMode listeners on main thread of host #131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8d2dbd
Move FragmentStrictMode to common package to access package-private f…
simonschiller Feb 19, 2021
e5f4e2e
Call listeners and throw exceptions on correct thread
simonschiller Feb 19, 2021
dcc4d4d
Update API to reflect package change
simonschiller Feb 19, 2021
9f8aad3
Move FragmentStrictMode back into separate package
simonschiller Feb 23, 2021
4c0f698
Fix annotation ordering
simonschiller Mar 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions .../fragment/src/androidTest/java/androidx/fragment/app/strictmode/FragmentStrictModeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@

package androidx.fragment.app.strictmode

import android.os.Looper
import androidx.fragment.app.StrictFragment
import androidx.fragment.app.executePendingTransactions
import androidx.fragment.app.test.FragmentTestActivity
import androidx.test.core.app.ActivityScenario
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.MediumTest
import androidx.test.platform.app.InstrumentationRegistry
import androidx.testutils.withActivity
import com.google.common.truth.Truth.assertThat
import com.google.common.truth.Truth.assertWithMessage
Expand Down Expand Up @@ -87,15 +89,42 @@ public class FragmentStrictModeTest {

FragmentStrictMode.setDefaultPolicy(policy("Default policy"))
FragmentStrictMode.onPolicyViolation(childFragment, violation)
InstrumentationRegistry.getInstrumentation().waitForIdleSync()
assertThat(lastTriggeredPolicy).isEqualTo("Default policy")

fragmentManager.strictModePolicy = policy("Parent policy")
FragmentStrictMode.onPolicyViolation(childFragment, violation)
InstrumentationRegistry.getInstrumentation().waitForIdleSync()
assertThat(lastTriggeredPolicy).isEqualTo("Parent policy")

parentFragment.childFragmentManager.strictModePolicy = policy("Child policy")
FragmentStrictMode.onPolicyViolation(childFragment, violation)
InstrumentationRegistry.getInstrumentation().waitForIdleSync()
assertThat(lastTriggeredPolicy).isEqualTo("Child policy")
}
}

@Test
public fun listenerCalledOnCorrectThread() {
var thread: Thread? = null

val policy = FragmentStrictMode.Policy.Builder()
.penaltyListener { thread = Thread.currentThread() }
.build()
FragmentStrictMode.setDefaultPolicy(policy)

with(ActivityScenario.launch(FragmentTestActivity::class.java)) {
val fragmentManager = withActivity { supportFragmentManager }

val fragment = StrictFragment()
fragmentManager.beginTransaction()
.add(fragment, null)
.commit()
executePendingTransactions()

FragmentStrictMode.onPolicyViolation(fragment, object : Violation() {})
InstrumentationRegistry.getInstrumentation().waitForIdleSync()
assertThat(thread).isEqualTo(Looper.getMainLooper().thread)
}
}
}
5 changes: 4 additions & 1 deletion fragment/fragment/src/main/java/androidx/fragment/app/FragmentHostCallback.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import androidx.activity.result.contract.ActivityResultContract;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.RestrictTo;
import androidx.core.app.ActivityCompat;
import androidx.core.content.ContextCompat;
import androidx.core.util.Preconditions;
Expand Down Expand Up @@ -247,8 +248,10 @@ Context getContext() {
return mContext;
}

/** @hide */
@RestrictTo(RestrictTo.Scope.LIBRARY)
@NonNull
Handler getHandler() {
public Handler getHandler() {
return mHandler;
}
}
4 changes: 3 additions & 1 deletion fragment/fragment/src/main/java/androidx/fragment/app/FragmentManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2897,8 +2897,10 @@ void restoreSaveState(@Nullable Parcelable state) {
mLaunchedFragments = new ArrayDeque<>(fms.mLaunchedFragments);
}

/** @hide */
@RestrictTo(RestrictTo.Scope.LIBRARY)
Copy link
Member

@ianhanniballake ianhanniballake Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit here and elsewhere: Always add the @RestrictTo annotation as the topmost annotation (i.e., directly below the Javadoc comment where the @hide is so that they are visually next to each other in the code).

Copy link
Contributor Author

@simonschiller simonschiller Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - fixed in 4c0f698

@NonNull
FragmentHostCallback<?> getHost() {
public FragmentHostCallback<?> getHost() {
return mHost;
}

Expand Down
39 changes: 32 additions & 7 deletions fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package androidx.fragment.app.strictmode;

import android.annotation.SuppressLint;
import android.os.Handler;
import android.os.Looper;
import android.util.Log;

import androidx.annotation.NonNull;
Expand Down Expand Up @@ -129,7 +131,7 @@ public Builder penaltyDeath() {

/**
* Call #{@link OnViolationListener#onViolation} for every violation. The listener will
* be called on the thread which caused the violation.
* be called on the main thread of the fragment host.
*/
@NonNull
@SuppressLint("BuilderSetStyle")
Expand Down Expand Up @@ -184,21 +186,44 @@ private static Policy getNearestPolicy(@Nullable Fragment fragment) {
}

@VisibleForTesting
static void onPolicyViolation(@NonNull Fragment fragment, @NonNull Violation violation) {
Policy policy = getNearestPolicy(fragment);
String fragmentName = fragment.getClass().getName();
static void onPolicyViolation(@NonNull Fragment fragment, @NonNull final Violation violation) {
final Policy policy = getNearestPolicy(fragment);
final String fragmentName = fragment.getClass().getName();

if (policy.flags.contains(Flag.PENALTY_LOG)) {
Log.d(TAG, "Policy violation in " + fragmentName, violation);
}

if (policy.listener != null) {
policy.listener.onViolation(violation);
runOnHostThread(fragment, new Runnable() {
@Override
public void run() {
policy.listener.onViolation(violation);
}
});
}

if (policy.flags.contains(Flag.PENALTY_DEATH)) {
Log.e(TAG, "Policy violation with PENALTY_DEATH in " + fragmentName, violation);
throw violation;
runOnHostThread(fragment, new Runnable() {
@Override
public void run() {
Log.e(TAG, "Policy violation with PENALTY_DEATH in " + fragmentName, violation);
throw violation;
}
});
}
}

private static void runOnHostThread(@NonNull Fragment fragment, @NonNull Runnable runnable) {
if (fragment.isAdded()) {
Handler handler = fragment.getParentFragmentManager().getHost().getHandler();
if (handler.getLooper() == Looper.myLooper()) {
runnable.run(); // Already on correct thread -> run synchronously
} else {
handler.post(runnable); // Switch to correct thread
}
} else {
runnable.run(); // Fragment is not attached to any host -> run synchronously
}
}
}