Skip to content

Commit 5751bc5

Browse files
codebyterecodebytere
authored
fix: clear device metrics overrides when a DevTools client disconnects (#53133)
fix: clear device metrics overrides when a DevTools client disconnects (#52254) (cherry picked from commit 084e02b)
1 parent 277efdf commit 5751bc5

6 files changed

Lines changed: 195 additions & 3 deletions

File tree

‎shell/browser/ui/devtools_manager_delegate.cc‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,10 @@
1212
#include "base/functional/bind.h"
1313
#include "base/path_service.h"
1414
#include "base/strings/string_number_conversions.h"
15+
#include "content/browser/web_contents/web_contents_impl.h" // nogncheck
1516
#include "content/public/browser/browser_thread.h"
1617
#include "content/public/browser/devtools_agent_host.h"
18+
#include "content/public/browser/devtools_agent_host_client_channel.h"
1719
#include "content/public/browser/devtools_frontend_host.h"
1820
#include "content/public/browser/devtools_socket_factory.h"
1921
#include "content/public/browser/favicon_status.h"
@@ -82,6 +84,10 @@ std::unique_ptr<content::DevToolsSocketFactory> CreateSocketFactory() {
8284
}
8385

8486
const char kBrowserCloseMethod[] = "Browser.close";
87+
const char kSetDeviceMetricsOverrideMethod[] =
88+
"Emulation.setDeviceMetricsOverride";
89+
const char kClearDeviceMetricsOverrideMethod[] =
90+
"Emulation.clearDeviceMetricsOverride";
8591

8692
} // namespace
8793

@@ -107,6 +113,14 @@ void DevToolsManagerDelegate::HandleCommand(
107113
NotHandledCallback callback) {
108114
crdtp::Dispatchable dispatchable(crdtp::SpanFrom(message));
109115
DCHECK(dispatchable.ok());
116+
if (crdtp::SpanEquals(crdtp::SpanFrom(kSetDeviceMetricsOverrideMethod),
117+
dispatchable.Method())) {
118+
channels_with_device_overrides_.insert(channel);
119+
} else if (crdtp::SpanEquals(
120+
crdtp::SpanFrom(kClearDeviceMetricsOverrideMethod),
121+
dispatchable.Method())) {
122+
channels_with_device_overrides_.erase(channel);
123+
}
110124
if (crdtp::SpanEquals(crdtp::SpanFrom(kBrowserCloseMethod),
111125
dispatchable.Method())) {
112126
// In theory, we should respond over the protocol saying that the
@@ -122,6 +136,40 @@ void DevToolsManagerDelegate::HandleCommand(
122136
std::move(callback).Run(message);
123137
}
124138

139+
void DevToolsManagerDelegate::ClientAttached(
140+
content::DevToolsAgentHostClientChannel* channel) {
141+
// Drop stale bookkeeping in case the channel's address was reused.
142+
channels_with_device_overrides_.erase(channel);
143+
}
144+
145+
void DevToolsManagerDelegate::ClientDetached(
146+
content::DevToolsAgentHostClientChannel* channel) {
147+
if (channels_with_device_overrides_.erase(channel) == 0)
148+
return;
149+
150+
// Session teardown (EmulationHandler::Disable()) does not undo the view
151+
// resize done by Emulation.setDeviceMetricsOverride, so a client that
152+
// detaches without clearing its overrides leaves the view pinned at the
153+
// emulated size forever. Restore it here until that is fixed upstream.
154+
// This applies to every kind of client (remote debugging, the bundled
155+
// frontend, webContents.debugger) since they all detach the same way.
156+
content::WebContents* web_contents =
157+
channel->GetAgentHost()->GetWebContents();
158+
if (!web_contents || web_contents->IsBeingDestroyed())
159+
return;
160+
161+
// Leave the size alone if another client still holds an override on the
162+
// same WebContents; it is cleared when that client clears it or detaches.
163+
for (content::DevToolsAgentHostClientChannel* other :
164+
channels_with_device_overrides_) {
165+
if (other->GetAgentHost()->GetWebContents() == web_contents)
166+
return;
167+
}
168+
169+
static_cast<content::WebContentsImpl*>(web_contents)
170+
->ClearDeviceEmulationSize();
171+
}
172+
125173
scoped_refptr<content::DevToolsAgentHost>
126174
DevToolsManagerDelegate::CreateNewTarget(const GURL& url,
127175
TargetType target_type,

‎shell/browser/ui/devtools_manager_delegate.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
#include <string>
99

10+
#include "base/containers/flat_set.h"
1011
#include "content/public/browser/devtools_manager_delegate.h"
1112

1213
namespace content {
@@ -31,6 +32,10 @@ class DevToolsManagerDelegate : public content::DevToolsManagerDelegate {
3132
void HandleCommand(content::DevToolsAgentHostClientChannel* channel,
3233
base::span<const uint8_t> message,
3334
NotHandledCallback callback) override;
35+
void ClientAttached(
36+
content::DevToolsAgentHostClientChannel* channel) override;
37+
void ClientDetached(
38+
content::DevToolsAgentHostClientChannel* channel) override;
3439
scoped_refptr<content::DevToolsAgentHost> CreateNewTarget(
3540
const GURL& url,
3641
TargetType target_type,
@@ -39,6 +44,11 @@ class DevToolsManagerDelegate : public content::DevToolsManagerDelegate {
3944
bool HasBundledFrontendResources() override;
4045
bool ShouldUseBundledFrontendResources() override;
4146
content::BrowserContext* GetDefaultBrowserContext() override;
47+
48+
private:
49+
// Channels with an uncleared Emulation.setDeviceMetricsOverride.
50+
base::flat_set<content::DevToolsAgentHostClientChannel*>
51+
channels_with_device_overrides_;
4252
};
4353

4454
} // namespace electron

‎spec/api-debugger-spec.ts‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import * as http from 'node:http';
77
import * as path from 'node:path';
88

99
import { emittedUntil } from './lib/events-helpers';
10-
import { listen } from './lib/spec-helpers';
10+
import { listen, waitUntil } from './lib/spec-helpers';
1111
import { closeAllWindows } from './lib/window-helpers';
1212

1313
describe('debugger module', () => {
@@ -65,6 +65,25 @@ describe('debugger module', () => {
6565
expect(w.webContents.debugger.isAttached()).to.be.false();
6666
expect(w.devToolsWebContents.isDestroyed()).to.be.false();
6767
});
68+
69+
it('clears device metrics overrides left behind by the session', async () => {
70+
await w.webContents.loadURL('about:blank');
71+
const innerSize = () => w.webContents.executeJavaScript('window.innerWidth + "x" + window.innerHeight');
72+
73+
w.webContents.debugger.attach();
74+
await w.webContents.debugger.sendCommand('Emulation.setDeviceMetricsOverride', {
75+
width: 200,
76+
height: 150,
77+
deviceScaleFactor: 0,
78+
mobile: false
79+
});
80+
expect(await innerSize()).to.equal('200x150');
81+
82+
// Detaching without Emulation.clearDeviceMetricsOverride used to leave
83+
// the view pinned at the emulated size.
84+
w.webContents.debugger.detach();
85+
await waitUntil(async () => (await innerSize()) !== '200x150');
86+
});
6887
});
6988

7089
describe('debugger.sendCommand', () => {

‎spec/chromium-spec.ts‎

Lines changed: 103 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {
1414
} from 'electron/main';
1515

1616
import { expect } from 'chai';
17-
import * as ws from 'ws';
17+
import WebSocketClient = require('ws');
1818

1919
import * as ChildProcess from 'node:child_process';
2020
import { EventEmitter, once } from 'node:events';
@@ -773,6 +773,107 @@ describe('command line switches', () => {
773773
);
774774
}
775775
});
776+
777+
it('clears device metrics overrides when a client disconnects without detaching', async function () {
778+
// A client dying without clearing its overrides used to leave the page
779+
// pinned at the emulated size forever.
780+
const appPath = path.join(fixturesPath, 'apps', 'remote-debugging-emulation');
781+
appProcess = ChildProcess.spawn(process.execPath, [appPath, '--remote-debugging-port=0']);
782+
783+
let stderr = '';
784+
const browserWsUrl = await new Promise<string>((resolve, reject) => {
785+
appProcess!.stderr.on('data', (data: Buffer) => {
786+
stderr += data.toString();
787+
const m = /DevTools listening on (ws:\/\/\S+)/.exec(stderr);
788+
if (m) {
789+
appProcess!.stderr.removeAllListeners('data');
790+
resolve(m[1]);
791+
}
792+
});
793+
appProcess!.on('exit', () =>
794+
reject(new Error(`Process exited before DevTools URL was found. stderr: ${stderr}`))
795+
);
796+
});
797+
798+
type Client = {
799+
socket: WebSocketClient;
800+
send(method: string, params?: unknown, sessionId?: string): Promise<any>;
801+
attachToPage(): Promise<string>;
802+
};
803+
const connectClient = async (): Promise<Client> => {
804+
const socket = new WebSocketClient(browserWsUrl);
805+
await once(socket, 'open');
806+
let nextId = 1;
807+
const pending = new Map<number, { resolve: (result: any) => void; reject: (error: Error) => void }>();
808+
socket.on('message', (data: WebSocketClient.Data) => {
809+
const message = JSON.parse(data.toString());
810+
const handler = message.id && pending.get(message.id);
811+
if (handler) {
812+
pending.delete(message.id);
813+
if (message.error) handler.reject(new Error(message.error.message));
814+
else handler.resolve(message.result);
815+
}
816+
});
817+
const failPending = (why: string) => {
818+
for (const handler of pending.values()) handler.reject(new Error(why));
819+
pending.clear();
820+
};
821+
socket.on('error', (error: Error) => failPending(`websocket error: ${error.message}`));
822+
socket.on('close', () => failPending('websocket closed'));
823+
const send = (method: string, params: unknown = {}, sessionId?: string) =>
824+
new Promise<any>((resolve, reject) => {
825+
const id = nextId++;
826+
pending.set(id, { resolve, reject });
827+
socket.send(JSON.stringify({ id, method, params, sessionId }));
828+
});
829+
const attachToPage = async () => {
830+
// The window may not exist yet when the DevTools server comes up.
831+
let page: any;
832+
await waitUntil(async () => {
833+
const { targetInfos } = await send('Target.getTargets');
834+
page = targetInfos.find((target: any) => target.type === 'page');
835+
return page !== undefined;
836+
});
837+
const { sessionId } = await send('Target.attachToTarget', { targetId: page.targetId, flatten: true });
838+
return sessionId;
839+
};
840+
return { socket, send, attachToPage };
841+
};
842+
const innerSize = async (client: Client, sessionId: string) => {
843+
const { result } = await client.send(
844+
'Runtime.evaluate',
845+
{
846+
expression: 'window.innerWidth + "x" + window.innerHeight',
847+
returnByValue: true
848+
},
849+
sessionId
850+
);
851+
return result.value;
852+
};
853+
854+
const clientA = await connectClient();
855+
const sessionA = await clientA.attachToPage();
856+
const originalSize = await innerSize(clientA, sessionA);
857+
await clientA.send(
858+
'Emulation.setDeviceMetricsOverride',
859+
{
860+
width: 800,
861+
height: 450,
862+
deviceScaleFactor: 0,
863+
mobile: false
864+
},
865+
sessionA
866+
);
867+
expect(await innerSize(clientA, sessionA)).to.equal('800x450');
868+
869+
// Drop the TCP connection like a killed client process would.
870+
(clientA.socket as any)._socket.destroy();
871+
872+
const clientB = await connectClient();
873+
const sessionB = await clientB.attachToPage();
874+
await waitUntil(async () => (await innerSize(clientB, sessionB)) === originalSize);
875+
clientB.socket.close();
876+
});
776877
});
777878

778879
describe('--trace-startup switch', () => {
@@ -3474,7 +3575,7 @@ describe('chromium features', () => {
34743575
const server = http.createServer();
34753576
defer(() => server.close());
34763577
const { port } = await listen(server);
3477-
const wss = new ws.Server({ server });
3578+
const wss = new WebSocketClient.Server({ server });
34783579
const finished = new Promise<string | undefined>((resolve, reject) => {
34793580
wss.on('error', reject);
34803581
wss.on('connection', (ws, upgradeReq) => {

‎spec/fixtures/apps/remote-debugging-emulation/main.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
const { app, BrowserWindow, Menu } = require('electron');
2+
3+
app.whenReady().then(() => {
4+
// No menu bar, so the web contents size is a stable test baseline.
5+
Menu.setApplicationMenu(null);
6+
const win = new BrowserWindow({ width: 600, height: 400, show: true });
7+
win.loadURL('data:text/html,<title>remote-debugging-emulation</title>');
8+
});
9+
10+
app.on('window-all-closed', () => app.quit());

‎spec/fixtures/apps/remote-debugging-emulation/package.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"name": "remote-debugging-emulation",
3+
"main": "main.js"
4+
}

0 commit comments

Comments
 (0)