From f3ad480ccfe74afd0aab76344e152e6e0529db26 Mon Sep 17 00:00:00 2001 From: Chris Dobson Date: Sun, 21 Feb 2021 22:29:07 +0000 Subject: [PATCH 01/13] ensure sync-xhr is allowed before reload and profile --- .../src/backend/agent.js | 23 +++++++++++-------- .../src/backend/utils.js | 7 ++++++ .../src/devtools/store.js | 16 +++++++++++++ .../views/Profiler/ReloadAndProfileButton.js | 20 ++++++++++++++-- 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 41993700252cf..5412ea986c0ca 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -38,6 +38,7 @@ import type { RendererInterface, } from './types'; import type {ComponentFilter} from '../types'; +import {isSynchronousXHRSupported} from './utils'; const debug = (methodName, ...args) => { if (__DEBUG__) { @@ -492,16 +493,20 @@ export default class Agent extends EventEmitter<{| }; reloadAndProfile = (recordChangeDescriptions: boolean) => { - sessionStorageSetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, 'true'); - sessionStorageSetItem( - SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, - recordChangeDescriptions ? 'true' : 'false', - ); + if (isSynchronousXHRSupported()) { + sessionStorageSetItem(SESSION_STORAGE_RELOAD_AND_PROFILE_KEY, 'true'); + sessionStorageSetItem( + SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY, + recordChangeDescriptions ? 'true' : 'false', + ); - // This code path should only be hit if the shell has explicitly told the Store that it supports profiling. - // In that case, the shell must also listen for this specific message to know when it needs to reload the app. - // The agent can't do this in a way that is renderer agnostic. - this._bridge.send('reloadAppForProfiling'); + // This code path should only be hit if the shell has explicitly told the Store that it supports profiling. + // In that case, the shell must also listen for this specific message to know when it needs to reload the app. + // The agent can't do this in a way that is renderer agnostic. + this._bridge.send('reloadAppForProfiling'); + } else { + this._bridge.send('isSynchronousXHRSupported', false); + } }; renamePath = ({ diff --git a/packages/react-devtools-shared/src/backend/utils.js b/packages/react-devtools-shared/src/backend/utils.js index 06329eff72150..5a4de539fedaa 100644 --- a/packages/react-devtools-shared/src/backend/utils.js +++ b/packages/react-devtools-shared/src/backend/utils.js @@ -183,3 +183,10 @@ export function format( return '' + formatted; } + +export function isSynchronousXHRSupported() { + return ( + window.document.featurePolicy && + window.document.featurePolicy.allowsFeature('sync-xhr') + ); +} diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index e5a6ac15fbde1..d22fa0f7c19d7 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -141,6 +141,9 @@ export default class Store extends EventEmitter<{| _supportsReloadAndProfile: boolean = false; _supportsTraceUpdates: boolean = false; + // Needed for reload and profile - assume supported until told otherwise + _supportsSynchronousXHR: boolean = true; + _unsupportedRendererVersionDetected: boolean = false; // Total number of visible elements (within all roots). @@ -205,6 +208,10 @@ export default class Store extends EventEmitter<{| 'unsupportedRendererVersion', this.onBridgeUnsupportedRendererVersion, ); + bridge.addListener( + 'isSynchronousXHRSupported', + this.onBridgeSynchronousXHRSupported, + ); this._profilerStore = new ProfilerStore(bridge, this, isProfiling); } @@ -365,6 +372,9 @@ export default class Store extends EventEmitter<{| // Both of these are required for the reload-and-profile feature to work. return this._supportsReloadAndProfile && this._isBackendStorageAPISupported; } + get supportsSynchronousXHR(): boolean { + return this._supportsSynchronousXHR; + } get supportsTraceUpdates(): boolean { return this._supportsTraceUpdates; @@ -1149,4 +1159,10 @@ export default class Store extends EventEmitter<{| this.emit('unsupportedRendererVersionDetected'); }; + + onBridgeSynchronousXHRSupported = (isSynchronousXHRSupported: boolean) => { + this._supportsSynchronousXHR = isSynchronousXHRSupported; + + this.emit('supportsSynchronousXHR'); + }; } diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js index a54804bdce7e1..9c4f987f6e7fb 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js @@ -8,11 +8,12 @@ */ import * as React from 'react'; -import {useCallback, useContext, useMemo} from 'react'; +import {useCallback, useContext, useMemo, useEffect} from 'react'; import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; import {BridgeContext, StoreContext} from '../context'; import {useSubscription} from '../hooks'; +import {ModalDialogContext} from '../ModalDialog'; type SubscriptionData = {| recordChangeDescriptions: boolean, @@ -28,13 +29,16 @@ export default function ReloadAndProfileButton() { getCurrentValue: () => ({ recordChangeDescriptions: store.recordChangeDescriptions, supportsReloadAndProfile: store.supportsReloadAndProfile, + supportsSynchronousXHR: store.supportsSynchronousXHR, }), subscribe: (callback: Function) => { store.addListener('recordChangeDescriptions', callback); store.addListener('supportsReloadAndProfile', callback); + store.addListener('supportsSynchronousXHR', callback); return () => { store.removeListener('recordChangeDescriptions', callback); store.removeListener('supportsReloadAndProfile', callback); + store.removeListener('supportsSynchronousXHR', callback); }; }, }), @@ -43,8 +47,20 @@ export default function ReloadAndProfileButton() { const { recordChangeDescriptions, supportsReloadAndProfile, + supportsSynchronousXHR, } = useSubscription(subscription); + const {dispatch: modalDialogDispatch} = useContext(ModalDialogContext); + useEffect(() => { + if (!supportsSynchronousXHR) { + modalDialogDispatch({ + type: 'SHOW', + content: + 'Synchronous XHR is required for reload and profile but is disabled on this site.', + }); + } + }, [supportsSynchronousXHR, modalDialogDispatch]); + const reloadAndProfile = useCallback(() => { // TODO If we want to support reload-and-profile for e.g. React Native, // we might need to also start profiling here before reloading the app (since DevTools itself isn't reloaded). @@ -61,7 +77,7 @@ export default function ReloadAndProfileButton() { return ( ); From 9665d7e02e7217c5cf65db24d70b48c523ef1e1c Mon Sep 17 00:00:00 2001 From: Chris Dobson Date: Tue, 9 Mar 2021 16:56:47 +0000 Subject: [PATCH 05/13] ensure window.document is truthy when checking feature policy --- packages/react-devtools-shared/src/backend/utils.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/backend/utils.js b/packages/react-devtools-shared/src/backend/utils.js index 5a4de539fedaa..a275ffdf82939 100644 --- a/packages/react-devtools-shared/src/backend/utils.js +++ b/packages/react-devtools-shared/src/backend/utils.js @@ -184,8 +184,9 @@ export function format( return '' + formatted; } -export function isSynchronousXHRSupported() { +export function isSynchronousXHRSupported(): boolean { return ( + window.document && window.document.featurePolicy && window.document.featurePolicy.allowsFeature('sync-xhr') ); From b10523abae73057fe1e2ec6b52f8d338d78fedbe Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 11 Mar 2021 09:50:21 -0500 Subject: [PATCH 06/13] Added an inline comment. --- packages/react-devtools-shared/src/backend/agent.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 86c35ae1b49bd..11eab85a01935 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -38,7 +38,8 @@ import type { RendererInterface, } from './types'; import type {ComponentFilter} from '../types'; -import {isSynchronousXHRSupported} from './utils'; +import { +} from './utils'; const debug = (methodName, ...args) => { if (__DEBUG__) { @@ -216,6 +217,7 @@ export default class Agent extends EventEmitter<{| // Notify the frontend if the backend supports the Storage API (e.g. localStorage). // If not, features like reload-and-profile will not work correctly and must be disabled. + // The same goes for sync XHR requests; it's how DevTools injects before ReactDOM starts rendering. let isBackendStorageAPISupported = false; try { localStorage.getItem('test'); From 3648db28a7e74e0c978f2db0e05fdd12bfef58bb Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 11 Mar 2021 10:03:38 -0500 Subject: [PATCH 07/13] Convert isSynchronousXHRSupported to boolean explicitly --- packages/react-devtools-shared/src/backend/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/backend/utils.js b/packages/react-devtools-shared/src/backend/utils.js index a275ffdf82939..7a4ff6295ab99 100644 --- a/packages/react-devtools-shared/src/backend/utils.js +++ b/packages/react-devtools-shared/src/backend/utils.js @@ -185,7 +185,7 @@ export function format( } export function isSynchronousXHRSupported(): boolean { - return ( + return !!( window.document && window.document.featurePolicy && window.document.featurePolicy.allowsFeature('sync-xhr') From fea90f79a962e059b37f786e48d174eed10ba839 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 11 Mar 2021 10:17:31 -0500 Subject: [PATCH 08/13] Store supportsReloadAndProfile getter also considers isSynchronousXHRSupported --- .../src/devtools/store.js | 78 ++++++++++++------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 9ce40f4981ac2..caface99b2adc 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -77,7 +77,7 @@ export default class Store extends EventEmitter<{| supportsProfiling: [], supportsReloadAndProfile: [], unsupportedRendererVersionDetected: [], - supportsSynchronousXHR: [], + isSynchronousXHRSupported: [], |}> { _bridge: FrontendBridge; @@ -112,6 +112,12 @@ export default class Store extends EventEmitter<{| // If not, features like reload-and-profile will not work correctly and must be disabled. _isBackendStorageAPISupported: boolean = false; + // Can DevTools use sync XHR requests? + // If not, features like reload-and-profile will not work correctly and must be disabled. + // This current limitation applies only to web extension builds + // and will need to be reconsidered in the future if we add support for reload to React Native. + _isSynchronousXHRSupported: boolean = false; + _nativeStyleEditorValidAttributes: $ReadOnlyArray | null = null; // Map of element (id) to the set of elements (ids) it owns. @@ -142,9 +148,6 @@ export default class Store extends EventEmitter<{| _supportsReloadAndProfile: boolean = false; _supportsTraceUpdates: boolean = false; - // Needed for reload and profile - _supportsSynchronousXHR: boolean = false; - _unsupportedRendererVersionDetected: boolean = false; // Total number of visible elements (within all roots). @@ -199,20 +202,20 @@ export default class Store extends EventEmitter<{| bridge.addListener('shutdown', this.onBridgeShutdown); bridge.addListener( 'isBackendStorageAPISupported', - this.onBridgeStorageSupported, + this.onBackendStorageAPISupported, ); bridge.addListener( 'isNativeStyleEditorSupported', this.onBridgeNativeStyleEditorSupported, ); - bridge.addListener( - 'unsupportedRendererVersion', - this.onBridgeUnsupportedRendererVersion, - ); bridge.addListener( 'isSynchronousXHRSupported', this.onBridgeSynchronousXHRSupported, ); + bridge.addListener( + 'unsupportedRendererVersion', + this.onBridgeUnsupportedRendererVersion, + ); this._profilerStore = new ProfilerStore(bridge, this, isProfiling); } @@ -318,6 +321,10 @@ export default class Store extends EventEmitter<{| return this._hasOwnerMetadata; } + get isSynchronousXHRSupported(): boolean { + return this._isSynchronousXHRSupported; + } + get nativeStyleEditorValidAttributes(): $ReadOnlyArray | null { return this._nativeStyleEditorValidAttributes; } @@ -367,14 +374,16 @@ export default class Store extends EventEmitter<{| get supportsProfiling(): boolean { return this._supportsProfiling; } + get supportsReloadAndProfile(): boolean { // Does the DevTools shell support reloading and eagerly injecting the renderer interface? - // And if so, can the backend use the localStorage API? - // Both of these are required for the reload-and-profile feature to work. - return this._supportsReloadAndProfile && this._isBackendStorageAPISupported; - } - get supportsSynchronousXHR(): boolean { - return this._supportsSynchronousXHR; + // And if so, can the backend use the localStorage API and sync XHR? + // All of these are currently required for the reload-and-profile feature to work. + return ( + this._supportsReloadAndProfile && + this._isBackendStorageAPISupported && + this._isSynchronousXHRSupported + ); } get supportsTraceUpdates(): boolean { @@ -1141,29 +1150,46 @@ export default class Store extends EventEmitter<{| debug('onBridgeShutdown', 'unsubscribing from Bridge'); } - this._bridge.removeListener('operations', this.onBridgeOperations); - this._bridge.removeListener('shutdown', this.onBridgeShutdown); - this._bridge.removeListener( + const bridge = this._bridge; + bridge.removeListener('operations', this.onBridgeOperations); + bridge.removeListener( + 'overrideComponentFilters', + this.onBridgeOverrideComponentFilters, + ); + bridge.removeListener('shutdown', this.onBridgeShutdown); + bridge.removeListener( 'isBackendStorageAPISupported', - this.onBridgeStorageSupported, + this.onBackendStorageAPISupported, + ); + bridge.removeListener( + 'isNativeStyleEditorSupported', + this.onBridgeNativeStyleEditorSupported, + ); + bridge.removeListener( + 'isSynchronousXHRSupported', + this.onBridgeSynchronousXHRSupported, + ); + bridge.removeListener( + 'unsupportedRendererVersion', + this.onBridgeUnsupportedRendererVersion, ); }; - onBridgeStorageSupported = (isBackendStorageAPISupported: boolean) => { + onBackendStorageAPISupported = (isBackendStorageAPISupported: boolean) => { this._isBackendStorageAPISupported = isBackendStorageAPISupported; this.emit('supportsReloadAndProfile'); }; - onBridgeUnsupportedRendererVersion = () => { - this._unsupportedRendererVersionDetected = true; + onBridgeSynchronousXHRSupported = (isSynchronousXHRSupported: boolean) => { + this._isSynchronousXHRSupported = isSynchronousXHRSupported; - this.emit('unsupportedRendererVersionDetected'); + this.emit('supportsReloadAndProfile'); }; - onBridgeSynchronousXHRSupported = (isSynchronousXHRSupported: boolean) => { - this._supportsSynchronousXHR = isSynchronousXHRSupported; + onBridgeUnsupportedRendererVersion = () => { + this._unsupportedRendererVersionDetected = true; - this.emit('supportsSynchronousXHR'); + this.emit('unsupportedRendererVersionDetected'); }; } From 49edee75f60c7989f65969063e89b14e6d5873f2 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 11 Mar 2021 10:17:57 -0500 Subject: [PATCH 09/13] Reverted change to ReloadAndProfileButton tooltip. --- .../views/Profiler/ReloadAndProfileButton.js | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js b/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js index 9c41b24bf8627..a54804bdce7e1 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/ReloadAndProfileButton.js @@ -17,7 +17,6 @@ import {useSubscription} from '../hooks'; type SubscriptionData = {| recordChangeDescriptions: boolean, supportsReloadAndProfile: boolean, - supportsSynchronousXHR: boolean, |}; export default function ReloadAndProfileButton() { @@ -29,16 +28,13 @@ export default function ReloadAndProfileButton() { getCurrentValue: () => ({ recordChangeDescriptions: store.recordChangeDescriptions, supportsReloadAndProfile: store.supportsReloadAndProfile, - supportsSynchronousXHR: store.supportsSynchronousXHR, }), subscribe: (callback: Function) => { store.addListener('recordChangeDescriptions', callback); store.addListener('supportsReloadAndProfile', callback); - store.addListener('supportsSynchronousXHR', callback); return () => { store.removeListener('recordChangeDescriptions', callback); store.removeListener('supportsReloadAndProfile', callback); - store.removeListener('supportsSynchronousXHR', callback); }; }, }), @@ -47,7 +43,6 @@ export default function ReloadAndProfileButton() { const { recordChangeDescriptions, supportsReloadAndProfile, - supportsSynchronousXHR, } = useSubscription(subscription); const reloadAndProfile = useCallback(() => { @@ -64,15 +59,11 @@ export default function ReloadAndProfileButton() { return null; } - const buttonTitle = supportsSynchronousXHR - ? 'Reload and start profiling' - : 'Reload and start profiling has been disabled because synchronous XHR is not supported on this site'; - return ( ); From 1df7ab1b5735959ab7b330ea6f3ce3c0507e7117 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 11 Mar 2021 10:18:59 -0500 Subject: [PATCH 10/13] Alpha sort nit --- packages/react-devtools-shared/src/bridge.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/bridge.js b/packages/react-devtools-shared/src/bridge.js index 0fae9cfe0c06f..d3b669bd42871 100644 --- a/packages/react-devtools-shared/src/bridge.js +++ b/packages/react-devtools-shared/src/bridge.js @@ -122,6 +122,7 @@ export type BackendEvents = {| extensionBackendInitialized: [], inspectedElement: [InspectedElementPayload], isBackendStorageAPISupported: [boolean], + isSynchronousXHRSupported: [boolean], operations: [Array], ownersList: [OwnersList], overrideComponentFilters: [Array], @@ -134,7 +135,6 @@ export type BackendEvents = {| syncSelectionFromNativeElementsPanel: [], syncSelectionToNativeElementsPanel: [], unsupportedRendererVersion: [RendererID], - isSynchronousXHRSupported: [boolean], // React Native style editor plug-in. isNativeStyleEditorSupported: [ From df07a8df8bcf7668c6b50dfe7dfab569153b52c5 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 11 Mar 2021 10:20:23 -0500 Subject: [PATCH 11/13] Removed no-longer-used isSynchronousXHRSupported getter --- packages/react-devtools-shared/src/devtools/store.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index caface99b2adc..c795e23263985 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -321,10 +321,6 @@ export default class Store extends EventEmitter<{| return this._hasOwnerMetadata; } - get isSynchronousXHRSupported(): boolean { - return this._isSynchronousXHRSupported; - } - get nativeStyleEditorValidAttributes(): $ReadOnlyArray | null { return this._nativeStyleEditorValidAttributes; } From 92c6cae4a1b3fc1b74cf55e29f614aa0c8bb7625 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 11 Mar 2021 10:23:44 -0500 Subject: [PATCH 12/13] Updating agent.js again GitHub's inline editor seems to have messed up the previous inline edit. --- packages/react-devtools-shared/src/backend/agent.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 11eab85a01935..86c35ae1b49bd 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -38,8 +38,7 @@ import type { RendererInterface, } from './types'; import type {ComponentFilter} from '../types'; -import { -} from './utils'; +import {isSynchronousXHRSupported} from './utils'; const debug = (methodName, ...args) => { if (__DEBUG__) { @@ -217,7 +216,6 @@ export default class Agent extends EventEmitter<{| // Notify the frontend if the backend supports the Storage API (e.g. localStorage). // If not, features like reload-and-profile will not work correctly and must be disabled. - // The same goes for sync XHR requests; it's how DevTools injects before ReactDOM starts rendering. let isBackendStorageAPISupported = false; try { localStorage.getItem('test'); From 2310a7d5b2cd925afe499fd56dd832db43b79856 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 11 Mar 2021 10:26:47 -0500 Subject: [PATCH 13/13] Oops removed 'isSynchronousXHRSupported' from EventEmitter config We're no longer emitting an event for that. --- packages/react-devtools-shared/src/devtools/store.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index c795e23263985..c0083f90a16e4 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -77,7 +77,6 @@ export default class Store extends EventEmitter<{| supportsProfiling: [], supportsReloadAndProfile: [], unsupportedRendererVersionDetected: [], - isSynchronousXHRSupported: [], |}> { _bridge: FrontendBridge;