Fix Android T deprecated WindowManager INCORRECT_CONTEXT_USAGE #28774

GaryQian · 2021-09-21T20:01:25Z

Internal bug: b/199439780

Android T enforces not using an application context to get a WidowManager.

flutter-dashboard · 2021-09-21T20:01:27Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jason-simmons · 2021-09-21T21:09:34Z

Can this provide a Display or a refresh rate to the VsyncWaiter instead of passing a WindowManager?

That appears to be the only parameter required by the VsyncWaiter

GaryQian · 2021-09-21T21:17:24Z

Yeah, good catch, I'll modify to remove WindowManager from VsyncWaiter

blasten · 2021-09-22T18:10:34Z

shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java

-    VsyncWaiter.getInstance((WindowManager) appContext.getSystemService(Context.WINDOW_SERVICE))
-        .init();
+
+    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {


did it mean TIRAMISU ? https://source.corp.google.com/android/frameworks/base/core/java/android/os/Build.java;l=1137;bpv=0;bpt=1

This check is specifically making sure the new code is used only for API 23+ which is when the display API was added. I guess it could be gated to only apply to T and above, I have no strong opinions either way. End goal is just to obtain the FPS float.

ah ok, that's fine too then

Gating at 23 is more likely to have better test coverage as most of our testing is with API 30 and below (as of now), making this safer in the long run. I'll leave it as is.

The display API was actually added in 17. I'd like to change this, is there any good reason not to?

Or at least, all of the display code we're using :)

I'm changing this in #29800 - please add comments if it needs to be at 23 for some reason I'm missing.

That should hav ebeen #29800....

I see now - getSystemService was using the API 23 call instead of the one compatible with API 17. Fixing that upstream in my PR.

jason-simmons · 2021-09-22T18:10:30Z

shell/platform/android/io/flutter/embedding/engine/loader/FlutterLoader.java

+      final Display primaryDisplay = dm.getDisplay(Display.DEFAULT_DISPLAY);
+      VsyncWaiter.getInstance(primaryDisplay.getRefreshRate()).init();
+    } else {
+      VsyncWaiter.getInstance(


Factor the duplicated VsyncWaiter.getInstance().init() calls out of the if/else blocks.

blasten · 2021-09-22T18:13:17Z

shell/platform/android/io/flutter/view/VsyncWaiter.java

    }
    return instance;
  }

-  @NonNull private final WindowManager windowManager;
+  @NonNull private final float fps;


nit: @NonNull can be removed

blasten · 2021-09-22T18:14:18Z

shell/platform/android/io/flutter/view/VsyncWaiter.java

@@ -41,15 +39,14 @@ public void doFrame(long frameTimeNanos) {
        }
      };

-  private VsyncWaiter(@NonNull WindowManager windowManager) {
-    this.windowManager = windowManager;
+  private VsyncWaiter(@NonNull float fps) {


ditto in private constructor & in the getter above

blasten · 2021-09-22T18:27:22Z

shell/platform/android/test/io/flutter/embedding/engine/loader/FlutterLoaderTest.java

+    FlutterLoader flutterLoader = new FlutterLoader(mockFlutterJNI);
+
+    assertFalse(flutterLoader.initialized());
+    flutterLoader.startInitialization(RuntimeEnvironment.application);


can it also ensure that context.getSystemService(Context.WINDOW_SERVIC) isn't called?

blasten

LGTM

fluttergithubbot · 2021-09-23T00:53:02Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux Android Debug Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux Unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite build_and_test_linux_unopt_debug has failed. Please fix the issues identified (or deflake) before re-applying this label.

Formatting Version Gate Formatting

Remove extra import Remove extra var

import

Formatting formatting

GaryQian · 2021-09-23T23:37:55Z

Landing, LUCI redness is due to SkParagraph testing.

…cWaiter takes FPS float (flutter/engine#28774)

flutter-dashboard bot added platform-android needs tests labels Sep 21, 2021

google-cla bot added the cla: yes label Sep 21, 2021

GaryQian force-pushed the loader branch from 665d89f to 899ffbc Compare September 21, 2021 21:14

GaryQian force-pushed the loader branch from 38b99e0 to 9c26cfe Compare September 21, 2021 22:45

GaryQian requested review from jason-simmons and blasten September 22, 2021 17:52

blasten reviewed Sep 22, 2021

View reviewed changes

jason-simmons approved these changes Sep 22, 2021

View reviewed changes

blasten reviewed Sep 22, 2021

View reviewed changes

blasten approved these changes Sep 22, 2021

View reviewed changes

GaryQian added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 22, 2021

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 23, 2021

GaryQian added 11 commits September 23, 2021 02:30

Fix Android T deprecated WindowManager getter call

39a4059

Formatting Version Gate Formatting

Imports

433a7f5

VsyncWaiter takes Display instead of WindowManager

b5a86a8

Remove extra import Remove extra var

Version gate and take fps directly

2428099

import

Loader test

5bd268f

Test sdk differences

7f4ebd5

Rename tests

baea4ca

Address comments

49aee59

Formatting formatting

Verify getSystemService is not ccalled

1fc56c1

Missing import

8cea69e

Imports

03f137e

No mock static methods

38af333

GaryQian force-pushed the loader branch from 9080ba2 to 38af333 Compare September 23, 2021 09:30

GaryQian added 5 commits September 23, 2021 11:51

Fix tests

3115d91

Tests

ae196c1

Float literal

d41270f

Cleanup

99c4a19

Cleanup

8b9ccd1

GaryQian added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 23, 2021

GaryQian merged commit 6946640 into flutter:master Sep 23, 2021

engine-flutter-autoroll mentioned this pull request Sep 24, 2021

Roll Engine from eb94e4f595b6 to 694664037380 (6 revisions) flutter/flutter#90644

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 24, 2021

6946640 Fix deprecated WindowManager INCORRECT_CONTEXT_USAGE and Vsyn…

da6489a

…cWaiter takes FPS float (flutter/engine#28774)

blasten mentioned this pull request Dec 10, 2021

FlutterLoader loads WindowManager from getApplicationContext() flutter/flutter#89798

Closed

dnfield mentioned this pull request Nov 2, 2023

Accessing WindowManager from the application context causes strict mode violation flutter/flutter#83949

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Android T deprecated WindowManager INCORRECT_CONTEXT_USAGE #28774

Fix Android T deprecated WindowManager INCORRECT_CONTEXT_USAGE #28774

GaryQian commented Sep 21, 2021

flutter-dashboard bot commented Sep 21, 2021

jason-simmons commented Sep 21, 2021

GaryQian commented Sep 21, 2021

blasten Sep 22, 2021

GaryQian Sep 22, 2021

blasten Sep 22, 2021

GaryQian Sep 22, 2021

dnfield Nov 17, 2021

dnfield Nov 17, 2021

dnfield Nov 18, 2021 •

edited

Loading

dnfield Nov 18, 2021

dnfield Nov 18, 2021

jason-simmons Sep 22, 2021

blasten Sep 22, 2021

blasten Sep 22, 2021

blasten Sep 22, 2021 •

edited

Loading

blasten left a comment

fluttergithubbot commented Sep 23, 2021

GaryQian commented Sep 23, 2021

Fix Android T deprecated WindowManager INCORRECT_CONTEXT_USAGE #28774

Fix Android T deprecated WindowManager INCORRECT_CONTEXT_USAGE #28774

Conversation

GaryQian commented Sep 21, 2021

flutter-dashboard bot commented Sep 21, 2021

jason-simmons commented Sep 21, 2021

GaryQian commented Sep 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield Nov 18, 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

blasten Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

blasten left a comment

Choose a reason for hiding this comment

fluttergithubbot commented Sep 23, 2021

GaryQian commented Sep 23, 2021

dnfield Nov 18, 2021 •

edited

Loading

blasten Sep 22, 2021 •

edited

Loading