-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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
Add initial infrastructure for FragmentStrictMode #123
Add initial infrastructure for FragmentStrictMode #123
Conversation
private static final String TAG = "FragmentStrictMode"; | ||
private static Policy defaultPolicy = Policy.LAX; | ||
|
||
private enum Flag { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're concerned about performance, this could also be a bitmask instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the Flags are find and make this clear.
} | ||
return defaultPolicy; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
/** Call #{@link OnViolationListener#onViolation} for every violation. */ | ||
@NonNull | ||
public Builder penaltyListener(@NonNull OnViolationListener listener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StrictMode
also requires an Executor
here on which the listener is called. Could possibly also make sense here 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can skip it here we should. Ideally this is as simple as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
if (policy.flags.contains(Flag.PENALTY_DEATH)) { | ||
Log.e(TAG, "FragmentStrictMode policy violation with PENALTY_DEATH - shutting down."); | ||
Process.killProcess(Process.myPid()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Killing the process might be extreme here, I think we should throw the Violation instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Process.killProcess
means that any crash analytics won't pick up these exceptions, which I think is a big downside. Let's just throw.
private static final String TAG = "FragmentStrictMode"; | ||
private static Policy defaultPolicy = Policy.LAX; | ||
|
||
private enum Flag { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the Flags are find and make this clear.
@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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do Policy.NONE instead here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this for the sake of consistency to StrictMode
(see here), but also fine with changing it if you prefer.
|
||
/** Call #{@link OnViolationListener#onViolation} for every violation. */ | ||
@NonNull | ||
public Builder penaltyListener(@NonNull OnViolationListener listener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can skip it here we should. Ideally this is as simple as possible.
} | ||
|
||
/** | ||
* {@link FragmentStrictMode} policy applied to a certain thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'to a certain FragmentManager', not to a certain thread.
with(ActivityScenario.launch(FragmentTestActivity::class.java)) { | ||
val fragmentManager = withActivity { supportFragmentManager } | ||
|
||
val parentFragment = Fragment() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use StrictFragment()
rather than just Fragment()
here and elsewhere as that gives us some extra debugging information for tests.
|
||
/** | ||
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
/** Call #{@link OnViolationListener#onViolation} for every violation. */ | ||
@NonNull | ||
public Builder penaltyListener(@NonNull OnViolationListener listener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
|
||
@VisibleForTesting(otherwise = VisibleForTesting.NONE) | ||
static void onPolicyViolation(@NonNull Fragment fragment, @NonNull Violation violation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not particularly enthusiastic about this static call from anywhere as I think it precludes the host Handler / Executor discussion above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
private static void onPolicyViolation(@NonNull Policy policy, @NonNull Violation violation) { | ||
if (policy.flags.contains(Flag.PENALTY_LOG)) { | ||
Log.d(TAG, "FragmentStrictMode policy violation: ", violation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
if (policy.flags.contains(Flag.PENALTY_DEATH)) { | ||
Log.e(TAG, "FragmentStrictMode policy violation with PENALTY_DEATH - shutting down."); | ||
Process.killProcess(Process.myPid()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Process.killProcess
means that any crash analytics won't pick up these exceptions, which I think is a big downside. Let's just throw.
|
||
/** Call #{@link OnViolationListener#onViolation} for every violation. */ | ||
@NonNull | ||
public Builder penaltyListener(@NonNull OnViolationListener listener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
|
||
@VisibleForTesting(otherwise = VisibleForTesting.NONE) | ||
static void onPolicyViolation(@NonNull Fragment fragment, @NonNull Violation violation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 :|
Could you fix these lint warnings? You can check using |
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 🤔 |
.penaltyDeath() | ||
.build()) | ||
.build() | ||
FragmentStrictMode.setDefaultPolicy(policy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Lint actually wanted you to do was:
FragmentStrictMode.setDefaultPolicy(
FragmentStrictMode.Policy.Builder()
.penaltyDeath()
.build()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I just thought this would be more readable. You want me to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used ./gradlew studio
, but could be that I just didn't auto-format before committing.
Is there still something I should do before this can be merged? |
Interestingly, Copybara failed to mirror your latest commit due to a flake. Let me manually re-trigger. |
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
Proposed Changes
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