Add exception metadata wrapping for OnEvent methods

Summary: To help with times we get a crash when invoking error handler code, now wrap calls to event dispatching (other than OnError) in a boundary that will wrap in a metadata wrapper. Ex: https://fb.workplace.com/groups/147373332784023/permalink/876584686529547/?comment_id=876585946529421 Reviewed By: adityasharat Differential Revision: D24362286 fbshipit-source-id: 932036adeae552baa650005dcb1c865ab5334fa5
facebook · Oct 19, 2020 · 0969fa2 · 0969fa2
1 parent b07f38e
commit 0969fa2
Show file tree

Hide file tree

Showing 9 changed files with 88 additions and 22 deletions.
diff --git a/litho-core/src/main/java/com/facebook/litho/ComponentLifecycle.java b/litho-core/src/main/java/com/facebook/litho/ComponentLifecycle.java
@@ -124,9 +124,26 @@ public Object createMountContent(Context c) {
   }
 
   @Override
-  public @Nullable Object dispatchOnEvent(EventHandler eventHandler, Object eventState) {
+  public final @Nullable Object dispatchOnEvent(EventHandler eventHandler, Object eventState) {
+    // We don't want to wrap and throw error events
     if (eventHandler.id == ERROR_EVENT_HANDLER_ID) {
-      ((Component) this).getErrorHandler().dispatchEvent(((ErrorEvent) eventState));
+      return dispatchOnEventImpl(eventHandler, eventState);
+    }
+
+    try {
+      return dispatchOnEventImpl(eventHandler, eventState);
+    } catch (Exception e) {
+      if (eventHandler.params != null && eventHandler.params[0] instanceof ComponentContext) {
+        throw ComponentUtils.wrapWithMetadata((ComponentContext) eventHandler.params[0], e);
+      } else {
+        throw e;
+      }
+    }
+  }
+
+  protected @Nullable Object dispatchOnEventImpl(EventHandler eventHandler, Object eventState) {
+    if (eventHandler.id == ERROR_EVENT_HANDLER_ID) {
+      getErrorHandler().dispatchEvent((ErrorEvent) eventState);
     }
 
     // Don't do anything by default, unless we're handling an error.

diff --git a/litho-core/src/main/java/com/facebook/litho/ComponentTree.java b/litho-core/src/main/java/com/facebook/litho/ComponentTree.java
@@ -827,7 +827,7 @@ private void mountComponentInternal(
         recordRenderData(layoutState);
       }
     } catch (Exception e) {
-      ComponentUtils.wrapWithMetadataAndThrow(this, e);
+      throw ComponentUtils.wrapWithMetadata(this, e);
     } finally {
       mIsMounting = false;
       mRootHeightAnimation = null;

diff --git a/litho-core/src/main/java/com/facebook/litho/ComponentUtils.java b/litho-core/src/main/java/com/facebook/litho/ComponentUtils.java
@@ -512,9 +512,9 @@ static void handle(ComponentContext c, Exception exception) {
     try {
       dispatchErrorEvent(c, exception);
     } catch (ReThrownException re) {
-      wrapWithMetadataAndThrow(c, exception);
+      throw wrapWithMetadata(c, exception);
     } catch (Exception e) {
-      wrapWithMetadataAndThrow(c, e);
+      throw wrapWithMetadata(c, e);
     }
   }
 
@@ -531,23 +531,23 @@ static void rethrow(Exception e) {
 
   /**
    * Uses the given ComponentContext to add metadata to a wrapper exception (if the wrapper doesn't
-   * already exist) and rethrows.
+   * already exist) and return it.
    */
-  public static void wrapWithMetadataAndThrow(ComponentContext c, Exception e) {
+  public static LithoMetadataExceptionWrapper wrapWithMetadata(ComponentContext c, Exception e) {
     if (e instanceof LithoMetadataExceptionWrapper) {
-      throw (LithoMetadataExceptionWrapper) e;
+      return (LithoMetadataExceptionWrapper) e;
     }
-    throw new LithoMetadataExceptionWrapper(c, e);
+    return new LithoMetadataExceptionWrapper(c, e);
   }
 
   /**
    * Uses the given ComponentTree to add metadata to a wrapper exception (if the wrapper doesn't
-   * already exist) and rethrows.
+   * already exist) and return it.
    */
-  public static void wrapWithMetadataAndThrow(ComponentTree c, Exception e) {
+  public static LithoMetadataExceptionWrapper wrapWithMetadata(ComponentTree c, Exception e) {
     if (e instanceof LithoMetadataExceptionWrapper) {
-      throw (LithoMetadataExceptionWrapper) e;
+      return (LithoMetadataExceptionWrapper) e;
     }
-    throw new LithoMetadataExceptionWrapper(c, e);
+    return new LithoMetadataExceptionWrapper(c, e);
   }
 }
diff --git a/litho-it/src/test/java/com/facebook/litho/LithoMetadataExceptionWrapperTest.java b/litho-it/src/test/java/com/facebook/litho/LithoMetadataExceptionWrapperTest.java
@@ -16,15 +16,18 @@
 
 package com.facebook.litho;
 
+import static com.facebook.litho.TouchExpansionDelegateTest.emulateClickEvent;
 import static org.hamcrest.core.Is.is;
 import static org.junit.Assume.assumeThat;
 
+import android.view.View;
 import com.facebook.litho.config.ComponentsConfiguration;
 import com.facebook.litho.testing.ComponentsRule;
 import com.facebook.litho.testing.LithoViewRule;
 import com.facebook.litho.testing.error.TestCrasherOnCreateLayout;
 import com.facebook.litho.testing.error.TestHasDelegateThatCrashesOnCreateLayout;
 import com.facebook.litho.testing.testrunner.LithoTestRunner;
+import com.facebook.litho.widget.OnClickCallbackComponent;
 import com.facebook.litho.widget.OnErrorNotPresentChild;
 import com.facebook.litho.widget.OnErrorPassUpChildTester;
 import com.facebook.litho.widget.OnErrorPassUpParentTester;
@@ -152,4 +155,36 @@ public void onMount_withLogTag_showsLogTagInStack() {
         .layout()
         .attachToWindow();
   }
+
+  @Test
+  public void onClickEvent_withLogTag_showsLogTagInStack() {
+    mExpectedException.expect(LithoMetadataExceptionWrapper.class);
+    mExpectedException.expectMessage("log_tag: myLogTag");
+
+    final ComponentContext c =
+        new ComponentContext(RuntimeEnvironment.application, "myLogTag", null);
+    final Component component =
+        Column.create(c)
+            .child(
+                OnClickCallbackComponent.create(c)
+                    .widthPx(10)
+                    .heightPx(10)
+                    .callback(
+                        new View.OnClickListener() {
+                          @Override
+                          public void onClick(View v) {
+                            throw new RuntimeException("Expected test exception");
+                          }
+                        }))
+            .build();
+
+    mLithoViewRule
+        .useComponentTree(ComponentTree.create(c).build())
+        .setRoot(component)
+        .attachToWindow()
+        .measure()
+        .layout();
+
+    emulateClickEvent(mLithoViewRule.getLithoView(), 7, 7);
+  }
 }
diff --git a/litho-it/src/test/java/com/facebook/litho/specmodels/generator/EventGeneratorTest.java b/litho-it/src/test/java/com/facebook/litho/specmodels/generator/EventGeneratorTest.java
@@ -168,10 +168,10 @@ public void testGenerateEventHandlerFactories() {
 
   @Test
   public void testGenerateDispatchOnEvent() {
-    assertThat(EventGenerator.generateDispatchOnEvent(mSpecModel).toString())
+    assertThat(EventGenerator.generateDispatchOnEventImpl(mSpecModel).toString())
         .isEqualTo(
             "@java.lang.Override\n"
-                + "public java.lang.Object dispatchOnEvent(final com.facebook.litho.EventHandler eventHandler,\n"
+                + "protected java.lang.Object dispatchOnEventImpl(final com.facebook.litho.EventHandler eventHandler,\n"
                 + "    final java.lang.Object eventState) {\n"
                 + "  int id = eventHandler.id;\n"
                 + "  switch (id) {\n"

diff --git a/litho-processor/src/main/java/com/facebook/litho/specmodels/generator/EventGenerator.java b/litho-processor/src/main/java/com/facebook/litho/specmodels/generator/EventGenerator.java
@@ -62,7 +62,7 @@ public static TypeSpecDataHolder generate(SpecModel specModel) {
             .addTypeSpecDataHolder(generateEventHandlerFactories(specModel));
 
     if (!specModel.getEventMethods().isEmpty()) {
-      builder.addMethod(generateDispatchOnEvent(specModel));
+      builder.addMethod(generateDispatchOnEventImpl(specModel));
     }
 
     return builder.build();
@@ -275,10 +275,10 @@ static String getContextParamName(SpecModel specModel, SpecMethodModel eventMeth
   }
 
   /** Generate a dispatchOnEvent() implementation for the component. */
-  static MethodSpec generateDispatchOnEvent(SpecModel specModel) {
+  static MethodSpec generateDispatchOnEventImpl(SpecModel specModel) {
     final MethodSpec.Builder methodBuilder =
-        MethodSpec.methodBuilder("dispatchOnEvent")
-            .addModifiers(Modifier.PUBLIC)
+        MethodSpec.methodBuilder("dispatchOnEventImpl")
+            .addModifiers(Modifier.PROTECTED)
             .addAnnotation(Override.class)
             .returns(TypeName.OBJECT)
             .addParameter(

diff --git a/litho-sections-core/src/main/java/com/facebook/litho/sections/SectionLifecycle.java b/litho-sections-core/src/main/java/com/facebook/litho/sections/SectionLifecycle.java
@@ -19,6 +19,8 @@
 import static com.facebook.litho.sections.SectionContext.NO_SCOPE_EVENT_HANDLER;
 
 import androidx.annotation.Nullable;
+import com.facebook.litho.ComponentContext;
+import com.facebook.litho.ComponentUtils;
 import com.facebook.litho.ComponentsReporter;
 import com.facebook.litho.EventDispatcher;
 import com.facebook.litho.EventHandler;
@@ -83,7 +85,19 @@ protected void createService(SectionContext c) {}
 
   @Nullable
   @Override
-  public Object dispatchOnEvent(EventHandler eventHandler, Object eventState) {
+  public final Object dispatchOnEvent(EventHandler eventHandler, Object eventState) {
+    try {
+      return dispatchOnEventImpl(eventHandler, eventState);
+    } catch (Exception e) {
+      if (eventHandler.params != null && eventHandler.params[0] instanceof ComponentContext) {
+        throw ComponentUtils.wrapWithMetadata((ComponentContext) eventHandler.params[0], e);
+      } else {
+        throw e;
+      }
+    }
+  }
+
+  protected @Nullable Object dispatchOnEventImpl(EventHandler eventHandler, Object eventState) {
     // Do nothing by default.
     return null;
   }

diff --git a/litho-testing/src/main/java/com/facebook/litho/testing/TestComponent.java b/litho-testing/src/main/java/com/facebook/litho/testing/TestComponent.java
@@ -174,7 +174,7 @@ public synchronized void resetInteractions() {
   }
 
   @Override
-  public Object dispatchOnEvent(EventHandler eventHandler, Object eventState) {
+  public Object dispatchOnEventImpl(EventHandler eventHandler, Object eventState) {
     mDispatchedEventHandlers.put(eventHandler, eventState);
     return null;
   }

diff --git a/...o-testing/src/main/java/com/facebook/litho/testing/inlinelayoutspec/InlineLayoutSpec.java b/...o-testing/src/main/java/com/facebook/litho/testing/inlinelayoutspec/InlineLayoutSpec.java
@@ -43,7 +43,7 @@ public boolean isEquivalentTo(Component other, boolean shouldCompareState) {
   }
 
   @Override
-  public Object dispatchOnEvent(EventHandler eventHandler, Object eventState) {
+  public Object dispatchOnEventImpl(EventHandler eventHandler, Object eventState) {
     // no-op
     return null;
   }