Guard against NewAPI failures #8048

mklim · 2019-03-05T22:29:52Z

The Java linter is pointing out some code paths that will cause crashes in older supported API versions. Most of this PR is just adding clear guards for code paths that look like they wouldn't effectively have been executed in lower SDKs anyway, but I did see a couple cases here that look pretty suspicious. We may want to file followup bugs for them.

flutter/flutter#28848

shell/platform/android/io/flutter/embedding/engine/android/FlutterSurfaceView.java

shell/platform/android/io/flutter/plugin/platform/PlatformPlugin.java

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

mklim · 2019-03-05T22:58:45Z

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

@@ -242,7 +251,7 @@ private boolean extractUpdate(File dataDir) {
        for (String asset : mResources) {
            String resource = null;
            ZipEntry entry = null;
-            if (asset.endsWith(".so")) {
+            if (asset.endsWith(".so") && Build.VERSION.SDK_INT > Build.VERSION_CODES.KITKAT_WATCH) {


Not being able to fetch these on older SDKs may be a bug? Build.SUPPORTED_ABIS is the problem.

Maybe it would be better to guard the for loop with this, and in the else block just get it from "lib/" + asset; or whatever the appropriate non-ABI aware thing is for lower APIs

Here again with mixing SDK checks and app logic.

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

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

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

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

tools/android_lint/project.xml

shell/platform/android/io/flutter/embedding/engine/android/FlutterSurfaceView.java

shell/platform/android/io/flutter/embedding/engine/android/FlutterView.java

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

matthew-carroll · 2019-03-05T23:21:53Z

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

@@ -325,7 +327,7 @@ public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) {
                        result.setClassName("android.widget.HorizontalScrollView");
                    }
                } else {
-                    if (shouldSetCollectionInfo(object)) {
+                    if (Build.VERSION.SDK_INT > Build.VERSION_CODES.JELLY_BEAN_MR2 && shouldSetCollectionInfo(object)) {


Here and elsewhere, can you add comments about what happens if the SDK versions are not met?

Added to the inline comments above this block.

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

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

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

matthew-carroll · 2019-03-05T23:24:34Z

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

@@ -242,7 +251,7 @@ private boolean extractUpdate(File dataDir) {
        for (String asset : mResources) {
            String resource = null;
            ZipEntry entry = null;
-            if (asset.endsWith(".so")) {
+            if (asset.endsWith(".so") && Build.VERSION.SDK_INT > Build.VERSION_CODES.KITKAT_WATCH) {


Here again with mixing SDK checks and app logic.

mklim

This patch should address all pending feedback. Let me know if I missed something and I'll circle back.

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

mklim · 2019-03-08T01:24:13Z

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

@@ -325,7 +327,7 @@ public AccessibilityNodeInfo createAccessibilityNodeInfo(int virtualViewId) {
                        result.setClassName("android.widget.HorizontalScrollView");
                    }
                } else {
-                    if (shouldSetCollectionInfo(object)) {
+                    if (Build.VERSION.SDK_INT > Build.VERSION_CODES.JELLY_BEAN_MR2 && shouldSetCollectionInfo(object)) {


Added to the inline comments above this block.

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

dnfield

LGTM. Will you be able to document what's not supported here? Do we have an issue filed to track that?

mklim · 2019-03-08T02:03:57Z

Will you be able to document what's not supported here? Do we have an issue filed to track that?

Filed flutter/flutter#29019 to track the Accessibility gap. I'm good to document it further if you think it should have its own wiki page. I thought an issue made sense for now since we probably do want to close this gap though.

The other larger uncovered gap here is with the SUPPORTED_ABIs. I'm not sure if this was intended to be available on older APIs originally, but if so I'll follow up. /cc @sbaranov

matthew-carroll · 2019-03-08T02:07:30Z

LGTM - would be helpful if this could land after AccessibilityBridge comments and refactor:
#8061

Revert those failures. Also actually fix up the relative paths in the linter XML files.

mklim · 2019-03-08T21:33:21Z

would be helpful if this could land after AccessibilityBridge comments and refactor: #8061

Discussed offline, that PR is blocked on these lint failures. Going to merge this first and fix conflicts there.

flutter/engine@87edd94...c48774c git log 87edd94..c48774c --no-merges --oneline c48774c Roll src/third_party/dart 571ea80e11..2fb6cd9f5f (122 commits) (flutter/engine#8086) 3c8ef04 Allow embedders to post tasks onto the render thread. (flutter/engine#8089) 1d10e0e Guard against NewAPI failures (flutter/engine#8048) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (mklim@google.com), and stop the roller if necessary.

PR flutter#8048 in flutter/engine produced a bug (flutter/flutter flutter#34791) in flutter for Android that doesn't permit the Recents app bar color to be changed. This restores the original arguments to the function found in the previous version (https://github.com/flutter/engine/blob/2f4a38dbd33061e950cf83ddc31785803d58c126/shell/platform/android/io/flutter/plugin/platform/PlatformPlugin.java) of PlatformPlugin.java while preserving the enhancements for the linter.

This is a fix for [flutter/flutter issue #34791](flutter/flutter#34791). PR #8048 in flutter/engine produced a bug/regression (flutter/flutter #34791) in flutter for Android that doesn't permit the Recents app bar color to be changed. This restores the original arguments to the function found in the previous version (https://github.com/flutter/engine/blob/2f4a38dbd33061e950cf83ddc31785803d58c126/shell/platform/android/io/flutter/plugin/platform/PlatformPlugin.java) of PlatformPlugin.java while preserving the enhancements for the linter. I've compiled and tested this fix locally. The bar changes color again.

googlebot added the cla: yes label Mar 5, 2019

mklim commented Mar 5, 2019

View reviewed changes

mklim requested review from dnfield, matthew-carroll and jason-simmons March 5, 2019 23:04

mklim marked this pull request as ready for review March 5, 2019 23:05

mklim requested a review from goderbauer March 5, 2019 23:07

dnfield reviewed Mar 5, 2019

View reviewed changes

shell/platform/android/io/flutter/view/AccessibilityBridge.java Show resolved Hide resolved

dnfield reviewed Mar 5, 2019

View reviewed changes

shell/platform/android/io/flutter/view/FlutterView.java Outdated Show resolved Hide resolved

dnfield reviewed Mar 5, 2019

View reviewed changes

shell/platform/android/io/flutter/view/ResourceExtractor.java Outdated Show resolved Hide resolved

dnfield reviewed Mar 5, 2019

View reviewed changes

shell/platform/android/io/flutter/view/ResourceExtractor.java Outdated Show resolved Hide resolved

dnfield reviewed Mar 5, 2019

View reviewed changes

tools/android_lint/project.xml Outdated Show resolved Hide resolved

matthew-carroll reviewed Mar 5, 2019

View reviewed changes

mklim commented Mar 8, 2019

View reviewed changes

dnfield approved these changes Mar 8, 2019

View reviewed changes

Michael Klimushyn added 5 commits March 8, 2019 09:53

Guard against NewAPI failures

cacbdf5

Review fixup.

d47b8fe

Ignore try-with-resoruces failures

edbe7b0

Revert those failures. Also actually fix up the relative paths in the linter XML files.

More misc. review cleanup.

7ea55c8

Use fallback logic for SUPPORTED_ABIS

01b24cf

mklim merged commit 1d10e0e into flutter:master Mar 8, 2019

mklim deleted the newapi branch March 8, 2019 21:34

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 9, 2019

1d10e0e Guard against NewAPI failures (flutter/engine#8048)

5fc427c

engine-flutter-autoroll mentioned this pull request Mar 9, 2019

Roll engine 87edd94e9cc7..1d10e0eb67fa (1 commits) flutter/flutter#29074

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 9, 2019

1d10e0e Guard against NewAPI failures (flutter/engine#8048)

84ddc7c

engine-flutter-autoroll mentioned this pull request Mar 9, 2019

Roll engine 87edd94e9cc7..c48774cfe09b (3 commits) flutter/flutter#29079

Merged

This was referenced Jul 21, 2019

How to set Android app "window" (screenshot says all) color? flutter/flutter#34791

Closed

Fix flutter/flutter #34791 #9977

Merged

Guard against NewAPI failures #8048

Guard against NewAPI failures #8048

Uh oh!

Conversation

mklim commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mklim Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

dnfield Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

matthew-carroll Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthew-carroll Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

mklim Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthew-carroll Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mklim Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

mklim commented Mar 8, 2019

Uh oh!

matthew-carroll commented Mar 8, 2019

Uh oh!

mklim commented Mar 8, 2019

Uh oh!

Uh oh!

mklim commented Mar 5, 2019 •

edited

Loading