Skip to content

Commit edff136

Browse files
sadym-chromiumOrKoN
sadym-chromium
and
OrKoN
authored
fix: do not override context config with undefined (#3633)
Addressing Puppeteer failures: https://github.com/puppeteer/puppeteer/actions/runs/16803966784/job/47591715262. --------- Co-authored-by: Alex Rudenko <[email protected]>
1 parent b9d4201 commit edff136

File tree

5 files changed

+266
-47
lines changed

5 files changed

+266
-47
lines changed

‎GEMINI.md

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,30 @@ This file provides context for the Gemini AI code assistant.
1414

1515
To restore the project to a known good state, run the following commands in order:
1616

17-
1. **Build:** `npm run build`
18-
2. **Unit Tests:** `npm run unit`
19-
3. **Format:** `npm run format`
20-
4. **Verify BidiMapper Import:** `node lib/esm/bidiMapper/BidiMapper.js`. If this
21-
fails with `ERR_MODULE_NOT_FOUND`, it's likely due to a missing `.js`
22-
extension in an import statement in one of the TypeScript source files.
17+
1. **Build:** `npm run build`
18+
2. **Unit Tests:** `npm run unit`
19+
3. **Format:** `npm run format`
20+
4. **Verify BidiMapper Import:** `node lib/esm/bidiMapper/BidiMapper.js`. If this
21+
fails with `ERR_MODULE_NOT_FOUND`, it's likely due to a missing `.js`
22+
extension in an import statement in one of the TypeScript source files.
2323

2424
Do not commit, pull, or push unless explicitly asked.
2525

26+
### Unit Tests
27+
28+
- Unit test source files are located in the `src` directory and have a `.spec.ts`
29+
extension.
30+
- They are co-located with the code they are testing.
31+
- Run unit tests with `npm run unit`. This command first compiles the TypeScript code
32+
(including tests) into JavaScript in the `lib` directory and then runs the tests
33+
using Mocha.
34+
- When adding new tests, create a new `*.spec.ts` file in the `src` directory next to
35+
the file you are testing. The build process will automatically pick it up.
36+
- **Important**: Do not delete the test files you create. They are a part of the
37+
project.
38+
- Use constants for test data like user context or browsing context IDs to improve
39+
readability and maintainability.
40+
2641
### E2E Tests
2742

2843
To run a specific E2E test, use the following command:
@@ -36,6 +51,7 @@ To debug a failing E2E test, try running it with different environment variables
3651
- Set `HEADLESS` to `true`, `false`, or `old`.
3752
- Set `CHROMEDRIVER` to `true` or `false`.
3853

39-
Each run should be done with `VERBOSE=true`. Inspect the latest log in the `logs` directory for errors.
54+
Each run should be done with `VERBOSE=true`. Inspect the latest log in the `logs`
55+
directory for errors.
4056

4157
Note: E2E tests are slow, so run only the necessary tests.

‎src/bidiMapper/modules/browser/BrowserProcessor.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,7 @@ export class BrowserProcessor {
6969
const w3cParams = params as Browser.CreateUserContextParameters;
7070

7171
if (w3cParams.acceptInsecureCerts !== undefined) {
72-
const globalConfig = this.#configStorage.getActiveConfig(
73-
undefined,
74-
undefined,
75-
);
72+
const globalConfig = this.#configStorage.getGlobalConfig();
7673
if (
7774
w3cParams.acceptInsecureCerts === false &&
7875
globalConfig.acceptInsecureCerts === true

‎src/bidiMapper/modules/browser/ContextConfig.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ import type {
2525

2626
/**
2727
* Represents a context configurations. It can be global, per User Context, or per
28-
* Browsing Context.
28+
* Browsing Context. The undefined value means the config will be taken from the upstream
29+
* config. `null` values means the value should be default regardless of the upstream.
2930
*/
3031
export class ContextConfig {
3132
acceptInsecureCerts?: boolean;
@@ -43,4 +44,27 @@ export class ContextConfig {
4344
// Timezone is kept in CDP format with GMT prefix for offset values.
4445
timezone?: string | null;
4546
userPromptHandler?: Session.UserPromptHandler;
47+
48+
/**
49+
* Merges multiple `ContextConfig` objects. The configs are merged in the
50+
* order they are provided. For each property, the value from the last config
51+
* that defines it (i.e., the value is not `undefined`) will be used.
52+
* The final result will not contain any `undefined` properties.
53+
*/
54+
static merge(...configs: (ContextConfig | undefined)[]): ContextConfig {
55+
const result = new ContextConfig();
56+
57+
for (const config of configs) {
58+
if (!config) {
59+
continue;
60+
}
61+
for (const key in config) {
62+
const value = config[key as keyof ContextConfig];
63+
if (value !== undefined) {
64+
(result as any)[key] = value;
65+
}
66+
}
67+
}
68+
return result;
69+
}
4670
}

‎src/bidiMapper/modules/browser/ContextConfigStorage.spec.ts

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
/*
2+
* Copyright 2025 Google LLC.
3+
* Copyright (c) Microsoft Corporation.
4+
*
5+
* Licensed under the Apache License, Version 2.0 (the "License");
6+
* you may not use this file except in compliance with the License.
7+
* You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
18+
import {expect} from 'chai';
19+
20+
import {ContextConfig} from './ContextConfig.js';
21+
import {ContextConfigStorage} from './ContextConfigStorage.js';
22+
23+
describe('ContextConfigStorage', () => {
24+
const USER_CONTEXT = 'some user context';
25+
const BROWSER_CONTEXT = 'some browsing context';
26+
27+
let storage: ContextConfigStorage;
28+
29+
beforeEach(() => {
30+
storage = new ContextConfigStorage();
31+
});
32+
33+
describe('getActiveConfig', () => {
34+
// Test cases for various configuration scenarios.
35+
[
36+
// Global config only.
37+
{
38+
global: {acceptInsecureCerts: true, prerenderingDisabled: true},
39+
expected: {acceptInsecureCerts: true, prerenderingDisabled: true},
40+
name: 'should return the global config if no other configs are set',
41+
},
42+
// User context overrides global.
43+
{
44+
global: {acceptInsecureCerts: true, prerenderingDisabled: true},
45+
user: {acceptInsecureCerts: false},
46+
expected: {acceptInsecureCerts: false, prerenderingDisabled: true},
47+
name: 'should override global config with user context config',
48+
},
49+
// Browsing context overrides global and user context.
50+
{
51+
global: {acceptInsecureCerts: false, prerenderingDisabled: true},
52+
user: {acceptInsecureCerts: false, prerenderingDisabled: false},
53+
browsing: {acceptInsecureCerts: true},
54+
expected: {acceptInsecureCerts: true, prerenderingDisabled: false},
55+
name: 'should override global and user context configs with browsing context config',
56+
},
57+
// Undefined in user context does not override global.
58+
{
59+
global: {acceptInsecureCerts: true, prerenderingDisabled: true},
60+
user: {acceptInsecureCerts: undefined, prerenderingDisabled: false},
61+
expected: {acceptInsecureCerts: true, prerenderingDisabled: false},
62+
name: 'should not override with undefined from user context config',
63+
},
64+
// Undefined in browsing context does not override user context.
65+
{
66+
global: {acceptInsecureCerts: true, prerenderingDisabled: true},
67+
user: {acceptInsecureCerts: false, prerenderingDisabled: false},
68+
browsing: {acceptInsecureCerts: undefined, prerenderingDisabled: true},
69+
expected: {acceptInsecureCerts: false, prerenderingDisabled: true},
70+
name: 'should not override with undefined from browsing context config',
71+
},
72+
// Undefined in both user and browsing context does not override global.
73+
{
74+
global: {acceptInsecureCerts: true, prerenderingDisabled: true},
75+
user: {acceptInsecureCerts: undefined, prerenderingDisabled: false},
76+
browsing: {
77+
acceptInsecureCerts: undefined,
78+
prerenderingDisabled: undefined,
79+
},
80+
expected: {acceptInsecureCerts: true, prerenderingDisabled: false},
81+
name: 'should not override with undefined from either user or browsing context config',
82+
},
83+
// Sequential updates to global context.
84+
{
85+
global: [{acceptInsecureCerts: true}, {acceptInsecureCerts: undefined}],
86+
expected: {acceptInsecureCerts: true},
87+
name: 'should ignore undefined when updating global config',
88+
},
89+
// Sequential updates to user context.
90+
{
91+
global: {acceptInsecureCerts: true},
92+
user: [{acceptInsecureCerts: false}, {acceptInsecureCerts: undefined}],
93+
expected: {acceptInsecureCerts: false},
94+
name: 'should ignore undefined when updating user context config',
95+
},
96+
// Sequential updates to browsing context.
97+
{
98+
global: {acceptInsecureCerts: true},
99+
user: {acceptInsecureCerts: false},
100+
browsing: [
101+
{acceptInsecureCerts: true},
102+
{acceptInsecureCerts: undefined},
103+
],
104+
expected: {acceptInsecureCerts: true},
105+
name: 'should ignore undefined when updating browsing context config',
106+
},
107+
// Null in user context overrides global.
108+
{
109+
global: {viewport: {width: 1, height: 1}},
110+
user: {viewport: null},
111+
expected: {viewport: null},
112+
name: 'should override with null from user context config',
113+
},
114+
// Null in browsing context overrides user context.
115+
{
116+
global: {viewport: {width: 1, height: 1}},
117+
user: {viewport: {width: 2, height: 2}},
118+
browsing: {viewport: null},
119+
expected: {viewport: null},
120+
name: 'should override with null from browsing context config',
121+
},
122+
// Value in browsing context overrides null in user context.
123+
{
124+
global: {viewport: {width: 1, height: 1}},
125+
user: {viewport: null},
126+
browsing: {viewport: {width: 3, height: 3}},
127+
expected: {viewport: {width: 3, height: 3}},
128+
name: 'should override null with value from browsing context config',
129+
},
130+
].forEach(({name, global, user, browsing, expected}) => {
131+
it(name, () => {
132+
if (global) {
133+
for (const config of Array.isArray(global) ? global : [global]) {
134+
storage.updateGlobalConfig(config);
135+
}
136+
}
137+
if (user) {
138+
for (const config of Array.isArray(user) ? user : [user]) {
139+
storage.updateUserContextConfig(USER_CONTEXT, config);
140+
}
141+
}
142+
if (browsing) {
143+
for (const config of Array.isArray(browsing)
144+
? browsing
145+
: [browsing]) {
146+
storage.updateBrowsingContextConfig(BROWSER_CONTEXT, config);
147+
}
148+
}
149+
150+
const activeConfig = storage.getActiveConfig(
151+
BROWSER_CONTEXT,
152+
USER_CONTEXT,
153+
);
154+
155+
const expectedConfig = new ContextConfig();
156+
Object.assign(expectedConfig, expected);
157+
158+
expect(activeConfig).to.deep.equal(expectedConfig);
159+
});
160+
});
161+
});
162+
});

‎src/bidiMapper/modules/browser/ContextConfigStorage.ts

Lines changed: 55 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -17,57 +17,77 @@
1717

1818
import {ContextConfig} from './ContextConfig.js';
1919

20+
/**
21+
* Manages context-specific configurations. This class allows setting
22+
* configurations at three levels: global, user context, and browsing context.
23+
*
24+
* When `getActiveConfig` is called, it merges the configurations in a specific
25+
* order of precedence: `global -> user context -> browsing context`. For each
26+
* configuration property, the value from the highest-precedence level that has a
27+
* non-`undefined` value is used.
28+
*
29+
* The `update` methods (`updateGlobalConfig`, `updateUserContextConfig`,
30+
* `updateBrowsingContextConfig`) merge the provided configuration with the
31+
* existing one at the corresponding level. Properties with `undefined` values in
32+
* the provided configuration are ignored, preserving the existing value.
33+
*/
2034
export class ContextConfigStorage {
2135
#global = new ContextConfig();
2236
#userContextConfigs = new Map<string, ContextConfig>();
2337
#browsingContextConfigs = new Map<string, ContextConfig>();
2438

39+
/**
40+
* Updates the global configuration. Properties with `undefined` values in the
41+
* provided `config` are ignored.
42+
*/
2543
updateGlobalConfig(config: ContextConfig) {
26-
this.#global = {...this.#global, ...config};
44+
this.#global = ContextConfig.merge(this.#global, config);
2745
}
2846

47+
/**
48+
* Updates the configuration for a specific browsing context. Properties with
49+
* `undefined` values in the provided `config` are ignored.
50+
*/
2951
updateBrowsingContextConfig(
3052
browsingContextId: string,
3153
config: ContextConfig,
3254
) {
33-
if (this.#browsingContextConfigs.has(browsingContextId)) {
34-
this.#browsingContextConfigs.set(browsingContextId, {
35-
...this.#browsingContextConfigs.get(browsingContextId),
36-
...config,
37-
});
38-
} else {
39-
this.#browsingContextConfigs.set(browsingContextId, config);
40-
}
55+
this.#browsingContextConfigs.set(
56+
browsingContextId,
57+
ContextConfig.merge(
58+
this.#browsingContextConfigs.get(browsingContextId),
59+
config,
60+
),
61+
);
4162
}
4263

64+
/**
65+
* Updates the configuration for a specific user context. Properties with
66+
* `undefined` values in the provided `config` are ignored.
67+
*/
4368
updateUserContextConfig(userContext: string, config: ContextConfig) {
44-
if (this.#userContextConfigs.has(userContext)) {
45-
this.#userContextConfigs.set(userContext, {
46-
...this.#userContextConfigs.get(userContext),
47-
...config,
48-
});
49-
} else {
50-
this.#userContextConfigs.set(userContext, config);
51-
}
69+
this.#userContextConfigs.set(
70+
userContext,
71+
ContextConfig.merge(this.#userContextConfigs.get(userContext), config),
72+
);
73+
}
74+
75+
/**
76+
* Returns the current global configuration.
77+
*/
78+
getGlobalConfig(): ContextConfig {
79+
return this.#global;
5280
}
5381

54-
getActiveConfig(topLevelBrowsingContextId?: string, userContext?: string) {
55-
let result = {...this.#global};
56-
if (
57-
userContext !== undefined &&
58-
this.#userContextConfigs.has(userContext)
59-
) {
60-
result = {...result, ...this.#userContextConfigs.get(userContext)};
61-
}
62-
if (
63-
topLevelBrowsingContextId !== undefined &&
64-
this.#browsingContextConfigs.has(topLevelBrowsingContextId)
65-
) {
66-
result = {
67-
...result,
68-
...this.#browsingContextConfigs.get(topLevelBrowsingContextId),
69-
};
70-
}
71-
return result;
82+
/**
83+
* Calculates the active configuration by merging global, user context, and
84+
* browsing context settings.
85+
*/
86+
getActiveConfig(topLevelBrowsingContextId: string, userContext: string) {
87+
return ContextConfig.merge(
88+
this.#global,
89+
this.#userContextConfigs.get(userContext),
90+
this.#browsingContextConfigs.get(topLevelBrowsingContextId),
91+
);
7292
}
7393
}

0 commit comments

Comments
 (0)