From 2c14f3e88e00a99ad81ba74eecbcda2afc8d8d94 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 1 Apr 2019 07:48:04 -0700 Subject: [PATCH 1/4] Inject early on when reloading-and-profiling --- shells/browser/chrome/manifest.json | 7 +++- shells/browser/firefox/manifest.json | 7 +++- shells/browser/shared/src/GlobalHook.js | 50 ++++++++++++++++++------- shells/browser/shared/src/renderer.js | 21 +++++++++++ shells/browser/shared/webpack.config.js | 1 + src/backend/agent.js | 4 +- src/backend/index.js | 12 +++++- src/backend/renderer.js | 9 +++++ src/constants.js | 2 + src/hook.js | 14 +++++++ 10 files changed, 107 insertions(+), 20 deletions(-) create mode 100644 shells/browser/shared/src/renderer.js diff --git a/shells/browser/chrome/manifest.json b/shells/browser/chrome/manifest.json index 2458a4a0f5..6a26c7b21a 100644 --- a/shells/browser/chrome/manifest.json +++ b/shells/browser/chrome/manifest.json @@ -27,7 +27,12 @@ "devtools_page": "main.html", "content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'", - "web_accessible_resources": ["main.html", "panel.html", "build/backend.js"], + "web_accessible_resources": [ + "main.html", + "panel.html", + "build/backend.js", + "build/renderer.js" + ], "background": { "scripts": ["build/background.js"], diff --git a/shells/browser/firefox/manifest.json b/shells/browser/firefox/manifest.json index 0cff52c594..7f6a1281b5 100644 --- a/shells/browser/firefox/manifest.json +++ b/shells/browser/firefox/manifest.json @@ -33,7 +33,12 @@ "devtools_page": "main.html", "content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self'", - "web_accessible_resources": ["main.html", "panel.html", "build/backend.js"], + "web_accessible_resources": [ + "main.html", + "panel.html", + "build/backend.js", + "build/renderer.js" + ], "background": { "scripts": ["build/background.js"], diff --git a/shells/browser/shared/src/GlobalHook.js b/shells/browser/shared/src/GlobalHook.js index 9ccab3bb8a..074b289db6 100644 --- a/shells/browser/shared/src/GlobalHook.js +++ b/shells/browser/shared/src/GlobalHook.js @@ -2,13 +2,24 @@ import nullthrows from 'nullthrows'; import { installHook } from 'src/hook'; +import { RELOAD_AND_PROFILE_KEY } from 'src/constants'; + +function injectCode(code) { + const script = document.createElement('script'); + script.textContent = code; + + // This script runs before the element is created, + // so we add the script to instead. + nullthrows(document.documentElement).appendChild(script); + nullthrows(script.parentNode).removeChild(script); +} let lastDetectionResult; -// We want to detect when a renderer attaches, and notify the "background -// page" (which is shared between tabs and can highlight the React icon). -// Currently we are in "content script" context, so we can't listen -// to the hook directly (it will be injected directly into the page). +// We want to detect when a renderer attaches, and notify the "background page" +// (which is shared between tabs and can highlight the React icon). +// Currently we are in "content script" context, so we can't listen to the hook directly +// (it will be injected directly into the page). // So instead, the hook will use postMessage() to pass message to us here. // And when this happens, we'll send a message to the "background page". window.addEventListener('message', function(evt) { @@ -51,14 +62,27 @@ window.__REACT_DEVTOOLS_GLOBAL_HOOK__.nativeWeakMap = WeakMap; window.__REACT_DEVTOOLS_GLOBAL_HOOK__.nativeSet = Set; `; +// If we have just reloaded to profile, we need to inject the renderer interface before the app loads. +if (localStorage.getItem(RELOAD_AND_PROFILE_KEY) === 'true') { + const rendererURL = chrome.runtime.getURL('build/renderer.js'); + let rendererCode; + + // We need to inject in time to catch the initial mount. + // This means we need to synchronously read the renderer code itself, + // and synchronously inject it into the page. + // There are very few ways to actually do this. + // This seems to be the best approach. + const request = new XMLHttpRequest(); + request.addEventListener('load', function() { + rendererCode = this.responseText; + }); + request.open('GET', rendererURL, false); + request.send(); + injectCode(rendererCode); +} + // Inject a `__REACT_DEVTOOLS_GLOBAL_HOOK__` global so that React can detect that the // devtools are installed (and skip its suggestion to install the devtools). -const js = - ';(' + installHook.toString() + '(window))' + saveNativeValues + detectReact; - -// This script runs before the element is created, so we add the script -// to instead. -const script = document.createElement('script'); -script.textContent = js; -nullthrows(document.documentElement).appendChild(script); -nullthrows(script.parentNode).removeChild(script); +injectCode( + ';(' + installHook.toString() + '(window))' + saveNativeValues + detectReact +); diff --git a/shells/browser/shared/src/renderer.js b/shells/browser/shared/src/renderer.js new file mode 100644 index 0000000000..5fcc38a8ca --- /dev/null +++ b/shells/browser/shared/src/renderer.js @@ -0,0 +1,21 @@ +/** + * Install the hook on window, which is an event emitter. + * Note because Chrome content scripts cannot directly modify the window object, + * we are evaling this function by inserting a script tag. + * That's why we have to inline the whole event emitter implementation here. + * + * @flow + */ + +import { attach } from 'src/backend/renderer'; + +Object.defineProperty( + window, + '__REACT_DEVTOOLS_ATTACH__', + ({ + enumerable: false, + get() { + return attach; + }, + }: Object) +); diff --git a/shells/browser/shared/webpack.config.js b/shells/browser/shared/webpack.config.js index 5c11458ac7..b33c9248dd 100644 --- a/shells/browser/shared/webpack.config.js +++ b/shells/browser/shared/webpack.config.js @@ -18,6 +18,7 @@ module.exports = { inject: './src/GlobalHook.js', main: './src/main.js', panel: './src/panel.js', + renderer: './src/renderer.js', }, output: { path: __dirname + '/build', diff --git a/src/backend/agent.js b/src/backend/agent.js index ef363dbaae..dab48def13 100644 --- a/src/backend/agent.js +++ b/src/backend/agent.js @@ -1,7 +1,7 @@ // @flow import EventEmitter from 'events'; -import { __DEBUG__ } from '../constants'; +import { RELOAD_AND_PROFILE_KEY, __DEBUG__ } from '../constants'; import { hideOverlay, showOverlay } from './views/Highlighter'; import type { RendererID, RendererInterface } from './types'; @@ -38,8 +38,6 @@ type SetInParams = {| value: any, |}; -const RELOAD_AND_PROFILE_KEY = 'React::DevTools::reloadAndProfile'; - export default class Agent extends EventEmitter { _bridge: Bridge = ((null: any): Bridge); _isProfiling: boolean = false; diff --git a/src/backend/index.js b/src/backend/index.js index 4b08474056..a45c843c13 100644 --- a/src/backend/index.js +++ b/src/backend/index.js @@ -33,8 +33,16 @@ export function initBackend( ]; const attachRenderer = (id: number, renderer: ReactRenderer) => { - const rendererInterface = attach(hook, id, renderer, global); - hook.rendererInterfaces.set(id, rendererInterface); + let rendererInterface = hook.rendererInterfaces.get(id); + + // Inject any not-yet-injected renderers (if we didn't reload-and-profile) + if (!rendererInterface) { + rendererInterface = attach(hook, id, renderer, global); + + hook.rendererInterfaces.set(id, rendererInterface); + } + + // Notify the DevTools frontend about any renderers that were attached early. hook.emit('renderer-attached', { id, renderer, diff --git a/src/backend/renderer.js b/src/backend/renderer.js index 6a43c969ef..f0a3db897a 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -1595,6 +1595,10 @@ export function attach( } function startProfiling() { + if (isProfiling) { + return; + } + // Capture initial values as of the time profiling starts. // It's important we snapshot both the durations and the id-to-root map, // since either of these may change during the profiling session @@ -1611,6 +1615,11 @@ export function attach( isProfiling = false; } + // Automatically start profiling so that we don't miss timing info from initial "mount". + if (localStorage.getItem('React::DevTools::reloadAndProfile') === 'true') { + startProfiling(); + } + return { cleanup, getCommitDetails, diff --git a/src/constants.js b/src/constants.js index 1800fa8001..3523aa5214 100644 --- a/src/constants.js +++ b/src/constants.js @@ -5,4 +5,6 @@ export const TREE_OPERATION_REMOVE = 2; export const TREE_OPERATION_RESET_CHILDREN = 3; export const TREE_OPERATION_UPDATE_TREE_BASE_DURATION = 4; +export const RELOAD_AND_PROFILE_KEY = 'React::DevTools::reloadAndProfile'; + export const __DEBUG__ = false; diff --git a/src/hook.js b/src/hook.js index e5d66c40ac..5eaf9b63fd 100644 --- a/src/hook.js +++ b/src/hook.js @@ -79,6 +79,20 @@ export function installHook(target: any): DevToolsHook | null { hook.emit('renderer', { id, renderer, reactBuildType }); + // If we have just reloaded to profile, we need to inject the renderer interface before the app loads. + // Otherwise the renderer won't yet exist and we can skip this step. + const attach = target.__REACT_DEVTOOLS_ATTACH__; + if (typeof attach === 'function') { + const rendererInterface = attach(hook, id, renderer, target); + hook.rendererInterfaces.set(id, rendererInterface); + + /*hook.emit('renderer-attached', { + id, + renderer, + rendererInterface, + });*/ + } + return id; } From 74cd1a5d29dc670788ac2955495150f57d911db7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 1 Apr 2019 09:05:40 -0700 Subject: [PATCH 2/4] Misc cleanup of comments and localStorage key names --- shells/browser/shared/src/renderer.js | 9 +++++---- src/backend/index.js | 3 ++- src/backend/renderer.js | 3 ++- src/devtools/views/Profiler/ProfilerContext.js | 2 +- src/devtools/views/Settings/SettingsContext.js | 7 +++++-- src/hook.js | 10 ++-------- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/shells/browser/shared/src/renderer.js b/shells/browser/shared/src/renderer.js index 5fcc38a8ca..c4cc4c6f55 100644 --- a/shells/browser/shared/src/renderer.js +++ b/shells/browser/shared/src/renderer.js @@ -1,8 +1,9 @@ /** - * Install the hook on window, which is an event emitter. - * Note because Chrome content scripts cannot directly modify the window object, - * we are evaling this function by inserting a script tag. - * That's why we have to inline the whole event emitter implementation here. + * In order to support reload-and-profile functionality, the renderer needs to be injected before any other scripts. + * Since it is a complex file (with imports) we can't just toString() it like we do with the hook itself, + * So this entry point (one of the web_accessible_resources) provcides a way to eagerly inject it. + * The hook will look for the presence of a global __REACT_DEVTOOLS_ATTACH__ and attach an injected renderer early. + * The normal case (not a reload-and-profile) will not make use of this entry point though. * * @flow */ diff --git a/src/backend/index.js b/src/backend/index.js index a45c843c13..efa6e83489 100644 --- a/src/backend/index.js +++ b/src/backend/index.js @@ -42,7 +42,8 @@ export function initBackend( hook.rendererInterfaces.set(id, rendererInterface); } - // Notify the DevTools frontend about any renderers that were attached early. + // Notify the DevTools frontend about new renderers. + // This includes any that were attached early (via __REACT_DEVTOOLS_ATTACH__). hook.emit('renderer-attached', { id, renderer, diff --git a/src/backend/renderer.js b/src/backend/renderer.js index f0a3db897a..8f9d866a41 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -18,6 +18,7 @@ import { getDisplayName, utfEncodeString } from '../utils'; import { cleanForBridge, copyWithSet, setInObject } from './utils'; import { __DEBUG__, + RELOAD_AND_PROFILE_KEY, TREE_OPERATION_ADD, TREE_OPERATION_REMOVE, TREE_OPERATION_RESET_CHILDREN, @@ -1616,7 +1617,7 @@ export function attach( } // Automatically start profiling so that we don't miss timing info from initial "mount". - if (localStorage.getItem('React::DevTools::reloadAndProfile') === 'true') { + if (localStorage.getItem(RELOAD_AND_PROFILE_KEY) === 'true') { startProfiling(); } diff --git a/src/devtools/views/Profiler/ProfilerContext.js b/src/devtools/views/Profiler/ProfilerContext.js index 123386e156..b50cd12f98 100644 --- a/src/devtools/views/Profiler/ProfilerContext.js +++ b/src/devtools/views/Profiler/ProfilerContext.js @@ -130,7 +130,7 @@ function ProfilerContextController({ children }: Props) { const [ isCommitFilterEnabled, setIsCommitFilterEnabled, - ] = useLocalStorage('isCommitFilterEnabled', false); + ] = useLocalStorage('React::DevTools::isCommitFilterEnabled', false); const [minCommitDuration, setMinCommitDuration] = useLocalStorage( 'minCommitDuration', 0 diff --git a/src/devtools/views/Settings/SettingsContext.js b/src/devtools/views/Settings/SettingsContext.js index 4e51b5554e..c47fc224b4 100644 --- a/src/devtools/views/Settings/SettingsContext.js +++ b/src/devtools/views/Settings/SettingsContext.js @@ -41,10 +41,13 @@ function SettingsContextController({ settingsPortalContainer, }: Props) { const [displayDensity, setDisplayDensity] = useLocalStorage( - 'displayDensity', + 'React::DevTools::displayDensity', 'compact' ); - const [theme, setTheme] = useLocalStorage('theme', 'auto'); + const [theme, setTheme] = useLocalStorage( + 'React::DevTools::theme', + 'auto' + ); const documentElements = useMemo(() => { const array: Array = [ diff --git a/src/hook.js b/src/hook.js index 5eaf9b63fd..fa960d73be 100644 --- a/src/hook.js +++ b/src/hook.js @@ -77,22 +77,16 @@ export function installHook(target: any): DevToolsHook | null { ? 'deadcode' : detectReactBuildType(renderer); - hook.emit('renderer', { id, renderer, reactBuildType }); - // If we have just reloaded to profile, we need to inject the renderer interface before the app loads. // Otherwise the renderer won't yet exist and we can skip this step. const attach = target.__REACT_DEVTOOLS_ATTACH__; if (typeof attach === 'function') { const rendererInterface = attach(hook, id, renderer, target); hook.rendererInterfaces.set(id, rendererInterface); - - /*hook.emit('renderer-attached', { - id, - renderer, - rendererInterface, - });*/ } + hook.emit('renderer', { id, renderer, reactBuildType }); + return id; } From f5f7cb5bdf7822a65b772ccdb901aa678f645c45 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 1 Apr 2019 14:29:52 -0700 Subject: [PATCH 3/4] Fixed some missing operations that could happen after reload-and-profile --- src/backend/index.js | 5 +- src/backend/renderer.js | 75 ++++++++++++------- src/backend/types.js | 2 +- .../views/Profiler/CommitTreeBuilder.js | 7 +- .../views/Profiler/FlamegraphChartBuilder.js | 5 ++ .../views/Profiler/RankedChartBuilder.js | 4 + 6 files changed, 65 insertions(+), 33 deletions(-) diff --git a/src/backend/index.js b/src/backend/index.js index efa6e83489..298d06a121 100644 --- a/src/backend/index.js +++ b/src/backend/index.js @@ -23,7 +23,10 @@ export function initBackend( rendererInterface: RendererInterface, }) => { agent.setRendererInterface(id, rendererInterface); - rendererInterface.walkTree(); + + // Now that the Store and the renderer interface are connected, + // it's time to flush the pending operation codes to the frontend. + rendererInterface.flushInitialOperations(); } ), diff --git a/src/backend/renderer.js b/src/backend/renderer.js index 8f9d866a41..821060fc78 100644 --- a/src/backend/renderer.js +++ b/src/backend/renderer.js @@ -567,6 +567,7 @@ export function attach( } let pendingOperations: Uint32Array = new Uint32Array(0); + let pendingOperationsQueue: Array | null = []; function addOperation( newAction: Uint32Array, @@ -604,7 +605,16 @@ export function attach( // Let the frontend know about tree operations. // The first value in this array will identify which root it corresponds to, // so we do no longer need to dispatch a separate root-committed event. - hook.emit('operations', pendingOperations); + if (pendingOperationsQueue !== null) { + // Until the frontend has been connected, store the tree operations. + // This will let us avoid walking the tree later when the frontend connects, + // and it enables the Profiler's reload-and-profile functionality to work as well. + pendingOperationsQueue.push(pendingOperations); + } else { + // If we've already connected to the frontend, just pass the operations through. + hook.emit('operations', pendingOperations); + } + pendingOperations = new Uint32Array(0); } @@ -914,31 +924,46 @@ export function attach( // We don't patch any methods so there is no cleanup. } - function walkTree() { - // Hydrate all the roots for the first time. - hook.getFiberRoots(rendererID).forEach(root => { - currentRootID = getFiberID(getPrimaryFiber(root.current)); + function flushInitialOperations() { + const localPendingOperationsQueue = pendingOperationsQueue; - if (isProfiling) { - // If profiling is active, store commit time and duration, and the current interactions. - // The frontend may request this information after profiling has stopped. - currentCommitProfilingMetadata = { - actualDurations: [], - commitTime: performance.now() - profilingStartTime, - interactions: Array.from(root.memoizedInteractions).map( - (interaction: Interaction) => ({ - ...interaction, - timestamp: interaction.timestamp - profilingStartTime, - }) - ), - maxActualDuration: 0, - }; - } + pendingOperationsQueue = null; - mountFiber(root.current, null); - flushPendingEvents(root); - currentRootID = -1; - }); + if ( + localPendingOperationsQueue !== null && + localPendingOperationsQueue.length > 0 + ) { + // We may have already queued up some operations before the frontend connected + // If so, let the frontend know about them. + localPendingOperationsQueue.forEach(pendingOperations => { + hook.emit('operations', pendingOperations); + }); + } else { + // If we have not been profiling, then we can just walk the tree and build up its current state as-is. + hook.getFiberRoots(rendererID).forEach(root => { + currentRootID = getFiberID(getPrimaryFiber(root.current)); + + if (isProfiling) { + // If profiling is active, store commit time and duration, and the current interactions. + // The frontend may request this information after profiling has stopped. + currentCommitProfilingMetadata = { + actualDurations: [], + commitTime: performance.now() - profilingStartTime, + interactions: Array.from(root.memoizedInteractions).map( + (interaction: Interaction) => ({ + ...interaction, + timestamp: interaction.timestamp - profilingStartTime, + }) + ), + maxActualDuration: 0, + }; + } + + mountFiber(root.current, null); + flushPendingEvents(root); + currentRootID = -1; + }); + } } function handleCommitFiberUnmount(fiber) { @@ -1623,6 +1648,7 @@ export function attach( return { cleanup, + flushInitialOperations, getCommitDetails, getFiberIDFromNative, getInteractions, @@ -1641,6 +1667,5 @@ export function attach( setInState, startProfiling, stopProfiling, - walkTree, }; } diff --git a/src/backend/types.js b/src/backend/types.js index 9e85dac6d2..8768715d41 100644 --- a/src/backend/types.js +++ b/src/backend/types.js @@ -82,6 +82,7 @@ export type ProfilingSummary = {| export type RendererInterface = { cleanup: () => void, + flushInitialOperations: () => void, getCommitDetails: (rootID: number, commitIndex: number) => CommitDetails, getNativeFromReactElement?: ?(component: Fiber) => ?NativeType, getFiberIDFromNative: ( @@ -108,7 +109,6 @@ export type RendererInterface = { setInState: (id: number, path: Array, value: any) => void, startProfiling: () => void, stopProfiling: () => void, - walkTree: () => void, }; export type Handler = (data: any) => void; diff --git a/src/devtools/views/Profiler/CommitTreeBuilder.js b/src/devtools/views/Profiler/CommitTreeBuilder.js index 92e85c62ad..2f714aaa9d 100644 --- a/src/devtools/views/Profiler/CommitTreeBuilder.js +++ b/src/devtools/views/Profiler/CommitTreeBuilder.js @@ -112,14 +112,9 @@ export function getCommitTree({ } } - console.error( + throw Error( `getCommitTree(): Unable to reconstruct tree for root "${rootID}" and commit ${commitIndex}` ); - - return { - nodes: new Map(), - rootID, - }; } function recursivelyIniitliazeTree( diff --git a/src/devtools/views/Profiler/FlamegraphChartBuilder.js b/src/devtools/views/Profiler/FlamegraphChartBuilder.js index df183d63d9..9a7339cd01 100644 --- a/src/devtools/views/Profiler/FlamegraphChartBuilder.js +++ b/src/devtools/views/Profiler/FlamegraphChartBuilder.js @@ -56,6 +56,11 @@ export function getChartData({ idToDepthMap.set(id, currentDepth); const node = ((nodes.get(id): any): Node); + + if (node == null) { + throw Error(`Could not find node with id "${id}" in commit tree`); + } + const name = node.displayName || 'Unknown'; const selfDuration = calculateSelfDuration(id, commitTree, commitDetails); diff --git a/src/devtools/views/Profiler/RankedChartBuilder.js b/src/devtools/views/Profiler/RankedChartBuilder.js index f900a6d3f0..b514cac3bc 100644 --- a/src/devtools/views/Profiler/RankedChartBuilder.js +++ b/src/devtools/views/Profiler/RankedChartBuilder.js @@ -41,6 +41,10 @@ export function getChartData({ actualDurations.forEach((actualDuration, id) => { const node = ((nodes.get(id): any): Node); + if (node == null) { + throw Error(`Could not find node with id "${id}" in commit tree`); + } + // Don't show the root node in this chart. if (node.parentID === 0) { return; From 2a80f8ca9ca0be44cca9539d83ede9a5d62b62d0 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 1 Apr 2019 14:43:46 -0700 Subject: [PATCH 4/4] Show is-recording indicator earlier after a reload-and-profile --- shells/browser/shared/src/main.js | 3 +++ src/devtools/store.js | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/shells/browser/shared/src/main.js b/shells/browser/shared/src/main.js index 08befa651b..d1d4eb1024 100644 --- a/shells/browser/shared/src/main.js +++ b/shells/browser/shared/src/main.js @@ -78,15 +78,18 @@ function createPanelIfReactLoaded() { // This flag lets us tip the Store off early that we expect to be profiling. // This avoids flashing a temporary "Profiling not supported" message in the Profiler tab, // after a user has clicked the "reload and profile" button. + let isProfiling = false; let supportsProfiling = false; if (localStorage.getItem(SUPPORTS_PROFILING_KEY) === 'true') { supportsProfiling = true; + isProfiling = true; localStorage.removeItem(SUPPORTS_PROFILING_KEY); } const browserName = getBrowserName(); store = new Store(bridge, { + isProfiling, supportsFileDownloads: browserName === 'Chrome', supportsReloadAndProfile: true, supportsProfiling, diff --git a/src/devtools/store.js b/src/devtools/store.js index 708f3ee50b..345dee9b0c 100644 --- a/src/devtools/store.js +++ b/src/devtools/store.js @@ -103,10 +103,14 @@ export default class Store extends EventEmitter { if (config != null) { const { + isProfiling, supportsFileDownloads, supportsProfiling, supportsReloadAndProfile, } = config; + if (isProfiling) { + this._isProfiling = true; + } if (supportsFileDownloads) { this._supportsFileDownloads = true; }