Only call Drawable.setVisible(false) for visible outgoing drawables

In framework views where we're handling the new visibility aggregated
call we only update the drawable visibility when we're attached to a
window. For old outgoing drawables being replaced, gate this on
whether the drawable is already marked visible instead.

This catches a case where views being inflated might set drawables in
in a superclass constructor and have them replaced in a later
constructor. Gating the call into a drawable that might invoke its
callback (the view being constructed) avoids potential problems where
overridden methods are called unexpectedly on a view subclass that has
not finished running its constructor.

This is a better check than isAttachedToWindow, as isAttachedToWindow
will return false if the view has been temporarily detached from its
parent by a view-recycling container. In those cases, the view would
not correctly update the outgoing drawable.

Bug 27461617

Change-Id: I733a2dd3e3df0a8d80d5dc542ca7b30064159d5d

2 files changed, 23 insertions, 5 deletions

diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java
index 358648432863..6d35a58c3be4 100644
--- a/core/java/android/view/View.java
+++ b/core/java/android/view/View.java
@@ -18002,7 +18002,13 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
          * to clear the previous drawable. setVisible first while we still have the callback set.
          */
         if (mBackground != null) {
-            if (isAttachedToWindow()) {
+            // It's possible for this method to be invoked from the View constructor before
+            // subclass constructors have run. Drawables can and should trigger invalidations
+            // and other activity with their callback on visibility changes, which shouldn't
+            // happen before subclass constructors finish. However, we won't have set the
+            // drawable as visible until the view becomes attached. This guard below keeps
+            // multiple calls to this method from constructors from causing issues.
+            if (mBackground.isVisible()) {
                 mBackground.setVisible(false, false);
             }
             mBackground.setCallback(null);
@@ -18237,7 +18243,13 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
         }
 
         if (mForegroundInfo.mDrawable != null) {
-            if (isAttachedToWindow()) {
+            // It's possible for this method to be invoked from the View constructor before
+            // subclass constructors have run. Drawables can and should trigger invalidations
+            // and other activity with their callback on visibility changes, which shouldn't
+            // happen before subclass constructors finish. However, we won't have set the
+            // drawable as visible until the view becomes attached. This guard below keeps
+            // multiple calls to this method from constructors from causing issues.
+            if (mForegroundInfo.mDrawable.isVisible()) {
                 mForegroundInfo.mDrawable.setVisible(false, false);
             }
             mForegroundInfo.mDrawable.setCallback(null);
diff --git a/core/java/android/widget/ImageView.java b/core/java/android/widget/ImageView.java
index 222a040d2b3c..02065779ef91 100644
--- a/core/java/android/widget/ImageView.java
+++ b/core/java/android/widget/ImageView.java
@@ -911,11 +911,17 @@ public class ImageView extends View {
         }
 
         if (mDrawable != null) {
-            mDrawable.setCallback(null);
-            unscheduleDrawable(mDrawable);
-            if (isAttachedToWindow()) {
+            // It's possible for this method to be invoked from the constructor before
+            // subclass constructors have run. Drawables can and should trigger invalidations
+            // and other activity with their callback on visibility changes, which shouldn't
+            // happen before subclass constructors finish. However, we won't have set the
+            // drawable as visible until the view becomes attached. This guard below keeps
+            // multiple calls to this method from constructors from causing issues.
+            if (mDrawable.isVisible()) {
                 mDrawable.setVisible(false, false);
             }
+            mDrawable.setCallback(null);
+            unscheduleDrawable(mDrawable);
         }
 
         mDrawable = d;