From 0e6546b93acbb8d7485c485b371bc8bfdd44817e Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 10 Sep 2020 05:54:34 -0700 Subject: [PATCH] Lazily build copyText and deserialize request bodies Summary: This diff tries to address T74467181, where Flipper (Electron) crash hard with a segfault. Although I failed to find the root issue, I noticed that this typically happened when switching to the network plugin after a long time. Switching the network plugin causes all rows to be rebuild, and during the process all request and response bodies are deserialized which can take very long and fully blocks Flipper making it totally unresponsive. This diff fixes that issue, by making sure the lately added lazy evaluation of copyText is used, and that requests aren't deserialized during row generation, which is only needed for full body search purposes, by making that lazily as well. While at it fixed D21929666 (which I never finished) as well, which is a Sandy prerequisite, since it coupled the generic search table to hardcoded networking concepts like request and response. "body" search, has now been rebranded "contents" search. That is generic and makes sense in both the network and qpl plugin. Probably will make these labelings better customisable when revisiting the grids in Sandy N.B. I thought the issue might maybe that during the blocking process the garbage collector can't run, and the process would OOM. But that doesn't seem the case (or at least not without some other factor contribution as well), as in a quick test I can trivally allocate 30 GB of memory (making the system swap), but still the process runs stably. So even with this diff I can't satisfactory explain the crash, but at least it avoids a common circumstance in which they happen, and it significantly improves the user experience. changelog: [Network] Fixed issue where Flipper would hang for seconds or even crash when opening the Network plugin Reviewed By: jknoxville Differential Revision: D23599338 fbshipit-source-id: 52164fe6997b879c91907e0afe42e08ca3f315b4 --- .../ui/components/searchable/Searchable.tsx | 36 +++++----- .../components/searchable/SearchableTable.tsx | 21 +++--- desktop/app/src/ui/components/table/types.tsx | 3 +- desktop/plugins/network/index.tsx | 65 ++++++++++--------- 4 files changed, 61 insertions(+), 64 deletions(-) diff --git a/desktop/app/src/ui/components/searchable/Searchable.tsx b/desktop/app/src/ui/components/searchable/Searchable.tsx index 987252328..868cb2184 100644 --- a/desktop/app/src/ui/components/searchable/Searchable.tsx +++ b/desktop/app/src/ui/components/searchable/Searchable.tsx @@ -108,9 +108,9 @@ export type SearchableProps = { searchTerm: string; filters: Array; allowRegexSearch?: boolean; - allowBodySearch?: boolean; + allowContentSearch?: boolean; regexEnabled?: boolean; - bodySearchEnabled?: boolean; + contentSearchEnabled?: boolean; }; type Props = { @@ -123,7 +123,7 @@ type Props = { clearSearchTerm: boolean; defaultSearchTerm: string; allowRegexSearch: boolean; - allowBodySearch: boolean; + allowContentSearch: boolean; }; type State = { @@ -132,7 +132,7 @@ type State = { searchTerm: string; hasFocus: boolean; regexEnabled: boolean; - bodySearchEnabled: boolean; + contentSearchEnabled: boolean; compiledRegex: RegExp | null | undefined; }; @@ -161,7 +161,7 @@ const Searchable = ( searchTerm: this.props.defaultSearchTerm ?? '', hasFocus: false, regexEnabled: false, - bodySearchEnabled: false, + contentSearchEnabled: false, compiledRegex: null, }; @@ -174,7 +174,7 @@ const Searchable = ( | { filters: Array; regexEnabled?: boolean; - bodySearchEnabled?: boolean; + contentSearchEnabled?: boolean; searchTerm?: string; } | undefined; @@ -220,8 +220,8 @@ const Searchable = ( searchTerm: searchTerm, filters: savedState.filters || this.state.filters, regexEnabled: savedState.regexEnabled || this.state.regexEnabled, - bodySearchEnabled: - savedState.bodySearchEnabled || this.state.bodySearchEnabled, + contentSearchEnabled: + savedState.contentSearchEnabled || this.state.contentSearchEnabled, compiledRegex: compileRegex(searchTerm), }); } @@ -232,7 +232,7 @@ const Searchable = ( this.getTableKey() && (prevState.searchTerm !== this.state.searchTerm || prevState.regexEnabled != this.state.regexEnabled || - prevState.bodySearchEnabled != this.state.bodySearchEnabled || + prevState.contentSearchEnabled != this.state.contentSearchEnabled || prevState.filters !== this.state.filters) ) { window.localStorage.setItem( @@ -241,7 +241,7 @@ const Searchable = ( searchTerm: this.state.searchTerm, filters: this.state.filters, regexEnabled: this.state.regexEnabled, - bodySearchEnabled: this.state.bodySearchEnabled, + contentSearchEnabled: this.state.contentSearchEnabled, }), ); if (this.props.onFilterChange != null) { @@ -449,9 +449,9 @@ const Searchable = ( }); }; - onBodySearchToggled = () => { + onContentSearchToggled = () => { this.setState({ - bodySearchEnabled: !this.state.bodySearchEnabled, + contentSearchEnabled: !this.state.contentSearchEnabled, }); }; @@ -519,13 +519,13 @@ const Searchable = ( label={'Regex'} /> ) : null} - {this.props.allowBodySearch ? ( + {this.props.allowContentSearch ? ( ) : null} @@ -537,7 +537,7 @@ const Searchable = ( addFilter={this.addFilter} searchTerm={this.state.searchTerm} regexEnabled={this.state.regexEnabled} - bodySearchEnabled={this.state.bodySearchEnabled} + contentSearchEnabled={this.state.contentSearchEnabled} filters={this.state.filters} /> diff --git a/desktop/app/src/ui/components/searchable/SearchableTable.tsx b/desktop/app/src/ui/components/searchable/SearchableTable.tsx index 60e34e414..f3c3f882e 100644 --- a/desktop/app/src/ui/components/searchable/SearchableTable.tsx +++ b/desktop/app/src/ui/components/searchable/SearchableTable.tsx @@ -68,7 +68,7 @@ function rowMatchesRegex(values: Array, regex: string): boolean { function rowMatchesSearchTerm( searchTerm: string, isRegex: boolean, - isBodySearchEnabled: boolean, + isContentSearchEnabled: boolean, row: TableBodyRow, ): boolean { if (searchTerm == null || searchTerm.length === 0) { @@ -77,13 +77,8 @@ function rowMatchesSearchTerm( const rowValues = Object.keys(row.columns).map((key) => textContent(row.columns[key].value), ); - if (isBodySearchEnabled) { - if (row.requestBody) { - rowValues.push(row.requestBody); - } - if (row.responseBody) { - rowValues.push(row.responseBody); - } + if (isContentSearchEnabled && typeof row.getSearchContent === 'function') { + rowValues.push(row.getSearchContent()); } if (row.filterValue != null) { rowValues.push(row.filterValue); @@ -100,10 +95,10 @@ const filterRowsFactory = ( filters: Array, searchTerm: string, regexSearch: boolean, - bodySearch: boolean, + contentSearch: boolean, ) => (row: TableBodyRow): boolean => rowMatchesFilters(filters, row) && - rowMatchesSearchTerm(searchTerm, regexSearch, bodySearch, row); + rowMatchesSearchTerm(searchTerm, regexSearch, contentSearch, row); class SearchableManagedTable extends PureComponent { static defaultProps = { @@ -115,7 +110,7 @@ class SearchableManagedTable extends PureComponent { this.props.filters, this.props.searchTerm, this.props.regexEnabled || false, - this.props.bodySearchEnabled || false, + this.props.contentSearchEnabled || false, ), }; @@ -127,7 +122,7 @@ class SearchableManagedTable extends PureComponent { if ( nextProps.searchTerm !== this.props.searchTerm || nextProps.regexEnabled != this.props.regexEnabled || - nextProps.bodySearchEnabled != this.props.bodySearchEnabled || + nextProps.contentSearchEnabled != this.props.contentSearchEnabled || !deepEqual(this.props.filters, nextProps.filters) ) { this.setState({ @@ -135,7 +130,7 @@ class SearchableManagedTable extends PureComponent { nextProps.filters, nextProps.searchTerm, nextProps.regexEnabled || false, - nextProps.bodySearchEnabled || false, + nextProps.contentSearchEnabled || false, ), }); } diff --git a/desktop/app/src/ui/components/table/types.tsx b/desktop/app/src/ui/components/table/types.tsx index 998eb455d..34c78f211 100644 --- a/desktop/app/src/ui/components/table/types.tsx +++ b/desktop/app/src/ui/components/table/types.tsx @@ -56,8 +56,7 @@ export type TableBodyRow = { zebraBackgroundColor?: BackgroundColorProperty | undefined; onDoubleClick?: (e: React.MouseEvent) => void; copyText?: string | (() => string); - requestBody?: string | null | undefined; - responseBody?: string | null | undefined; + getSearchContent?: () => string; highlightOnHover?: boolean; columns: { [key: string]: TableBodyColumn; diff --git a/desktop/plugins/network/index.tsx b/desktop/plugins/network/index.tsx index a39785d62..93481d3ef 100644 --- a/desktop/plugins/network/index.tsx +++ b/desktop/plugins/network/index.tsx @@ -609,39 +609,43 @@ function buildRow( const friendlyName = getHeaderValue(request.headers, 'X-FB-Friendly-Name'); const style = response && response.isMock ? mockingStyle : undefined; - let copyText = `# HTTP request for ${domain} (ID: ${request.id}) -## Request -HTTP ${request.method} ${request.url} -${request.headers - .map( - ({key, value}: {key: string; value: string}): string => - `${key}: ${String(value)}`, - ) - .join('\n')}`; + const copyText = () => { + let copyText = `# HTTP request for ${domain} (ID: ${request.id}) + ## Request + HTTP ${request.method} ${request.url} + ${request.headers + .map( + ({key, value}: {key: string; value: string}): string => + `${key}: ${String(value)}`, + ) + .join('\n')}`; - const requestData = request.data ? decodeBody(request) : null; - const responseData = response && response.data ? decodeBody(response) : null; + const requestData = request.data ? decodeBody(request) : null; + const responseData = + response && response.data ? decodeBody(response) : null; - if (requestData) { - copyText += `\n\n${requestData}`; - } + if (requestData) { + copyText += `\n\n${requestData}`; + } - if (response) { - copyText += ` + if (response) { + copyText += ` -## Response -HTTP ${response.status} ${response.reason} -${response.headers - .map( - ({key, value}: {key: string; value: string}): string => - `${key}: ${String(value)}`, - ) - .join('\n')}`; - } + ## Response + HTTP ${response.status} ${response.reason} + ${response.headers + .map( + ({key, value}: {key: string; value: string}): string => + `${key}: ${String(value)}`, + ) + .join('\n')}`; + } - if (responseData) { - copyText += `\n\n${responseData}`; - } + if (responseData) { + copyText += `\n\n${responseData}`; + } + return copyText; + }; return { columns: { @@ -684,10 +688,9 @@ ${response.headers filterValue: `${request.method} ${request.url}`, sortKey: request.timestamp, copyText, + getSearchContent: copyText, highlightOnHover: true, style: style, - requestBody: requestData, - responseBody: responseData, }; } @@ -815,7 +818,7 @@ class NetworkTable extends PureComponent { highlightedRows={this.props.highlightedRows} rowLineHeight={26} allowRegexSearch={true} - allowBodySearch={true} + allowContentSearch={true} zebra={false} clearSearchTerm={this.props.searchTerm !== ''} defaultSearchTerm={this.props.searchTerm}