From 79f6f34a627f3691a8b733e6c19f634dfd621eec Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 20 Oct 2021 10:04:54 -0400 Subject: [PATCH] Refactored code so that isInternalModule function could be unit tested. --- .../src/content-views/FlamechartView.js | 56 +-------- .../utils/__tests__/__modules__/module-one.js | 16 +++ .../utils/__tests__/__modules__/module-two.js | 18 +++ .../utils/__tests__/moduleFilters-test.js | 112 ++++++++++++++++++ .../src/content-views/utils/moduleFilters.js | 69 +++++++++++ .../src/backend/types.js | 2 +- packages/react-devtools-shared/src/hook.js | 34 ++++-- .../src/SchedulingProfiler.js | 13 +- 8 files changed, 245 insertions(+), 75 deletions(-) create mode 100644 packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-one.js create mode 100644 packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-two.js create mode 100644 packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/moduleFilters-test.js create mode 100644 packages/react-devtools-scheduling-profiler/src/content-views/utils/moduleFilters.js diff --git a/packages/react-devtools-scheduling-profiler/src/content-views/FlamechartView.js b/packages/react-devtools-scheduling-profiler/src/content-views/FlamechartView.js index e1fdf8f4327f4..bbc8dcf13936b 100644 --- a/packages/react-devtools-scheduling-profiler/src/content-views/FlamechartView.js +++ b/packages/react-devtools-scheduling-profiler/src/content-views/FlamechartView.js @@ -21,11 +21,6 @@ import type { ViewRefs, } from '../view-base'; -import { - CHROME_WEBSTORE_EXTENSION_ID, - INTERNAL_EXTENSION_ID, - LOCAL_EXTENSION_ID, -} from 'react-devtools-shared/src/constants'; import { BackgroundColorView, Surface, @@ -36,6 +31,7 @@ import { rectIntersectsRect, verticallyStackedLayout, } from '../view-base'; +import {isInternalModule} from './utils/moduleFilters'; import { durationToWidth, positioningScaleFactor, @@ -75,56 +71,6 @@ function hoverColorForStackFrame(stackFrame: FlamechartStackFrame): string { return hslaColorToString(color); } -function isInternalModule( - internalModuleSourceToRanges: InternalModuleSourceToRanges, - flamechartStackFrame: FlamechartStackFrame, -): boolean { - const {locationColumn, locationLine, scriptUrl} = flamechartStackFrame; - - if (scriptUrl == null || locationColumn == null || locationLine == null) { - return true; - } - - // Internal modules are only registered if DevTools was running when the profile was captured, - // but DevTools should also hide its own frames to avoid over-emphasizing them. - if ( - // Handle webpack-internal:// sources - scriptUrl.includes('/react-devtools') || - scriptUrl.includes('/react_devtools') || - // Filter out known extension IDs - scriptUrl.includes(CHROME_WEBSTORE_EXTENSION_ID) || - scriptUrl.includes(INTERNAL_EXTENSION_ID) || - scriptUrl.includes(LOCAL_EXTENSION_ID) - - // Unfortunately this won't get everything, like relatively loaded chunks or Web Worker files. - ) { - return true; - } - - // Filter out React internal packages. - const ranges = internalModuleSourceToRanges.get(scriptUrl); - if (ranges != null) { - for (let i = 0; i < ranges.length; i++) { - const [startStackFrame, stopStackFrame] = ranges[i]; - - const isAfterStart = - locationLine > startStackFrame.lineNumber || - (locationLine === startStackFrame.lineNumber && - locationColumn >= startStackFrame.columnNumber); - const isBeforeStop = - locationLine < stopStackFrame.lineNumber || - (locationLine === stopStackFrame.lineNumber && - locationColumn <= stopStackFrame.columnNumber); - - if (isAfterStart && isBeforeStop) { - return true; - } - } - } - - return false; -} - class FlamechartStackLayerView extends View { /** Layer to display */ _stackLayer: FlamechartStackLayer; diff --git a/packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-one.js b/packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-one.js new file mode 100644 index 0000000000000..6c3ace6bfbc65 --- /dev/null +++ b/packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-one.js @@ -0,0 +1,16 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +export const outerErrorA = new Error(); + +export const moduleStartError = new Error(); +export const innerError = new Error(); +export const moduleStopError = new Error(); + +export const outerErrorB = new Error(); diff --git a/packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-two.js b/packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-two.js new file mode 100644 index 0000000000000..994f317900555 --- /dev/null +++ b/packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/__modules__/module-two.js @@ -0,0 +1,18 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +export const moduleAStartError = new Error(); +export const innerErrorA = new Error(); +export const moduleAStopError = new Error(); + +export const outerError = new Error(); + +export const moduleBStartError = new Error(); +export const innerErrorB = new Error(); +export const moduleBStopError = new Error(); diff --git a/packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/moduleFilters-test.js b/packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/moduleFilters-test.js new file mode 100644 index 0000000000000..c778e71256f96 --- /dev/null +++ b/packages/react-devtools-scheduling-profiler/src/content-views/utils/__tests__/moduleFilters-test.js @@ -0,0 +1,112 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import ErrorStackParser from 'error-stack-parser'; +import {isInternalModule} from '../moduleFilters'; + +import * as moduleOne from './__modules__/module-one'; +import * as moduleTwo from './__modules__/module-two'; + +describe('isInternalModule', () => { + let moduleSourceToRanges; + + function parseAndGetStackFrame(error, index = 0) { + const stackFrame = ErrorStackParser.parse(error)[index]; + + // Strip out the portion of the file path that includes "react-devtools-scheduling-profiler" + // or every frame will be flagged as internal. + stackFrame.fileName = stackFrame.fileName.replace( + /.+__tests__/, + '../__tests__', + ); + + return stackFrame; + } + + function stackFrameToFlamechartStackFrame(stackFrame) { + return { + name: 'test', + timestamp: 0, + duration: 1, + scriptUrl: stackFrame.fileName, + locationLine: stackFrame.lineNumber, + locationColumn: stackFrame.columnNumber, + }; + } + + beforeEach(() => { + moduleSourceToRanges = new Map(); + + let startErrorStackFrame = parseAndGetStackFrame( + moduleOne.moduleStartError, + ); + let stopErrorStackFrame = parseAndGetStackFrame(moduleOne.moduleStopError); + moduleSourceToRanges.set(startErrorStackFrame.fileName, [ + [startErrorStackFrame, stopErrorStackFrame], + ]); + + const moduleTwoRanges = []; + + startErrorStackFrame = parseAndGetStackFrame(moduleTwo.moduleAStartError); + stopErrorStackFrame = parseAndGetStackFrame(moduleTwo.moduleAStopError); + moduleTwoRanges.push([startErrorStackFrame, stopErrorStackFrame]); + + startErrorStackFrame = parseAndGetStackFrame(moduleTwo.moduleBStartError); + stopErrorStackFrame = parseAndGetStackFrame(moduleTwo.moduleBStopError); + moduleTwoRanges.push([startErrorStackFrame, stopErrorStackFrame]); + + moduleSourceToRanges.set(startErrorStackFrame.fileName, moduleTwoRanges); + }); + + it('should properly identify stack frames within the provided module ranges', () => { + let flamechartStackFrame = stackFrameToFlamechartStackFrame( + parseAndGetStackFrame(moduleOne.innerError), + ); + expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe( + true, + ); + + flamechartStackFrame = stackFrameToFlamechartStackFrame( + parseAndGetStackFrame(moduleTwo.innerErrorA), + ); + expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe( + true, + ); + + flamechartStackFrame = stackFrameToFlamechartStackFrame( + parseAndGetStackFrame(moduleTwo.innerErrorB), + ); + expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe( + true, + ); + }); + + it('should properly identify stack frames outside of the provided module ranges', () => { + let flamechartStackFrame = stackFrameToFlamechartStackFrame( + parseAndGetStackFrame(moduleOne.outerErrorA), + ); + expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe( + false, + ); + + flamechartStackFrame = stackFrameToFlamechartStackFrame( + parseAndGetStackFrame(moduleOne.outerErrorB), + ); + expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe( + false, + ); + + flamechartStackFrame = stackFrameToFlamechartStackFrame( + parseAndGetStackFrame(moduleTwo.outerError), + ); + expect(isInternalModule(moduleSourceToRanges, flamechartStackFrame)).toBe( + false, + ); + }); +}); diff --git a/packages/react-devtools-scheduling-profiler/src/content-views/utils/moduleFilters.js b/packages/react-devtools-scheduling-profiler/src/content-views/utils/moduleFilters.js new file mode 100644 index 0000000000000..7030e6124b603 --- /dev/null +++ b/packages/react-devtools-scheduling-profiler/src/content-views/utils/moduleFilters.js @@ -0,0 +1,69 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import type { + FlamechartStackFrame, + InternalModuleSourceToRanges, +} from '../../types'; + +import { + CHROME_WEBSTORE_EXTENSION_ID, + INTERNAL_EXTENSION_ID, + LOCAL_EXTENSION_ID, +} from 'react-devtools-shared/src/constants'; + +export function isInternalModule( + internalModuleSourceToRanges: InternalModuleSourceToRanges, + flamechartStackFrame: FlamechartStackFrame, +): boolean { + const {locationColumn, locationLine, scriptUrl} = flamechartStackFrame; + + if (scriptUrl == null || locationColumn == null || locationLine == null) { + // This could indicate a browser-internal API like performance.mark(). + return false; + } + + // Internal modules are only registered if DevTools was running when the profile was captured, + // but DevTools should also hide its own frames to avoid over-emphasizing them. + if ( + // Handle webpack-internal:// sources + scriptUrl.includes('/react-devtools') || + scriptUrl.includes('/react_devtools') || + // Filter out known extension IDs + scriptUrl.includes(CHROME_WEBSTORE_EXTENSION_ID) || + scriptUrl.includes(INTERNAL_EXTENSION_ID) || + scriptUrl.includes(LOCAL_EXTENSION_ID) + // Unfortunately this won't get everything, like relatively loaded chunks or Web Worker files. + ) { + return true; + } + + // Filter out React internal packages. + const ranges = internalModuleSourceToRanges.get(scriptUrl); + if (ranges != null) { + for (let i = 0; i < ranges.length; i++) { + const [startStackFrame, stopStackFrame] = ranges[i]; + + const isAfterStart = + locationLine > startStackFrame.lineNumber || + (locationLine === startStackFrame.lineNumber && + locationColumn >= startStackFrame.columnNumber); + const isBeforeStop = + locationLine < stopStackFrame.lineNumber || + (locationLine === stopStackFrame.lineNumber && + locationColumn <= stopStackFrame.columnNumber); + + if (isAfterStart && isBeforeStop) { + return true; + } + } + } + + return false; +} diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 4f92f6b5b012e..46e1287797981 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -421,7 +421,7 @@ export type DevToolsHook = { ) => void, // Scheduling Profiler internal module filtering - getInternalModuleRanges: () => Array<[Error, Error]>, + getInternalModuleRanges: () => Array<[string, string]>, registerInternalModuleStart: (moduleStartError: Error) => void, registerInternalModuleStop: (moduleStopError: Error) => void, diff --git a/packages/react-devtools-shared/src/hook.js b/packages/react-devtools-shared/src/hook.js index 94ebe42a06046..e265212e31c42 100644 --- a/packages/react-devtools-shared/src/hook.js +++ b/packages/react-devtools-shared/src/hook.js @@ -490,21 +490,37 @@ export function installHook(target: any): DevToolsHook | null { } } - const openModuleRangesStack: Array = []; - const moduleRanges: Array<[Error, Error]> = []; + type StackFrameString = string; - function getInternalModuleRanges(): Array<[Error, Error]> { + const openModuleRangesStack: Array = []; + const moduleRanges: Array<[StackFrameString, StackFrameString]> = []; + + function getTopStackFrameString(error: Error): StackFrameString | null { + const frames = error.stack.split('\n'); + const frame = frames.length > 1 ? frames[1] : null; + return frame; + } + + function getInternalModuleRanges(): Array< + [StackFrameString, StackFrameString], + > { return moduleRanges; } - function registerInternalModuleStart(moduleStartError: Error) { - openModuleRangesStack.push(moduleStartError); + function registerInternalModuleStart(error: Error) { + const startStackFrame = getTopStackFrameString(error); + if (startStackFrame !== null) { + openModuleRangesStack.push(startStackFrame); + } } - function registerInternalModuleStop(moduleStopError: Error) { - const moduleStartError = openModuleRangesStack.pop(); - if (moduleStartError) { - moduleRanges.push([moduleStartError, moduleStopError]); + function registerInternalModuleStop(error: Error) { + if (openModuleRangesStack.length > 0) { + const startStackFrame = openModuleRangesStack.pop(); + const stopStackFrame = getTopStackFrameString(error); + if (stopStackFrame !== null) { + moduleRanges.push([startStackFrame, stopStackFrame]); + } } } diff --git a/packages/react-reconciler/src/SchedulingProfiler.js b/packages/react-reconciler/src/SchedulingProfiler.js index 6afaa8bab8ef6..4df55137fda97 100644 --- a/packages/react-reconciler/src/SchedulingProfiler.js +++ b/packages/react-reconciler/src/SchedulingProfiler.js @@ -106,17 +106,10 @@ function markInternalModuleRanges() { ) { const ranges = __REACT_DEVTOOLS_GLOBAL_HOOK__.getInternalModuleRanges(); for (let i = 0; i < ranges.length; i++) { - const [startError, stopError] = ranges[i]; + const [startStackFrame, stopStackFrame] = ranges[i]; - // Don't embed Error stack parsing logic into the reconciler. - // Just serialize the top stack frame and let the profiler parse it. - const startFrames = startError.stack.split('\n'); - const startFrame = startFrames.length > 1 ? startFrames[1] : ''; - const stopFrames = stopError.stack.split('\n'); - const stopFrame = stopFrames.length > 1 ? stopFrames[1] : ''; - - markAndClear(`--react-internal-module-start-${startFrame}`); - markAndClear(`--react-internal-module-stop-${stopFrame}`); + markAndClear(`--react-internal-module-start-${startStackFrame}`); + markAndClear(`--react-internal-module-stop-${stopStackFrame}`); } } }