CPLAT-8600: Unrecoverable ErrorBoundary Fix #435

kealjones-wk · 2019-12-04T19:54:12Z

Motivation

Even with ErrorBoundaries in place, we were seeing issues with the app still entirely unmounting even with the logic for "unrecoverable" errors. It turns out in React if an ErrorBoundary catches an error, it will have its error lifecycle methods called, if on that render cycle the component throws during the render cycle React will move up the tree to a parent ErrorBoundary instead. And due to all the Previous ErrorBoundaries having the same logic none would ever handle the render cycle error.

Changes

Updated ErrorBoundaryComponent to handle render cycle unrecoverable errors (hopefully this wont happen often.)
Moved the previous ErrorBoundary logic into RecoverableErrorBoundary and nested it as the first child of the updated ErrorBoundary.
- This nesting allows us to know that if RecoverableErrorBoundary was unable to recover, that the error is "Unrecoverable" and needs to be handled.

Release Notes

Updated ErrorBoundary to better handle Unrecoverable Errors that might be thrown during the render cycle.
Deprecated ErrorBoundaryMixin as it only handles errors that are able to be "recovered" and recommend using ErrorBoundary with your custom props instead.

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

Tests were updated and provide good coverage of the changeset and other affected code
Manual testing was performed if needed
- Steps from PR author:
- Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

A Client Platform member has reviewed these changes
There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
For release PRs - Version metadata in Rosie comment is correct

aviary2-wf · 2019-12-04T19:54:40Z

Security Insights

The items listed below may not capture all security relevant changes. Before providing a security review, be sure to review the entire PR for security impact.

(1) Security relevant changes were detected

Watched keyword InnerHtml in lib/src/component/error_boundary.dart line(s) ['100'] added

Action Items

Obtain a security review; reviewer should pay special attention to insights listed above
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

* master: lower dockerfile to 2.6 instead of 2.7 wdesk sdk hasnt upped its pubspec yet... remove dart 2.4 references address cr feedback fix tests for redux 7.1.3 Update CHANGELOG.md over_react_3.1.6 remove unneeded `if` commit updated generated file from redux Remove dead code & help dart2js compile perf Add comment Update test/over_react/component_declaration/builder_integration_tests/component2/required_accessor_integration_test.dart Apply suggestions from code review Update dependency validator Add front_end dependency Format a little Address CR feedback Refactor required accessor integration suite Refactor required accessor test suite

lib/src/component/error_boundary_recoverable.dart

lib/src/component/error_boundary_mixins.dart

lib/src/component/error_boundary_recoverable.dart

web/src/demos/faulty-on-mount-component.dart

lib/src/component/error_boundary.dart

web/component2/index.html

kealjones-wk · 2019-12-16T23:08:00Z

test/over_react/component/shared_error_boundary_tests.dart

-          expect(getByTestId(jacket.getInstance(), 'fallbackNode'), isNotNull);
+          if (!isWrapper) {
+            // wrapper ErrorBoundary doesn't use `props.fallbackUIRenderer` because `ResolvableErrorBoundary`
+            // will display it on first error. Meaning wrapper ErrorBoundary will never be reached if it is set.


The child error boundary catching and displaying the fallback renderer is tested here:
https://github.com/Workiva/over_react/pull/435/files#diff-89d97c9289557c366adc6964193a6057R372-R388

test/over_react/component/shared_error_boundary_tests.dart

aaronlademann-wf · 2019-12-17T13:57:50Z

web/src/demos/faulty-on-mount-component.dart

+class FaultyOnMountComponent extends UiComponent2<FaultyOnMountProps> {
+  @override
+  render() {
+    throw Shade();


greglittlefield-wf

+1

Nothin but nits

lib/src/component/error_boundary.dart

greglittlefield-wf · 2019-12-17T17:27:56Z

lib/src/component/error_boundary_mixins.dart

@@ -13,6 +13,8 @@ const String defaultErrorBoundaryLoggerName = 'over_react.ErrorBoundary';
 /// within a custom component.
 ///
 /// > See: [ErrorBoundaryMixin] for a usage example.
+@Deprecated('Building custom error boundaries with this mixin will no longer be supported in version 4.0.0.'
+    'Use ErrorBoundary and its prop API to customize error handling instead.')


test/over_react/component/shared_error_boundary_tests.dart

Co-Authored-By: Greg Littlefield <[email protected]>

kealjones-wk · 2019-12-18T00:19:08Z

+1

Nothin but nits

swish!

aaronlademann-wf

+1

aaronlademann-wf

QA +1

Passing CI
Pulled these changes into a consumer app in which I updated a component that is a child of an ErrorBoundary to throw from within render.
- Without these changes, throwing from within render resulted in the entire app being unmounted.
- With these changes, only the problematic component was unmounted, and the rest of the application continued to work as expected.

Nice work on this @kealjones-wk !!!

evanweible-wf · 2019-12-18T16:06:37Z

Security +1

Watched term InnerHtml was flagged, but it's actually just a part of a test ID.

kealjones-wk · 2019-12-18T17:57:30Z

@Workiva/release-management-p

inital pass

a559b44

rmconsole-wf added the Merge Requirements Unmet label Dec 4, 2019

fixing up errorboundary tests

b80c85f

aviary3-wk added the Security Review Required label Dec 11, 2019

kealjones-wk added 4 commits December 12, 2019 12:37

Fix all the tests!!!!

5609a77

uncomment out tests

727ebc4

more test uncommenting

5ea09a8

kealjones-wk marked this pull request as ready for review December 12, 2019 22:43

aaronlademann-wf requested changes Dec 13, 2019

View reviewed changes

address cr feedback

d388adb

aviary3-wk removed the Security Review Required label Dec 13, 2019

remove accidental print

6ceea56

aviary2-wf added the Security Review Required label Dec 13, 2019

kealjones-wk added 2 commits December 16, 2019 16:03

cleanup fallbackUIRenderer logic and update tests

2ce7ec1

fix find replace mistake

bdacdd8

kealjones-wk commented Dec 16, 2019

View reviewed changes

aaronlademann-wf requested changes Dec 17, 2019

View reviewed changes

kealjones-wk added 2 commits December 17, 2019 10:25

address cr feedback

997de19

remove unused async

ef2e755

greglittlefield-wf approved these changes Dec 17, 2019

View reviewed changes

kealjones-wk and others added 2 commits December 17, 2019 10:53

add ErrorBoundaryApi mixin for local testing

3258d61

Apply suggestions from code review

885d948

Co-Authored-By: Greg Littlefield <[email protected]>

aaronlademann-wf approved these changes Dec 18, 2019

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Dec 18, 2019

rmconsole-wf added the RM Ready label Dec 18, 2019

rmconsole7-wk merged commit bde6993 into master Dec 18, 2019

rmconsole7-wk deleted the errorboundary_unrecoverable branch December 18, 2019 17:58

aaronlademann-wf mentioned this pull request Dec 18, 2019

RM-63295 Release over_react 3.1.7 #445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPLAT-8600: Unrecoverable ErrorBoundary Fix #435

CPLAT-8600: Unrecoverable ErrorBoundary Fix #435

kealjones-wk commented Dec 4, 2019 •

edited

Loading

aviary2-wf commented Dec 4, 2019 •

edited

Loading

kealjones-wk Dec 16, 2019

aaronlademann-wf Dec 17, 2019

greglittlefield-wf left a comment

greglittlefield-wf Dec 17, 2019

kealjones-wk commented Dec 18, 2019

aaronlademann-wf left a comment

aaronlademann-wf left a comment

evanweible-wf commented Dec 18, 2019

kealjones-wk commented Dec 18, 2019

CPLAT-8600: Unrecoverable ErrorBoundary Fix #435

CPLAT-8600: Unrecoverable ErrorBoundary Fix #435

Conversation

kealjones-wk commented Dec 4, 2019 • edited Loading

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

aviary2-wf commented Dec 4, 2019 • edited Loading

Security Insights

Action Items

kealjones-wk Dec 16, 2019

Choose a reason for hiding this comment

aaronlademann-wf Dec 17, 2019

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf Dec 17, 2019

Choose a reason for hiding this comment

kealjones-wk commented Dec 18, 2019

aaronlademann-wf left a comment

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

evanweible-wf commented Dec 18, 2019

kealjones-wk commented Dec 18, 2019

kealjones-wk commented Dec 4, 2019 •

edited

Loading

aviary2-wf commented Dec 4, 2019 •

edited

Loading