CPLAT-7606 Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary #350

aaronlademann-wf · 2019-10-03T21:09:29Z

Motivation

While testing the recent 3.0.0-alpha.0 release in a consumer library, a problem was discovered.

Currently, our ErrorBoundary component - when props.fallbackUIRenderer is not specified - will attempt to remount its child component tree when an error is caught. In most cases, this is a sane default - however - if a component in that tree throws every time it mounts - this causes a huge problem, since it creates a sort of "infinite loop" of component throws in componentDidMount -> ErrorBoundary remounts the componnet -> component throws in componentDidMount -> .... This infinite loop has the potential to completely lock up the main CPU thread.

Changes

See code comments for much more in-depth information about what happens when / why

Add some internal component fields to ErrorBoundaryComponent that keep track of the error that was thrown, and the "component stack" from the info object made available from ReactJS when a component wrapped in an error boundary throws.
Use these new fields when a second error is thrown to evaluate whether the second error is identical to the first - and whether it was thrown from the exact same place in the tree.
If it is identical - and the errors happen within 5 seconds of each other, render a non-interactive String representation of the DOM that existed before the error was thrown.
Allow consumers to get ErrorBoundary out of error states by either passing in a new child, or by calling the new reset instance method.
Add lots of tests and docs.

Release Notes

Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary component.

Review

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

Please review: @greglittlefield-wf @kealjones-wk @joebingham-wk @sydneyjodon-wk

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

aviary-wf · 2019-10-03T21:09:53Z

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.

(3) Security relevant changes were detected

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

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

Watched keyword dangerouslySetInnerHTML in lib/src/component/error_boundary.dart line(s) ['348'] 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.

maxwellpeterson-wf · 2019-10-04T14:53:15Z

+1 security

Our CSP protects us from any potential XSS that this allows.

joebingham-wk · 2019-10-07T21:03:44Z

lib/src/component/error_boundary.dart

+    return (Dom.div()
+      ..key = 'ohnoes'
+      ..addTestId('ErrorBoundary.unrecoverableErrorInnerHtmlContainerNode')
+      ..['dangerouslySetInnerHTML'] = {'__html': _domAtTimeOfError} ?? ''


What are the cases when the null check will return ''? Because the Map is always set with a key, won't it always be non-null?

Is this supposed to be on _domAtTimeOfError?

Suggested change

..['dangerouslySetInnerHTML'] = {'__html': _domAtTimeOfError} ?? ''

..['dangerouslySetInnerHTML'] = {'__html': _domAtTimeOfError ?? ''}

Maybe we should add a test where the child component is a or renders a fragment, which would represent a case where _domAtTimeOfError would be null:

ErrorBoundary([ // Can't use variadic children or the fragment will be interpreted as two separate children // A list-style "fragment" as the single child. [ (Dom.div()..key = 1)(), (FaultyComponent()..key = 2)(), ], ])

Or actually, maybe even an empty component would do...

Nice catch on this typo @joebingham-wk !!!

Fixed the typo, added edge case test coverage in a9e864e

test/over_react/component/error_boundary_test.dart

greglittlefield-wf

Mostly nits and then a few questions/comments. Overall, looks really solid!

lib/src/component/error_boundary.dart

test/over_react/component/error_boundary_test.dart

lib/src/component/error_boundary.dart

+ and add more test coverage for edge cases involving `dangerouslySetInnerHTML`

joebingham-wk

+1

greglittlefield-wf · 2019-10-08T17:18:49Z

test/over_react/component/error_boundary_test.dart

-          expect(() => component.setState(component.newState()..hasError = true), throws);
+          expect(getByTestId(jacket.getInstance(), 'dummyChild'), isNull,
+              reason: 'The child component tree should have been removed from the dom');
+          expect(jacket.getNode(), hasAttr(defaultTestIdKey, 'ErrorBoundary.unrecoverableErrorInnerHtmlContainerNode'));


Can we assert here that the static HTML got rendered? I'm not seeing that tested anywhere

It is tested in the "gracefully handles errors in its tree when props.fallbackUIRenderer is not set" section:

https://github.com/Workiva/over_react/pull/350/files#diff-820fada62e348ea7fccc19d5cc72d0f6R342

Ah, sorry, I missed that. Thanks!

greglittlefield-wf

+1

greglittlefield-wf · 2019-10-08T17:39:11Z

Works well in WVC and over_react example

QA +1

@Workiva/release-management-p

maxwellpeterson-wf · 2019-10-08T17:53:47Z

+1 security refresh

Improve handling of “repeat” errors that can lock up the main thread

de99fd1

aaronlademann-wf added this to the 3.0.0 milestone Oct 3, 2019

rmconsole-wf added the Merge Requirements Unmet label Oct 3, 2019

aviary2-wf added the Security Review Required label Oct 3, 2019

This was referenced Oct 3, 2019

CPLAT-7607 [Dart 1] Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary #352

Merged

CPLAT-7608 [3.1.0] Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary #353

Merged

joebingham-wk reviewed Oct 7, 2019

View reviewed changes

greglittlefield-wf requested changes Oct 7, 2019

View reviewed changes

aaronlademann-wf added 6 commits October 8, 2019 08:26

Use a managed timer

078c84f

Address documentation CR feedback

a78f71f

Reset internal error tracking immediately

1998d8d

Move var closer to usage

f6b1f43

Fix null aware assignment typo

a9e864e

+ and add more test coverage for edge cases involving `dangerouslySetInnerHTML`

Improve readability of large function body

320c15f

joebingham-wk approved these changes Oct 8, 2019

View reviewed changes

greglittlefield-wf reviewed Oct 8, 2019

View reviewed changes

greglittlefield-wf approved these changes Oct 8, 2019

View reviewed changes

rmconsole-wf added the RM Ready label Oct 8, 2019

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Oct 8, 2019

rmconsole7-wk merged commit 0d21705 into master Oct 8, 2019

rmconsole7-wk deleted the 3.0.0/error-boundary-improvements branch October 8, 2019 17:54

aaronlademann-wf mentioned this pull request Oct 8, 2019

RM-58745 Release over_react 3.0.0-alpha.1+dart2 #359

Merged

1 task

aaronlademann-wf added the Type: enhancement label Nov 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPLAT-7606 Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary #350

CPLAT-7606 Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary #350

aaronlademann-wf commented Oct 3, 2019

aviary-wf commented Oct 3, 2019 •

edited by aviary3-wk

maxwellpeterson-wf commented Oct 4, 2019

joebingham-wk Oct 7, 2019

greglittlefield-wf Oct 7, 2019

aaronlademann-wf Oct 8, 2019

aaronlademann-wf Oct 8, 2019

greglittlefield-wf left a comment

joebingham-wk left a comment

greglittlefield-wf Oct 8, 2019

aaronlademann-wf Oct 8, 2019

greglittlefield-wf Oct 8, 2019

greglittlefield-wf left a comment

greglittlefield-wf commented Oct 8, 2019

maxwellpeterson-wf commented Oct 8, 2019

	..['dangerouslySetInnerHTML'] = {'__html': _domAtTimeOfError} ?? ''
	..['dangerouslySetInnerHTML'] = {'__html': _domAtTimeOfError ?? ''}

CPLAT-7606 Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary #350

CPLAT-7606 Improve handling of “repeat” errors thrown from components wrapped by an ErrorBoundary #350

Conversation

aaronlademann-wf commented Oct 3, 2019

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

aviary-wf commented Oct 3, 2019 • edited by aviary3-wk

Security Insights

Action Items

maxwellpeterson-wf commented Oct 4, 2019

joebingham-wk Oct 7, 2019

Choose a reason for hiding this comment

greglittlefield-wf Oct 7, 2019

Choose a reason for hiding this comment

aaronlademann-wf Oct 8, 2019

Choose a reason for hiding this comment

aaronlademann-wf Oct 8, 2019

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

joebingham-wk left a comment

Choose a reason for hiding this comment

greglittlefield-wf Oct 8, 2019

Choose a reason for hiding this comment

aaronlademann-wf Oct 8, 2019

Choose a reason for hiding this comment

greglittlefield-wf Oct 8, 2019

Choose a reason for hiding this comment

greglittlefield-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Oct 8, 2019

maxwellpeterson-wf commented Oct 8, 2019

aviary-wf commented Oct 3, 2019 •

edited by aviary3-wk