Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

StrictMode.resetPoliciesAfter – strict number of checks, and add performance team as code owners for the file #13959

Closed
sraturi opened this issue Aug 20, 2020 · 5 comments
Closed

StrictMode.resetPoliciesAfter – strict number of checks, and add performance team as code owners for the file #13959

sraturi opened this issue Aug 20, 2020 · 5 comments
Assignees
@mcomella
Labels
needs:triage Issue needs triage performance Possible performance wins

Comments

@sraturi
Copy link
Contributor

sraturi commented Aug 20, 2020 •
edited by data-sync-user
Loading

There are currently two versions

  • resetPoliciesAfter(in fenix)
  • resetAfter (outside of fenix)
  • usages in GV (maybe we shouldn't worry about these right now & file a follow-up)

We want to restrict the number of calls to these methods to prevent regressions.

┆Issue is synchronized with this Jira Task
@sraturi sraturi added the performance Possible performance wins label Aug 20, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Aug 20, 2020
@mcomella mcomella self-assigned this Sep 22, 2020
mcomella added a commit to mcomella/fenix that referenced this issue Sep 22, 2020
@mcomella
For mozilla-mobile#13959: always enable detectNonSdkApiUsage.
e81e564 
We don't use penalty death for the VM policy so we theoretically don't
need to disable this check if penalty death is enabled.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 22, 2020
@mcomella
For mozilla-mobile#13959: remove unused penaltyDialog parameter.
0c28112 
Additional branching introduces complexity so we should avoid it when
possible. This branch was also unused so it's more likely to have bugs
if we tried to use it after some refactor.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 22, 2020
@mcomella
For mozilla-mobile#13959: rename to attachListenerToDisablePenaltyDea…
75493b6 
…th for clarity.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 22, 2020
@mcomella
For mozilla-mobile#13959: use ac StrictMode.resetAfter rather than du…
5363390 
…plicating functionality.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 22, 2020
@mcomella
For mozilla-mobile#13959: deduplicate condition where StrictMode is e…
f77ec9e 
…nabled; fix tests.

I don't like the implicit dependencies from using `object` but making
this a `class`/Component makes it inconvenient to access. If we continue
to add functionality like this, we should consider switching its access
pattern.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 22, 2020
@mcomella
For mozilla-mobile#13959: add comment to explain why we only resetAft…
ad08359 
…er in certain build modes.
@mcomella mcomella mentioned this issue Sep 22, 2020
Merged
3 tasks
mcomella added a commit to mcomella/fenix that referenced this issue Sep 24, 2020
@mcomella
For mozilla-mobile#13959: add comment to explain why we only resetAft…
33df5b5 
…er in certain build modes.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 24, 2020
@mcomella
For mozilla-mobile#13959: comment about duplication in logic in Stric…
0915ad5 
…tMode.

I had to drop a commit that addressed the issue because it was too hard
to fix.
mcomella added a commit that referenced this issue Sep 28, 2020
@mcomella
For #13959: always enable detectNonSdkApiUsage.
f05a542 
We don't use penalty death for the VM policy so we theoretically don't
need to disable this check if penalty death is enabled.
mcomella added a commit that referenced this issue Sep 28, 2020
@mcomella
For #13959: remove unused penaltyDialog parameter.
c03c7ef 
Additional branching introduces complexity so we should avoid it when
possible. This branch was also unused so it's more likely to have bugs
if we tried to use it after some refactor.
mcomella added a commit that referenced this issue Sep 28, 2020
@mcomella
For #13959: rename to attachListenerToDisablePenaltyDeath for clarity.
2c1befa
mcomella added a commit that referenced this issue Sep 28, 2020
@mcomella
For #13959: use ac StrictMode.resetAfter rather than duplicating func…
3bf71ef 
…tionality.
mcomella added a commit that referenced this issue Sep 28, 2020
@mcomella
For #13959: add comment to explain why we only resetAfter in certain …
e1bd619 
…build modes.
mcomella added a commit that referenced this issue Sep 28, 2020
@mcomella
For #13959: comment about duplication in logic in StrictMode.
a92356f 
I had to drop a commit that addressed the issue because it was too hard
to fix.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 28, 2020
@mcomella
For mozilla-mobile#13959: change StrictModeManager to class from object.
cd290db 
I originally tried to create this PR leaving this as an object to keep
the change simple but it wasn't worth it - once the object started to
keep state, we'd need to manually reset the state between runs. Also,
the tests were already getting hacky with static mocking so it was
easier to address some of those issues this way too.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 28, 2020
@mcomella
For mozilla-mobile#13959: move resetAfter into StrictModeManager.
9213c40 
In a followup PR, we need to add state to strictModeManager (the
number of suppressions). This is much simpler to do when this is defined
as a class rather than an object. However, when this is defined as a
class, `resetAfter` needs access to the strictModeManager. Instead of
passing it in as an argument, it made sense to move this function onto
the strictModeManager instead.

Since folks are used to calling:
```
StrictMode.ThreadPolicy.allowThreadDiskReads().resetAfter
```

We're going to have to add a lint check to prevent them from doing that.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 28, 2020
@mcomella
For mozilla-mobile#13959: clean up existing StrictMode test names.
6c353ff
mcomella added a commit to mcomella/fenix that referenced this issue Sep 28, 2020
@mcomella
For mozilla-mobile#13959: remove resetAfter & port tests to StrictMod…
b2590de 
…eManager.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 28, 2020
@mcomella
For mozilla-mobile#13959: fix startup crash by using arg Context.
0922f57 
The `context` member function returns null in attachBaseContext so we
need to use the Context that's being attached instead.
mcomella added a commit that referenced this issue Sep 29, 2020
@mcomella
For #13959: change StrictModeManager to class from object.
6abeb2d 
I originally tried to create this PR leaving this as an object to keep
the change simple but it wasn't worth it - once the object started to
keep state, we'd need to manually reset the state between runs. Also,
the tests were already getting hacky with static mocking so it was
easier to address some of those issues this way too.
mcomella added a commit that referenced this issue Sep 29, 2020
@mcomella
For #13959: move resetAfter into StrictModeManager.
f19c992 
In a followup PR, we need to add state to strictModeManager (the
number of suppressions). This is much simpler to do when this is defined
as a class rather than an object. However, when this is defined as a
class, `resetAfter` needs access to the strictModeManager. Instead of
passing it in as an argument, it made sense to move this function onto
the strictModeManager instead.

Since folks are used to calling:
```
StrictMode.ThreadPolicy.allowThreadDiskReads().resetAfter
```

We're going to have to add a lint check to prevent them from doing that.
mcomella added a commit that referenced this issue Sep 29, 2020
@mcomella
For #13959: clean up existing StrictMode test names.
dd73cb6
mcomella added a commit that referenced this issue Sep 29, 2020
@mcomella
For #13959: remove resetAfter & port tests to StrictModeManager.
42cca07
mcomella added a commit to mcomella/fenix that referenced this issue Sep 30, 2020
@mcomella
For mozilla-mobile#13959: add HomeActivityTestRule.skipOnboarding.
ab7cb50 
This is more correct, faster, and results in less copy-paste duplication
than the current behavior:
  homeScreen { }.dismissOnboarding()

Which opens settings to dismiss onboarding.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 30, 2020
@mcomella
For mozilla-mobile#13959: add test to prevent increasing StrictMode s…
af10b02 
…uppression count.
mcomella added a commit to mcomella/fenix that referenced this issue Sep 30, 2020
@mcomella
For mozilla-mobile#13959: improve custom detekt violation output.
0473a2d 
Before it used to output the violations all one one line. Now it looks
like:
```
MozillaStrictModeSuppression:
    'import mozilla.components.support.ktx.android.os.resetAfter' at
(17,1) in /StrictModeManager.kt
    Please use `components.strictMode.resetAfter` instead because it has
performance improvements and additional code to monitor for performance
regressions.

MozillaStrictModeSuppression:
    'setThreadPolicy(threadPolicy.build())' at (56,24) in
/StrictModeManager.kt
    Please use `components.strictMode.resetAfter` instead because it has
performance improvements and additional code to monitor for performance
regressions.

MozillaStrictModeSuppression:
    'setVmPolicy(builder.build())' at (71,24) in /StrictModeManager.kt
    NOT YET IMPLEMENTED: please consult the perf team about
implementing`StrictModeManager.resetAfter`: we want to understand the
performance implications of suppressing setVmPolicy before allowing it.
```
mcomella added a commit to mcomella/fenix that referenced this issue Sep 30, 2020
@mcomella
For mozilla-mobile#13959: add MozillaStrictModeSuppression detekt check.
5c438c9
mcomella added a commit to mcomella/fenix that referenced this issue Sep 30, 2020
@mcomella
For mozilla-mobile#13959: add marker when StrictMode is suppressed.
ecad78c
@mcomella mcomella mentioned this issue Oct 5, 2020
Closed
mcomella added a commit to mcomella/fenix that referenced this issue Oct 6, 2020
@mcomella
For mozilla-mobile#13959: count StrictMode suppressions on startup.
64a07fa
mcomella added a commit to mcomella/fenix that referenced this issue Oct 6, 2020
@mcomella
For mozilla-mobile#13959: add HomeActivityTestRule.skipOnboarding.
caf164b 
This is more correct, faster, and results in less copy-paste duplication
than the current behavior:
  homeScreen { }.dismissOnboarding()

Which opens settings to dismiss onboarding.
mcomella added a commit to mcomella/fenix that referenced this issue Oct 6, 2020
@mcomella
For mozilla-mobile#13959: add test to prevent increasing StrictMode s…
697f191 
…uppression count.
mcomella added a commit to mcomella/fenix that referenced this issue Oct 6, 2020
@mcomella
For mozilla-mobile#13959: improve custom detekt violation output.
3a7f633 
Before it used to output the violations all one one line. Now it looks
like:
```
MozillaStrictModeSuppression:
    'import mozilla.components.support.ktx.android.os.resetAfter' at
(17,1) in /StrictModeManager.kt
    Please use `components.strictMode.resetAfter` instead because it has
performance improvements and additional code to monitor for performance
regressions.

MozillaStrictModeSuppression:
    'setThreadPolicy(threadPolicy.build())' at (56,24) in
/StrictModeManager.kt
    Please use `components.strictMode.resetAfter` instead because it has
performance improvements and additional code to monitor for performance
regressions.

MozillaStrictModeSuppression:
    'setVmPolicy(builder.build())' at (71,24) in /StrictModeManager.kt
    NOT YET IMPLEMENTED: please consult the perf team about
implementing`StrictModeManager.resetAfter`: we want to understand the
performance implications of suppressing setVmPolicy before allowing it.
```
mcomella added a commit to mcomella/fenix that referenced this issue Oct 6, 2020
@mcomella
For mozilla-mobile#13959: add MozillaStrictModeSuppression detekt check.
0439c65
mcomella added a commit to mcomella/fenix that referenced this issue Oct 6, 2020
@mcomella
For mozilla-mobile#13959: add marker when StrictMode is suppressed.
0aa1cde
mcomella added a commit to mcomella/fenix that referenced this issue Oct 6, 2020
@mcomella
For mozilla-mobile#13959 - review: add clarifying comments.
beaf656
mcomella added a commit to mcomella/fenix that referenced this issue Oct 6, 2020
@mcomella
For mozilla-mobile#13959 - post: ignore new StrictMode rule in tests.
0323e70
mcomella added a commit to mcomella/fenix that referenced this issue Oct 6, 2020
@mcomella
For mozilla-mobile#13959 - post: decrement expected suppression count.
e2d4c05 
Running locally, I get the same error: I think that there legitimately
was an reduction in the number of StrictMode suppressions on start up.
mcomella added a commit that referenced this issue Oct 6, 2020
@mcomella
For #13959: count StrictMode suppressions on startup.
4a4edeb
mcomella added a commit that referenced this issue Oct 6, 2020
@mcomella
For #13959: add HomeActivityTestRule.skipOnboarding.
c33aafb 
This is more correct, faster, and results in less copy-paste duplication
than the current behavior:
  homeScreen { }.dismissOnboarding()

Which opens settings to dismiss onboarding.
mcomella added a commit that referenced this issue Oct 6, 2020
@mcomella
For #13959: add test to prevent increasing StrictMode suppression count.
b222376
mcomella added a commit that referenced this issue Oct 6, 2020
@mcomella
For #13959: improve custom detekt violation output.
66f220c 
Before it used to output the violations all one one line. Now it looks
like:
```
MozillaStrictModeSuppression:
    'import mozilla.components.support.ktx.android.os.resetAfter' at
(17,1) in /StrictModeManager.kt
    Please use `components.strictMode.resetAfter` instead because it has
performance improvements and additional code to monitor for performance
regressions.

MozillaStrictModeSuppression:
    'setThreadPolicy(threadPolicy.build())' at (56,24) in
/StrictModeManager.kt
    Please use `components.strictMode.resetAfter` instead because it has
performance improvements and additional code to monitor for performance
regressions.

MozillaStrictModeSuppression:
    'setVmPolicy(builder.build())' at (71,24) in /StrictModeManager.kt
    NOT YET IMPLEMENTED: please consult the perf team about
implementing`StrictModeManager.resetAfter`: we want to understand the
performance implications of suppressing setVmPolicy before allowing it.
```
mcomella added a commit that referenced this issue Oct 6, 2020
@mcomella
For #13959: add MozillaStrictModeSuppression detekt check.
3b5d6d5
mcomella added a commit that referenced this issue Oct 6, 2020
@mcomella
For #13959: add marker when StrictMode is suppressed.
50959d9
mcomella added a commit that referenced this issue Oct 6, 2020
@mcomella
For #13959 - review: add clarifying comments.
3e96f6b
mcomella added a commit that referenced this issue Oct 6, 2020
@mcomella
For #13959 - post: ignore new StrictMode rule in tests.
d2467f7
mcomella added a commit that referenced this issue Oct 6, 2020
@mcomella
For #13959 - post: decrement expected suppression count.
4302cd2 
Running locally, I get the same error: I think that there legitimately
was an reduction in the number of StrictMode suppressions on start up.
@mcomella
Copy link
Contributor

mcomella commented Oct 6, 2020

Addressed in #15570

@mcomella mcomella closed this as completed Oct 6, 2020
@Mugurell
Copy link
Contributor

Mugurell commented Oct 7, 2020

I think this causes the app to crash when changing orientation with the following log:

org.mozilla.fenix.debug E/ExceptionHandler: Uncaught exception handled:
java.lang.IllegalStateException: Failed to initialize GeckoRuntime
at org.mozilla.geckoview.GeckoRuntime.create(GeckoRuntime.java:490)
at GeckoProvider.createRuntime(GeckoProvider.kt:56)
at GeckoProvider.getOrCreateRuntime(GeckoProvider.kt:28)
at org.mozilla.fenix.components.Core$engine$2.invoke(Core.kt:108)
at org.mozilla.fenix.components.Core$engine$2.invoke(Core.kt:81)
at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74)
at org.mozilla.fenix.components.Core.getEngine(Unknown Source:2)
at org.mozilla.fenix.FenixApplication.onConfigurationChanged(FenixApplication.kt:484)

the problem seemingly stemming from components.core.engine.profiler?.addMarker(..)
@mcomella can you please check?

@mcomella
Copy link
Contributor

mcomella commented Oct 7, 2020

Thanks. I confirmed that I see the same behavior so I opened https://github.com/mozilla-mobile/fenix/issues/15751

@mcomella mcomella mentioned this issue Oct 7, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mcomella mcomella

Labels
needs:triage Issue needs triage performance Possible performance wins
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@Archaeopteryx @mcomella @Mugurell @sraturi