Add ability to log when forbidden view property modification happens …

…on ComponentHost

Summary:
I have an hypothesis that there are "forbidden" modifications of View properties (such as click/touch listeners) on `ComponentHost`. This means that if we try to recycle them, we will carry over click listeners that were set by clients, but never unset by litho - and this will lead to erratic behavior.

I want to add some logging that allows us to hopefully detect these situations and emit an MME so that we can understand their sources and proceed to fix them.

Reviewed By: adityasharat

Differential Revision: D53126942

fbshipit-source-id: c1a895ac4b22308de34c4747ccdf19541701df21

litho-core/src/main/java/com/facebook/litho/ComponentHost.java

@@ -21,6 +21,7 @@
 import static com.facebook.litho.LithoRenderUnit.getRenderUnit;
 import static com.facebook.litho.LithoRenderUnit.isTouchableDisabled;
 import static com.facebook.litho.ThreadUtils.assertMainThread;
+import static com.facebook.rendercore.debug.DebugEventAttribute.Key;
 
 import android.annotation.SuppressLint;
 import android.content.Context;
@@ -45,14 +46,18 @@
 import androidx.core.graphics.drawable.DrawableCompat;
 import androidx.core.view.ViewCompat;
 import com.facebook.litho.config.ComponentsConfiguration;
+import com.facebook.litho.debug.LithoDebugEvent;
 import com.facebook.proguard.annotations.DoNotStrip;
 import com.facebook.rendercore.Host;
+import com.facebook.rendercore.LogLevel;
 import com.facebook.rendercore.MountItem;
 import com.facebook.rendercore.MountState;
+import com.facebook.rendercore.debug.DebugEventDispatcher;
 import com.facebook.rendercore.transitions.DisappearingHost;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
+import kotlin.Unit;
 
 /**
  * A {@link ViewGroup} that can host the mounted state of a {@link Component}. This is used by
@@ -102,6 +107,7 @@ public class ComponentHost extends Host implements DisappearingHost {
   private TouchExpansionDelegate mTouchExpansionDelegate;
 
   interface ExceptionLogMessageProvider {
+
     StringBuilder getLogMessage();
   }
 
@@ -120,8 +126,9 @@ interface ExceptionLogMessageProvider {
    */
   private boolean mImplementsVirtualViews = false;
 
-  public ComponentHost(Context context) {
+  public ComponentHost(Context context, boolean logUnsafeViewModifications) {
     this(context, null);
+    mLogUnsafeViewModificationsEnabled = logUnsafeViewModifications;
   }
 
   public ComponentHost(ComponentContext context) {
@@ -1503,4 +1510,71 @@ public void setInLayout() {
   public void unsetInLayout() {
     mInLayout = false;
   }
+
+  /**
+   * This is used to help in an investigation around unsafe setting/unsetting of click/touch
+   * listeners, and which could help to unblock host recycling.
+   */
+  private boolean mLogUnsafeViewModificationsEnabled;
+
+  /**
+   * This flag is used to understand if a view property (e.g, click listener) was modified under the
+   * context of a Litho operation or not. It is used to detect unsafe modifications and log them.
+   *
+   * @see {@link LithoViewAttributesExtension}
+   */
+  private boolean mIsSafeViewModificationsEnabled;
+
+  protected void setSafeViewModificationsEnabled(boolean enabled) {
+    mIsSafeViewModificationsEnabled = enabled;
+  }
+
+  private void checkUnsafeViewModification() {
+    if (mLogUnsafeViewModificationsEnabled && !mIsSafeViewModificationsEnabled) {
+      DebugEventDispatcher.dispatch(
+          LithoDebugEvent.DebugInfo,
+          () -> "-1",
+          LogLevel.DEBUG,
+          (attribute) -> {
+            attribute.put(Key, "unsafe-component-host-modification");
+            return Unit.INSTANCE;
+          });
+    }
+  }
+
+  @Override
+  public void setOnClickListener(@Nullable OnClickListener l) {
+    checkUnsafeViewModification();
+    super.setOnClickListener(l);
+  }
+
+  @Override
+  public void setOnLongClickListener(@Nullable OnLongClickListener l) {
+    checkUnsafeViewModification();
+    super.setOnLongClickListener(l);
+  }
+
+  @Override
+  public void setOnTouchListener(OnTouchListener l) {
+    checkUnsafeViewModification();
+    super.setOnTouchListener(l);
+  }
+
+  @Override
+  public void setTag(Object tag) {
+    checkUnsafeViewModification();
+    super.setTag(tag);
+  }
+
+  @Override
+  public void setEnabled(boolean enabled) {
+    checkUnsafeViewModification();
+    super.setEnabled(enabled);
+  }
+
+  @Override
+  public void setOnFocusChangeListener(OnFocusChangeListener l) {
+    checkUnsafeViewModification();
+    super.setOnFocusChangeListener(l);
+  }
 }

litho-core/src/main/java/com/facebook/litho/HostComponent.java

@@ -35,11 +35,13 @@ class HostComponent extends SpecGeneratedComponent {
   @Nullable private SparseArray<DynamicValue<?>> mCommonDynamicProps;
 
   private boolean mImplementsVirtualViews = false;
-  private boolean mRecyclingEnabled;
+  private final boolean mRecyclingEnabled;
+  private final boolean mLogUnsafeViewModifications;
 
-  protected HostComponent(boolean recyclingEnabled) {
+  protected HostComponent(boolean recyclingEnabled, boolean logUnsafeViewModifications) {
     super("HostComponent");
     mRecyclingEnabled = recyclingEnabled;
+    mLogUnsafeViewModifications = logUnsafeViewModifications;
   }
 
   @Override
@@ -60,7 +62,7 @@ public boolean canPreallocate() {
 
   @Override
   protected Object onCreateMountContent(Context c) {
-    return new ComponentHost(c);
+    return new ComponentHost(c, mLogUnsafeViewModifications);
   }
 
   @Override
@@ -113,8 +115,12 @@ public MountType getMountType() {
   }
 
   static HostComponent create(ComponentContext context) {
+    ComponentsConfiguration componentsConfiguration =
+        context.getLithoConfiguration().componentsConfig;
+
     return new HostComponent(
-        context.getLithoConfiguration().componentsConfig.componentHostRecyclingEnabled);
+        componentsConfiguration.componentHostRecyclingEnabled,
+        componentsConfiguration.componentHostUnsafeModificationsLoggingEnabled);
   }
 
   @Override

litho-core/src/main/java/com/facebook/litho/LithoViewAttributesExtension.kt

@@ -187,6 +187,11 @@ class LithoViewAttributesExtension private constructor() :
       if (content !is View) {
         return
       }
+
+      if (content is ComponentHost) {
+        content.setSafeViewModificationsEnabled(true)
+      }
+
       setClickHandler(attributes.clickHandler, content)
       setLongClickHandler(attributes.longClickHandler, content)
       setFocusChangeHandler(attributes.focusChangeHandler, content)
@@ -243,6 +248,10 @@ class LithoViewAttributesExtension private constructor() :
         setViewForeground(content, attributes.foreground)
         setViewLayoutDirection(content, attributes)
       }
+
+      if (content is ComponentHost) {
+        content.setSafeViewModificationsEnabled(false)
+      }
     }
 
     @JvmStatic
@@ -251,6 +260,11 @@ class LithoViewAttributesExtension private constructor() :
       if (content !is View) {
         return
       }
+
+      if (content is ComponentHost) {
+        content.setSafeViewModificationsEnabled(true)
+      }
+
       if (attributes.clickHandler != null) {
         unsetClickHandler(content)
       }
@@ -311,7 +325,12 @@ class LithoViewAttributesExtension private constructor() :
         unsetViewLayoutDirection(content)
       }
       unsetViewLayerType(content, mountFlags)
+
+      if (content is ComponentHost) {
+        content.setSafeViewModificationsEnabled(false)
+      }
     }
+
     /**
      * Store a [NodeInfo] as a tag in `view`. [LithoView] contains the logic for setting/unsetting
      * it whenever accessibility is enabled/disabled
@@ -336,6 +355,7 @@ class LithoViewAttributesExtension private constructor() :
         ViewCompat.setAccessibilityDelegate(view, null)
       }
     }
+
     /**
      * Installs the click listeners that will dispatch the click handler defined in the component's
      * props. Unconditionally set the clickable flag on the view.
@@ -352,6 +372,7 @@ class LithoViewAttributesExtension private constructor() :
       view.setOnClickListener(null)
       view.isClickable = false
     }
+
     /**
      * Installs the long click listeners that will dispatch the click handler defined in the
      * component's props. Unconditionally set the clickable flag on the view.
@@ -392,6 +413,7 @@ class LithoViewAttributesExtension private constructor() :
         v.setTag(R.id.component_long_click_listener, listener)
       }
     }
+
     /**
      * Installs the on focus change listeners that will dispatch the click handler defined in the
      * component's props. Unconditionally set the clickable flag on the view.
@@ -435,6 +457,7 @@ class LithoViewAttributesExtension private constructor() :
         v.setTag(R.id.component_focus_change_listener, listener)
       }
     }
+
     /**
      * Installs the touch listeners that will dispatch the touch handler defined in the component's
      * props.
@@ -456,6 +479,7 @@ class LithoViewAttributesExtension private constructor() :
         listener.eventHandler = null
       }
     }
+
     /** Sets the intercept touch handler defined in the component's props. */
     private fun setInterceptTouchHandler(
         interceptTouchHandler: EventHandler<InterceptTouchEvent>?,

litho-core/src/main/java/com/facebook/litho/config/ComponentsConfiguration.kt

@@ -79,7 +79,12 @@ internal constructor(
      * By default, we do not allow preallocation of [Drawable]. By turning this option on, you can
      * also preallocate [Drawable] mount content, when [mountContentPreallocation] is enabled.
      */
-    @JvmField val enableDrawablePreAllocation: Boolean = false
+    @JvmField val enableDrawablePreAllocation: Boolean = false,
+    /**
+     * If [true] this will log debug events whenever we detect that a click/touch handler was set on
+     * a [com.facebook.litho.ComponentHost] outside of the [ViewAttributesExtension].
+     */
+    @JvmField val componentHostUnsafeModificationsLoggingEnabled: Boolean = false
 ) {
 
   val shouldAddRootHostViewOrDisableBgFgOutputs: Boolean =
@@ -239,6 +244,8 @@ internal constructor(
         baseConfig.shouldEnableDefaultAOSPLithoLifecycleProvider
     private var enableStateUpdatesBatching = baseConfig.enableStateUpdatesBatching
     private var enableDrawablePreAllocation = baseConfig.enableDrawablePreAllocation
+    private var componentHostUnsafeModificationsLoggingEnabled =
+        baseConfig.componentHostUnsafeModificationsLoggingEnabled
 
     fun shouldAddHostViewForRootComponent(enabled: Boolean): Builder = also {
       shouldAddHostViewForRootComponent = enabled
@@ -280,6 +287,10 @@ internal constructor(
       enableDrawablePreAllocation = enabled
     }
 
+    fun componentHostUnsafeModificationsLoggingEnabled(enabled: Boolean): Builder = also {
+      componentHostUnsafeModificationsLoggingEnabled = enabled
+    }
+
     fun build(): ComponentsConfiguration {
       return baseConfig.copy(
           specsApiStateUpdateDuplicateDetectionEnabled =
@@ -293,7 +304,9 @@ internal constructor(
           shouldEnableDefaultAOSPLithoLifecycleProvider =
               shouldEnableDefaultAOSPLithoLifecycleProvider,
           enableStateUpdatesBatching = enableStateUpdatesBatching,
-          enableDrawablePreAllocation = enableDrawablePreAllocation)
+          enableDrawablePreAllocation = enableDrawablePreAllocation,
+          componentHostUnsafeModificationsLoggingEnabled =
+              componentHostUnsafeModificationsLoggingEnabled)
     }
   }
 }

litho-it/src/test/java/com/facebook/litho/ComponentHostTest.kt

@@ -727,7 +727,7 @@ class ComponentHostTest {
 
     constructor(context: ComponentContext?) : super(context)
 
-    constructor(context: Context?) : super(context)
+    constructor(context: Context?) : super(context, false)
 
     override fun invalidate(dirty: Rect) {
       super.invalidate(dirty)

litho-it/src/test/java/com/facebook/litho/MountItemTest.kt

@@ -63,7 +63,7 @@ class MountItemTest {
           override fun onCreateLayout(c: ComponentContext): Component =
               SimpleMountSpecTester.create(c).build()
         }
-    componentHost = ComponentHost(getApplicationContext<Context>())
+    componentHost = ComponentHost(getApplicationContext<Context>(), false)
     content = View(getApplicationContext())
     contentDescription = "contentDescription"
     viewTag = "tag"

litho-it/src/test/java/com/facebook/litho/TouchExpansionDelegateTest.kt

@@ -48,7 +48,7 @@ class TouchExpansionDelegateTest {
 
   @Test
   fun onTouchEventOnEmptyDelegate_shouldNoOp() {
-    val host = ComponentHost(ApplicationProvider.getApplicationContext<Context>())
+    val host = ComponentHost(ApplicationProvider.getApplicationContext<Context>(), false)
     val delegate = TouchExpansionDelegate(host)
     val handled =
         delegate.onTouchEvent(