From 0969fa22c994319f9c7ad8ba9f5984ff47f1c5b5 Mon Sep 17 00:00:00 2001 From: Andy Street Date: Mon, 19 Oct 2020 04:17:04 -0700 Subject: [PATCH] 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/litho/ComponentLifecycle.java | 21 +++++++++-- .../com/facebook/litho/ComponentTree.java | 2 +- .../com/facebook/litho/ComponentUtils.java | 20 +++++------ .../LithoMetadataExceptionWrapperTest.java | 35 +++++++++++++++++++ .../generator/EventGeneratorTest.java | 4 +-- .../specmodels/generator/EventGenerator.java | 8 ++--- .../litho/sections/SectionLifecycle.java | 16 ++++++++- .../facebook/litho/testing/TestComponent.java | 2 +- .../inlinelayoutspec/InlineLayoutSpec.java | 2 +- 9 files changed, 88 insertions(+), 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 index c31ee0f2b54..a671f6728bb 100644 --- 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 index 7c4a1904865..7632b20b0fb 100644 --- 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 index edac0012337..7ad05c09343 100644 --- 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 index e33f4c32bc7..852d69c2296 100644 --- 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 index 11d8a2aa4c7..8049a1a9e63 100644 --- 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 index eca7033f037..9ef119e4bad 100644 --- 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 index 38ddab2916d..18e6adc875a 100644 --- 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 index 75e64785f0f..1965bca6498 100644 --- 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/litho-testing/src/main/java/com/facebook/litho/testing/inlinelayoutspec/InlineLayoutSpec.java b/litho-testing/src/main/java/com/facebook/litho/testing/inlinelayoutspec/InlineLayoutSpec.java index e7117155f29..a7c09605ac6 100644 --- a/litho-testing/src/main/java/com/facebook/litho/testing/inlinelayoutspec/InlineLayoutSpec.java +++ b/litho-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; }