Add initial infrastructure for FragmentStrictMode #123

simonschiller · 2021-01-24T20:42:54Z

Proposed Changes

Add initial infrastructure for FragmentStrictMode

Found this in the issue tracker and think it's a great idea. I tried to keep the first version as lean and as closely aligned to StrictMode as possible. I also plan on creating some follow-up PRs to add a few checks (see issue tracker), as soon as the infrastructure is discussed and somewhat agreed upon. For now the classes are restricted to the library, until we have the first few checks.

Testing

Test: See FragmentStrictModeTest

Issues Fixed

Fixes: 153737341

simonschiller · 2021-01-24T20:43:36Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+ private static final String TAG = "FragmentStrictMode";
+ private static Policy defaultPolicy = Policy.LAX;
+
+ private enum Flag {


If we're concerned about performance, this could also be a bitmask instead

No, the Flags are find and make this clear.

simonschiller · 2021-01-24T20:46:04Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+ }
+ return defaultPolicy;
+ }
+


Sample hook for violations could look like this:

public static void onDemoViolation(@NonNull Fragment fragment) { Policy policy = getNearestPolicy(fragment); if (policy.flags.contains(Flag.DETECT_DEMO)) { onPolicyViolation(policy, new DemoViolation()); } }

This would be called from wherever the violation is triggered.

simonschiller · 2021-01-24T21:05:07Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+
+ /** Call #{@link OnViolationListener#onViolation} for every violation. */
+ @NonNull
+ public Builder penaltyListener(@NonNull OnViolationListener listener) {


StrictMode also requires an Executor here on which the listener is called. Could possibly also make sense here 🤔

If we can skip it here we should. Ideally this is as simple as possible.

I think there's some interesting implications around this, particularly when we want all listeners to be triggered prior to processing the penaltyDeath that might inform the structure we want to provide here, even if we don't expose this directly.

For example, do we really want a direct executor to be the default? I could imagine that using the FragmentHostCallback's Handler (which, for FragmentActivity is the main thread) would be another valid default that would align with most/all of the actual cases we have in mind.

Do you think this work is feasible to do (without a massive rewrite) as a follow up CL? If so, I am okay with just making it more explicit in the Javadoc here with what thread this is being called on (right now, it seems like 'the thread that triggered the policy violation').

One other case I'd like for us to keep in mind is what coroutine friendly API could we build. I don't expect that as part of this initial CL (it is big enough as is), but I'd hate to close that avenue off entirely.

Moving it to the Handler of the FragmentHostCallback sounds like a good idea to me. The only thing that might cause problems is the guarantee that the penaltyDeath handling will happen after all listeners, so we might need to move this onto the Handler as well.

Anyways, should be doable without a big rewrite. For now I added a comment and I'll work on a follow-up PR for this once it is landed.

Yeah, I expect that we should do a check similar to what ensureExecReady() does to either 1) synchronously trigger listeners / penalty death if we're already on the right looper or 2) post to the Handler with two messages: one for dispatching to listeners, the second to trigger the penalty death.

SGTM to do this in a follow up CL, preferably attached to the same underlying issue.

jbw0033 · 2021-01-28T21:46:35Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+
+ if (policy.flags.contains(Flag.PENALTY_DEATH)) {
+ Log.e(TAG, "FragmentStrictMode policy violation with PENALTY_DEATH - shutting down.");
+ Process.killProcess(Process.myPid());


Killing the process might be extreme here, I think we should throw the Violation instead.

I got this from StrictMode, that's how it reacts to penaltyDeath violations: https://cs.android.com/android/platform/superproject/ /master:frameworks/base/core/java/android/os/StrictMode.java;l=2308-2312?q=strictmode

My guess as to why it isn't an exception would be to bypass any Thread.UncaughtExceptionHandler that might be registered. But I'd also be fine with just throwing here.

Process.killProcess means that any crash analytics won't pick up these exceptions, which I think is a big downside. Let's just throw.

jbw0033 · 2021-01-28T21:47:18Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+ private static final String TAG = "FragmentStrictMode";
+ private static Policy defaultPolicy = Policy.LAX;
+
+ private enum Flag {


No, the Flags are find and make this clear.

jbw0033 · 2021-01-28T21:47:20Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+@RestrictTo(RestrictTo.Scope.LIBRARY) // TODO: Make API public as soon as we have a few checks
+public final class FragmentStrictMode {
+ private static final String TAG = "FragmentStrictMode";
+ private static Policy defaultPolicy = Policy.LAX;


Let's do Policy.NONE instead here

Did this for the sake of consistency to StrictMode (see here), but also fine with changing it if you prefer.

jbw0033 · 2021-01-28T21:49:11Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+
+ /** Call #{@link OnViolationListener#onViolation} for every violation. */
+ @NonNull
+ public Builder penaltyListener(@NonNull OnViolationListener listener) {


If we can skip it here we should. Ideally this is as simple as possible.

ianhanniballake · 2021-02-02T00:38:12Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+ }
+
+ /**
+ * {@link FragmentStrictMode} policy applied to a certain thread.


'to a certain FragmentManager', not to a certain thread.

ianhanniballake · 2021-02-02T00:41:36Z

...ent/fragment/src/androidTest/java/androidx/fragment/app/strictmode/FragmentStrictModeTest.kt

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


Use StrictFragment() rather than just Fragment() here and elsewhere as that gives us some extra debugging information for tests.

ianhanniballake · 2021-02-02T00:44:32Z

fragment/fragment/src/main/java/androidx/fragment/app/FragmentManager.java

+
+ /**
+ * Sets the policy for what actions should be detected, as well as the penalty if such actions
+ * occur. This policy is specific to this FragmentManager and all children of it. Pass null to


Let's use the same wording as setFragmentFactory():

The {@link Fragment#getChildFragmentManager() child FragmentManager} of all Fragments in this FragmentManager will also use this policy if one is not explicitly set.

ianhanniballake · 2021-02-02T01:07:32Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+
+ /** Call #{@link OnViolationListener#onViolation} for every violation. */
+ @NonNull
+ public Builder penaltyListener(@NonNull OnViolationListener listener) {


I think there's some interesting implications around this, particularly when we want all listeners to be triggered prior to processing the penaltyDeath that might inform the structure we want to provide here, even if we don't expose this directly.

For example, do we really want a direct executor to be the default? I could imagine that using the FragmentHostCallback's Handler (which, for FragmentActivity is the main thread) would be another valid default that would align with most/all of the actual cases we have in mind.

Do you think this work is feasible to do (without a massive rewrite) as a follow up CL? If so, I am okay with just making it more explicit in the Javadoc here with what thread this is being called on (right now, it seems like 'the thread that triggered the policy violation').

One other case I'd like for us to keep in mind is what coroutine friendly API could we build. I don't expect that as part of this initial CL (it is big enough as is), but I'd hate to close that avenue off entirely.

ianhanniballake · 2021-02-02T01:17:11Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+ }
+
+ @VisibleForTesting(otherwise = VisibleForTesting.NONE)
+ static void onPolicyViolation(@NonNull Fragment fragment, @NonNull Violation violation) {


I'm not particularly enthusiastic about this static call from anywhere as I think it precludes the host Handler / Executor discussion above.

Depends on what we should run on the Handler/Executor I think. We could either

just trigger the listeners or

do the whole processing (logging, checking policy, listeners, ...)

on the Executor. In either case, I think we can easily switch get the right Handler, since we get the violating Fragment as parameter. Another option I can think of would be to make FragmentStrictMode a singleton and keep a reference to it in each Fragment, but I'm not sure if that's actually better.

I'd be okay if the follow up CL we talked about had another version of this that took in the Handler did the synchronous / posted dispatch I mentioned in my other comment. That would give the right behavior without necessarily precluding this static method style.

ianhanniballake · 2021-02-02T01:18:54Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+
+ private static void onPolicyViolation(@NonNull Policy policy, @NonNull Violation violation) {
+ if (policy.flags.contains(Flag.PENALTY_LOG)) {
+ Log.d(TAG, "FragmentStrictMode policy violation: ", violation);


FragmentStrictMode is already the tag, you can remove it from the string itself (and the trailing : ) - just "Policy Violation". I think it would be very helpful to print out the causing Fragment with the log message though so that developers can tie this log message back to a particular fragment.

ianhanniballake · 2021-02-02T01:28:46Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+
+ if (policy.flags.contains(Flag.PENALTY_DEATH)) {
+ Log.e(TAG, "FragmentStrictMode policy violation with PENALTY_DEATH - shutting down.");
+ Process.killProcess(Process.myPid());


Process.killProcess means that any crash analytics won't pick up these exceptions, which I think is a big downside. Let's just throw.

ianhanniballake · 2021-02-05T00:57:34Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+
+ /** Call #{@link OnViolationListener#onViolation} for every violation. */
+ @NonNull
+ public Builder penaltyListener(@NonNull OnViolationListener listener) {


Yeah, I expect that we should do a check similar to what ensureExecReady() does to either 1) synchronously trigger listeners / penalty death if we're already on the right looper or 2) post to the Handler with two messages: one for dispatching to listeners, the second to trigger the penalty death.

SGTM to do this in a follow up CL, preferably attached to the same underlying issue.

ianhanniballake · 2021-02-05T00:59:56Z

fragment/fragment/src/main/java/androidx/fragment/app/strictmode/FragmentStrictMode.java

+ }
+
+ @VisibleForTesting(otherwise = VisibleForTesting.NONE)
+ static void onPolicyViolation(@NonNull Fragment fragment, @NonNull Violation violation) {


I'd be okay if the follow up CL we talked about had another version of this that took in the Handler did the synchronous / posted dispatch I mentioned in my other comment. That would give the right behavior without necessarily precluding this static method style.

dlam · 2021-02-06T06:21:21Z

Looks like a couple lint errors got thrown, but somehow weren't caught by gh workflows - I think the workflow got confused about which files changed exactly and may have completely skipped them :|

$SUPPORT/fragment/fragment/src/androidTest/java/androidx/fragment/app/strictmode/FragmentStrictModeTest.kt:50:45: Missing newline after "("
$SUPPORT/fragment/fragment/src/androidTest/java/androidx/fragment/app/strictmode/FragmentStrictModeTest.kt:52:20: Missing newline before ")"

Could you fix these lint warnings?

You can check using ./gradlew ktlint from the fragment folder.

simonschiller · 2021-02-06T12:26:58Z

Sorry about that, should be fixed now. I actually got a mail notification about the build failure, but the UI showed all green. Weird that this didn't show up in the PR overview 🤔

ianhanniballake · 2021-02-07T06:57:38Z

...ent/fragment/src/androidTest/java/androidx/fragment/app/strictmode/FragmentStrictModeTest.kt

 .penaltyDeath()
- .build())
+ .build()
+ FragmentStrictMode.setDefaultPolicy(policy)


What Lint actually wanted you to do was:

FragmentStrictMode.setDefaultPolicy( FragmentStrictMode.Policy.Builder() .penaltyDeath() .build() )

Sure, I just thought this would be more readable. You want me to change it?

We actually have a codestyles.xml that enforces the style ian mentioned just to have some consistency, does that not automatically configure for you when starting studio via ./gradlew studio?

I used ./gradlew studio, but could be that I just didn't auto-format before committing.

simonschiller · 2021-02-16T14:22:47Z

Is there still something I should do before this can be merged?

dlam · 2021-02-16T18:33:18Z

Interestingly, Copybara failed to mirror your latest commit due to a flake. Let me manually re-trigger.

jbw0033 · 2021-02-18T18:26:48Z

Adding the new files means we also needed to update our jetifier files. Added that change to be submitted along with this one. (https://android-review.googlesource.com/c/platform/frameworks/support/ /1592897) Should be able to get this in.

## Proposed Changes - Don't call listeners on the violating thread anymore, instead use the main thread of the host. - Follow-up to #123, see [here](#123 (comment)) and [here](#123 (comment)) ## Testing Test: `FragmentStrictModeTest#listenerCalledOnCorrectThread` ## Issues Fixed Fixes: 153737341 This is an imported pull request from #131. Resolves #131 Github-Pr-Head-Sha: 4c0f698 GitOrigin-RevId: 77da4ec Change-Id: Ica37a16e5f258b765c105224cf0dcab61b29c52e

Add FragmentStrictMode infrastructure

83ba730

googlebot added the cla: yes label Jan 24, 2021

simonschiller commented Jan 24, 2021

View reviewed changes

simonschiller added 2 commits January 25, 2021 07:49

Fix API issues

34daacc

Regenerate API description

813359d

dlam requested a review from jbw0033 January 25, 2021 16:41

jbw0033 requested changes Jan 28, 2021

View reviewed changes

jbw0033 requested a review from ianhanniballake January 28, 2021 21:49

ianhanniballake requested changes Feb 2, 2021

View reviewed changes

simonschiller added 6 commits February 2, 2021 07:42

Fix errors in comments

6b08f81

Throw exception on violation, instead of killing process

717264a

Use StrictFragment in tests

0410dfc

Log name of violating fragment

959a20c

Regenerate API

ecf27e1

Clarify on which thread FragmentStrictMode listeners will be called

f9ae6e8

simonschiller requested a review from ianhanniballake February 2, 2021 10:52

ianhanniballake approved these changes Feb 5, 2021

View reviewed changes

Fix Ktlint issues

e79ad8e

simonschiller requested a review from jbw0033 February 6, 2021 12:27

ianhanniballake reviewed Feb 7, 2021

View reviewed changes

jbw0033 approved these changes Feb 8, 2021

View reviewed changes

copybara-service bot closed this in e154ee0 Feb 18, 2021

simonschiller deleted the feature/fragment-strict-mode-infra branch February 19, 2021 08:49

simonschiller mentioned this pull request Feb 19, 2021

Call FragmentStrictMode listeners on main thread of host #131

Closed

Add initial infrastructure for FragmentStrictMode #123

Add initial infrastructure for FragmentStrictMode #123

Conversation

simonschiller commented Jan 24, 2021

Proposed Changes

Testing

Issues Fixed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonschiller Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlam commented Feb 6, 2021

simonschiller commented Feb 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonschiller commented Feb 16, 2021

dlam commented Feb 16, 2021

jbw0033 commented Feb 18, 2021

simonschiller Feb 2, 2021 •

edited

Loading