Skip to content

[DevTools] browser extension: improve script injection logic #26492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 28, 2023
Merged

[DevTools] browser extension: improve script injection logic #26492

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
53442d3
[DevTools] improve script injection logic
mondaychen Mar 27, 2023
4b60980
fix lint
mondaychen Mar 28, 2023
0904f8d
fix lint
mondaychen Mar 28, 2023
096811d
fix per comment
mondaychen Mar 28, 2023
002d855
comment nit
mondaychen Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-devtools-extensions/chrome/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"description": "Adds React debugging tools to the Chrome Developer Tools.",
"version": "4.27.4",
"version_name": "4.27.4",
"minimum_chrome_version": "88",
"minimum_chrome_version": "102",
"icons": {
"16": "icons/16-production.png",
"32": "icons/32-production.png",
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-extensions/edge/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"description": "Adds React debugging tools to the Microsoft Edge Developer Tools.",
"version": "4.27.4",
"version_name": "4.27.4",
"minimum_chrome_version": "88",
"minimum_chrome_version": "102",
"icons": {
"16": "icons/16-production.png",
"32": "icons/32-production.png",
Expand Down
14 changes: 14 additions & 0 deletions packages/react-devtools-extensions/src/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {IS_FIREFOX} from './utils';
const ports = {};

if (!IS_FIREFOX) {
// equivalent logic for Firefox is in prepareInjection.js
// Manifest V3 method of injecting content scripts (not yet supported in Firefox)
// Note: the "world" option in registerContentScripts is only available in Chrome v102+
// It's critical since it allows us to directly run scripts on the "main" world on the page
Expand Down Expand Up @@ -182,5 +183,18 @@ chrome.runtime.onMessage.addListener((request, sender) => {
break;
}
}
} else if (request.payload?.tabId) {
const tabId = request.payload?.tabId;
// This is sent from the devtools page when it is ready for injecting the backend
if (request.payload.type === 'react-devtools-inject-backend') {
Copy link
Contributor

@hoxyq hoxyq Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should we put react-devtools-inject-backend into constant?

I see its used in multiple places:

react/packages/react-devtools-extensions/src/main.js

Line 192 in 41b4714

`window.postMessage({ source: 'react-devtools-inject-backend' }, '*');`,

react/packages/react-devtools-extensions/src/contentScripts/prepareInjection.js

Line 92 in 41b4714

case 'react-devtools-inject-backend':

Copy link
Contributor Author

@mondaychen mondaychen Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many event names/types are used in this way. I'm not against extracting them into a shared constant file as a better engineering work, but let's use another PR to take care of them all together.

Copy link

@incepter incepter Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The big difference imo between using a constant and literals in that a constant will get allocated at the module level and would weight the sum of all the literals plus some small management extra.
Literals on the other hand are only allocated when used and referenced (they may even get GCed like in case constant).

if (!IS_FIREFOX) {
// equivalent logic for Firefox is in prepareInjection.js
chrome.scripting.executeScript({
target: {tabId},
files: ['/build/react_devtools_backend.js'],
world: chrome.scripting.ExecutionWorld.MAIN,
});
}
}
}
});
45 changes: 18 additions & 27 deletions packages/react-devtools-extensions/src/contentScripts/prepareInjection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
import nullthrows from 'nullthrows';
import {IS_FIREFOX} from '../utils';

// We run scripts on the page via the service worker (backgroud.js) for
// Manifest V3 extensions (Chrome & Edge).
// We need to inject this code for Firefox only because it does not support ExecutionWorld.MAIN
// https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/scripting/ExecutionWorld
// In this content script we have access to DOM, but don't have access to the webpage's window,
// so we inject this inline script tag into the webpage (allowed in Manifest V2).
function injectScriptSync(src) {
let code = '';
const request = new XMLHttpRequest();
Expand All @@ -21,13 +27,6 @@ function injectScriptSync(src) {
nullthrows(script.parentNode).removeChild(script);
}

function injectScriptAsync(src) {
const script = document.createElement('script');
script.src = src;
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"
Expand Down Expand Up @@ -90,9 +89,11 @@ window.addEventListener('message', function onMessage({data, source}) {
}
break;
case 'react-devtools-inject-backend':
injectScriptAsync(
chrome.runtime.getURL('build/react_devtools_backend.js'),
);
if (IS_FIREFOX) {
injectScriptSync(
chrome.runtime.getURL('build/react_devtools_backend.js'),
);
}
break;
}
});
Expand All @@ -108,25 +109,15 @@ window.addEventListener('pageshow', function ({target}) {
chrome.runtime.sendMessage(lastDetectionResult);
});

// We create a "sync" script tag to page to inject the global hook on Manifest V2 extensions.
// To comply with the new security policy in V3, we use chrome.scripting.registerContentScripts instead (see background.js).
// However, the new API only works for Chrome v102+.
// We insert a "async" script tag as a fallback for older versions.
// It has known issues if JS on the page is faster than the extension.
// Users will see a notice in components tab when that happens (see <Tree>).
// For Firefox, V3 is not ready, so sync injection is still the best approach.
const injectScript = IS_FIREFOX ? injectScriptSync : injectScriptAsync;

// Inject a __REACT_DEVTOOLS_GLOBAL_HOOK__ global for React to interact with.
// Only do this for HTML documents though, to avoid e.g. breaking syntax highlighting for XML docs.
// We need to inject this code because content scripts (ie injectGlobalHook.js) don't have access
// to the webpage's window, so in order to access front end settings
// and communicate with React, we must inject this code into the webpage
switch (document.contentType) {
case 'text/html':
case 'application/xhtml+xml': {
injectScript(chrome.runtime.getURL('build/installHook.js'));
break;
if (IS_FIREFOX) {
switch (document.contentType) {
case 'text/html':
case 'application/xhtml+xml': {
injectScriptSync(chrome.runtime.getURL('build/installHook.js'));
break;
}
}
}

Expand Down
43 changes: 26 additions & 17 deletions packages/react-devtools-extensions/src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {flushSync} from 'react-dom';
import {createRoot} from 'react-dom/client';
import Bridge from 'react-devtools-shared/src/bridge';
import Store from 'react-devtools-shared/src/devtools/store';
import {getBrowserName, getBrowserTheme} from './utils';
import {IS_CHROME, IS_EDGE, getBrowserTheme} from './utils';
import {LOCAL_STORAGE_TRACE_UPDATES_ENABLED_KEY} from 'react-devtools-shared/src/constants';
import {registerDevToolsEventLogger} from 'react-devtools-shared/src/registerDevToolsEventLogger';
import {
Expand All @@ -27,9 +27,6 @@ import {logEvent} from 'react-devtools-shared/src/Logger';
const LOCAL_STORAGE_SUPPORTS_PROFILING_KEY =
'React::DevTools::supportsProfiling';

const isChrome = getBrowserName() === 'Chrome';
const isEdge = getBrowserName() === 'Edge';

// rAF never fires on devtools_page (because it's in the background)
// https://bugs.chromium.org/p/chromium/issues/detail?id=1241986#c31
// Since we render React elements here, we need to polyfill it with setTimeout
Expand Down Expand Up @@ -176,10 +173,10 @@ function createPanelIfReactLoaded() {

store = new Store(bridge, {
isProfiling,
supportsReloadAndProfile: isChrome || isEdge,
supportsReloadAndProfile: IS_CHROME || IS_EDGE,
supportsProfiling,
// At this time, the timeline can only parse Chrome performance profiles.
supportsTimeline: isChrome,
supportsTimeline: IS_CHROME,
supportsTraceUpdates: true,
});
if (!isProfiling) {
Expand All @@ -188,14 +185,26 @@ function createPanelIfReactLoaded() {

// Initialize the backend only once the Store has been initialized.
// Otherwise the Store may miss important initial tree op codes.
chrome.devtools.inspectedWindow.eval(
`window.postMessage({ source: 'react-devtools-inject-backend' }, '*');`,
function (response, evalError) {
if (evalError) {
console.error(evalError);
}
},
);
if (IS_CHROME | IS_EDGE) {
Copy link
Contributor

@hoxyq hoxyq Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is xor operator expected here?

Copy link
Contributor Author

@mondaychen mondaychen Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this!

Copy link

@discover59 discover59 Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it XOR operator? I think ^ is XOR operator.

Copy link
Contributor

@hoxyq hoxyq Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it XOR operator? I think ^ is XOR operator.

Yeah, my bad, its a bitwise or

chrome.runtime.sendMessage({
source: 'react-devtools-main',
payload: {
type: 'react-devtools-inject-backend',
tabId,
},
});
} else {
// Firefox does not support executing script in MAIN wolrd from content script.
// see prepareInjection.js
chrome.devtools.inspectedWindow.eval(
`window.postMessage({ source: 'react-devtools-inject-backend' }, '*');`,
function (response, evalError) {
if (evalError) {
console.error(evalError);
}
},
);
}

const viewAttributeSourceFunction = (id, path) => {
const rendererID = store.getRendererIDForElement(id);
Expand Down Expand Up @@ -255,7 +264,7 @@ function createPanelIfReactLoaded() {
// For some reason in Firefox, chrome.runtime.sendMessage() from a content script
// never reaches the chrome.runtime.onMessage event listener.
let fetchFileWithCaching = null;
if (isChrome) {
if (IS_CHROME) {
const fetchFromNetworkCache = (url, resolve, reject) => {
// Debug ID allows us to avoid re-logging (potentially long) URL strings below,
// while also still associating (potentially) interleaved logs with the original request.
Expand Down Expand Up @@ -463,7 +472,7 @@ function createPanelIfReactLoaded() {
let needsToSyncElementSelection = false;

chrome.devtools.panels.create(
isChrome || isEdge ? '⚛️ Components' : 'Components',
IS_CHROME || IS_EDGE ? '⚛️ Components' : 'Components',
'',
'panel.html',
extensionPanel => {
Expand Down Expand Up @@ -494,7 +503,7 @@ function createPanelIfReactLoaded() {
);

chrome.devtools.panels.create(
isChrome || isEdge ? '⚛️ Profiler' : 'Profiler',
IS_CHROME || IS_EDGE ? '⚛️ Profiler' : 'Profiler',
'',
'panel.html',
extensionPanel => {
Expand Down