Skip to content

Commit

Permalink
fix(chromium): use frameDetached reason (#4468)
Browse files Browse the repository at this point in the history 
This fixes the local -> remote frame swap when
Page.frameDetached arrives before Target.attachedToTarget.

Instead of error-prone logic we do currently, new CDP exposes
frame detach reason that we can use.
  • Loading branch information
@dgozman
dgozman authored Nov 17, 2020
1 parent c1a5cd5 commit 38fadca
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 47 deletions.
56 changes: 17 additions & 39 deletions src/server/chromium/crPage.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ import { VideoRecorder } from './videoRecorder';

const UTILITY_WORLD_NAME = '__playwright_utility_world__';

const swappingOutChildFrames = Symbol('swappingOutChildFrames');

export class CRPage implements PageDelegate {
readonly _mainFrameSession: FrameSession;
readonly _sessions = new Map<Protocol.Target.TargetID, FrameSession>();
Expand Down Expand Up @@ -363,7 +361,7 @@ class FrameSession {
helper.addEventListener(this._client, 'Log.entryAdded', event => this._onLogEntryAdded(event)),
helper.addEventListener(this._client, 'Page.fileChooserOpened', event => this._onFileChooserOpened(event)),
helper.addEventListener(this._client, 'Page.frameAttached', event => this._onFrameAttached(event.frameId, event.parentFrameId)),
helper.addEventListener(this._client, 'Page.frameDetached', event => this._onFrameDetached(event.frameId)),
helper.addEventListener(this._client, 'Page.frameDetached', event => this._onFrameDetached(event.frameId, event.reason)),
helper.addEventListener(this._client, 'Page.frameNavigated', event => this._onFrameNavigated(event.frame, false)),
helper.addEventListener(this._client, 'Page.frameRequestedNavigation', event => this._onFrameRequestedNavigation(event)),
helper.addEventListener(this._client, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)),
Expand Down Expand Up @@ -551,35 +549,23 @@ class FrameSession {
this._page._frameManager.frameCommittedSameDocumentNavigation(frameId, url);
}

private _takeParentForSwappingOutFrame(targetId: string) {
for (const frame of this._page.frames()) {
if (!(frame as any)[swappingOutChildFrames])
continue;
if ((frame as any)[swappingOutChildFrames].delete(targetId))
return frame._id;
}
return null;
}

private _frameMaybeSwappingOut(frameId: string) {
const frame = this._page._frameManager.frame(frameId);
if (!frame)
return;
const parent = frame.parentFrame() as any;
if (!parent)
return;
if (!parent[swappingOutChildFrames])
parent[swappingOutChildFrames] = new Set<string>();
parent[swappingOutChildFrames].add(frameId);
}

_onFrameDetached(frameId: string) {
_onFrameDetached(frameId: string, reason: 'remove' | 'swap') {
if (this._crPage._sessions.has(frameId)) {
// This is a local -> remote frame transtion.
// We already got a new target and handled frame reattach - nothing to do here.
// This is a local -> remote frame transtion, where
// Page.frameDetached arrives after Target.attachedToTarget.
// We've already handled the new target and frame reattach - nothing to do here.
return;
}
if (reason === 'swap') {
// This is a local -> remote frame transtion, where
// Page.frameDetached arrives before Target.attachedToTarget.
// We should keep the frame in the tree, and it will be used for the new target.
const frame = this._page._frameManager.frame(frameId);
if (frame)
this._page._frameManager.removeChildFramesRecursively(frame);
return;
}
this._frameMaybeSwappingOut(frameId);
// Just a regular frame detach.
this._page._frameManager.frameDetached(frameId);
}

Expand Down Expand Up @@ -615,16 +601,8 @@ class FrameSession {
if (event.targetInfo.type === 'iframe') {
// Frame id equals target id.
const targetId = event.targetInfo.targetId;
const frame = this._page._frameManager.frame(targetId);
if (frame) {
this._page._frameManager.removeChildFramesRecursively(frame);
} else {
// There is a race between Page.frameDetached and Target.attachedToTarget. If the frame
// has already been detached we look up its last parent frame.
const parentFrameId = this._takeParentForSwappingOutFrame(targetId);
assert(parentFrameId, 'Cannot find parent for iframe: ' + targetId);
this._page._frameManager.frameAttached(targetId, parentFrameId);
}
const frame = this._page._frameManager.frame(targetId)!;
this._page._frameManager.removeChildFramesRecursively(frame);
const frameSession = new FrameSession(this._crPage, session, targetId, this);
this._crPage._sessions.set(targetId, frameSession);
frameSession._initialize(false).catch(e => e);
Expand Down
51 changes: 46 additions & 5 deletions src/server/chromium/protocol.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1041,7 +1041,7 @@ Note that userVisibleOnly = true is the only currently supported type.
/**
* Browser command ids used by executeBrowserCommand.
*/
export type BrowserCommandId = "openTabSearch";
export type BrowserCommandId = "openTabSearch"|"closeTabSearch";
/**
* Chrome histogram bucket.
*/
Expand Down Expand Up @@ -6866,12 +6866,13 @@ passed by the developer (e.g. via "fetch") as understood by the backend.
export type ServiceWorkerResponseSource = "cache-storage"|"http-cache"|"fallback-code"|"network";
/**
* Determines what type of Trust Token operation is executed and
depending on the type, some additional parameters.
depending on the type, some additional parameters. The values
are specified in third_party/blink/renderer/core/fetch/trust_token.idl.
*/
export interface TrustTokenParams {
type: TrustTokenOperationType;
/**
* Only set for "srr-token-redemption" type and determine whether
* Only set for "token-redemption" type and determine whether
to request a fresh SRR or use a still valid cached SRR.
*/
refreshPolicy: "UseCached"|"Refresh";
Expand Down Expand Up @@ -7410,8 +7411,8 @@ https://wicg.github.io/webpackage/draft-yasskin-httpbis-origin-signed-exchanges-
reportOnlyReportingEndpoint?: string;
}
export interface SecurityIsolationStatus {
coop: CrossOriginOpenerPolicyStatus;
coep: CrossOriginEmbedderPolicyStatus;
coop?: CrossOriginOpenerPolicyStatus;
coep?: CrossOriginEmbedderPolicyStatus;
}
/**
* An object providing the result of a network resource load.
Expand Down Expand Up @@ -8509,6 +8510,36 @@ continueInterceptedRequest call.
*/
gridBackgroundColor?: DOM.RGBA;
}
/**
* Configuration data for the highlighting of Flex container elements.
*/
export interface FlexContainerHighlightConfig {
/**
* The style of the container border
*/
containerBorder?: LineStyle;
/**
* The style of the separator between lines
*/
lineSeparator?: LineStyle;
/**
* The style of the separator between items
*/
itemSeparator?: LineStyle;
}
/**
* Style information for drawing a line.
*/
export interface LineStyle {
/**
* The color of the line (default: transparent)
*/
color?: DOM.RGBA;
/**
* The line pattern (default: solid)
*/
pattern?: "dashed"|"dotted";
}
/**
* Configuration data for the highlighting of page elements.
*/
Expand Down Expand Up @@ -8573,6 +8604,10 @@ continueInterceptedRequest call.
* The grid layout highlight configuration (default: all transparent).
*/
gridHighlightConfig?: GridHighlightConfig;
/**
* The flex container highlight configuration (default: all transparent).
*/
flexContainerHighlightConfig?: FlexContainerHighlightConfig;
}
export type ColorFormat = "rgb"|"hsl"|"hex";
/**
Expand Down Expand Up @@ -8997,6 +9032,7 @@ Backend then generates 'inspectNodeRequested' event upon element selection.
* Indicates whether the frame is cross-origin isolated and why it is the case.
*/
export type CrossOriginIsolatedContextType = "Isolated"|"NotIsolated"|"NotIsolatedFeatureDisabled";
export type GatedAPIFeatures = "SharedArrayBuffers"|"SharedArrayBuffersTransferAllowed"|"PerformanceMeasureMemory"|"PerformanceProfile";
/**
* Information about the Frame on the page.
*/
Expand Down Expand Up @@ -9056,6 +9092,10 @@ Example URLs: http://www.google.com/file.html -> "google.com"
* Indicates whether this is a cross origin isolated context.
*/
crossOriginIsolatedContextType: CrossOriginIsolatedContextType;
/**
* Indicated which gated APIs / features are available.
*/
gatedAPIFeatures: GatedAPIFeatures[];
}
/**
* Information about the Resource on the page.
Expand Down Expand Up @@ -9433,6 +9473,7 @@ Example URLs: http://www.google.com/file.html -> "google.com"
* Id of the frame that has been detached.
*/
frameId: FrameId;
reason: "remove"|"swap";
}
/**
* Fired once navigation of the frame has completed. Frame is now associated with the new loader.
Expand Down
4 changes: 1 addition & 3 deletions utils/protocol-types-generator/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@ async function generateProtocol(name, executablePath) {

async function generateChromiumProtocol(executablePath) {
const outputPath = path.join(__dirname, '../../src/server/chromium/protocol.ts');
process.env.PLAYWRIGHT_CHROMIUM_DEBUG_PORT = '9339';
const playwright = require('../../index').chromium;
const browser = await playwright.launch({ executablePath });
delete process.env.PLAYWRIGHT_CHROMIUM_DEBUG_PORT;
const browser = await playwright.launch({ executablePath, args: ['--remote-debugging-port=9339'] });
const page = await browser.newPage();
await page.goto(`http://localhost:9339/json/protocol`);
const json = JSON.parse(await page.evaluate(() => document.documentElement.innerText));
Expand Down

0 comments on commit 38fadca

Please sign in to comment.