Skip to content

Commit

Permalink
fix(oopifs): account for various races between processes (#5320)
Browse files Browse the repository at this point in the history
Consider the following situation (one among many possible).
- FrameA has an oopif child FrameB;
- FrameA navigates to same-process origin (e.g. about:blank);
- at the same time, FrameC is attached to the FrameB in the
  FrameB's process.

In this case, we get `frameNavigated` event for FrameA, immediately
followed by `frameAttached` event for FrameC. Since we detach all
FrameA's child frames on navigation, including the oopif FrameB,
there is no parent frame for FrameC to attach to.

In general, multiple processes coming from oopif may send their
events in wildly different order, and their view about the frame
tree may not always correspond to the "up to date" frame tree as
seen from the main frame's process. We try to keep our frame tree
aligned with what main process thinks, and ignore events that
reference frames absent in this tree.

Drive-by: handle filechooser exceptions because of async processing.
  • Loading branch information
dgozman committed Feb 5, 2021
1 parent 494f0f6 commit de30ee0
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 11 deletions.
37 changes: 30 additions & 7 deletions src/server/chromium/crPage.ts
Expand Up @@ -532,14 +532,25 @@ class FrameSession {
if (frameSession && frameId !== this._targetId) {
// This is a remote -> local frame transition.
frameSession._swappedIn = true;
const frame = this._page._frameManager.frame(frameId)!;
this._page._frameManager.removeChildFramesRecursively(frame);
const frame = this._page._frameManager.frame(frameId);
// Frame or even a whole subtree may be already gone, because some ancestor did navigate.
if (frame)
this._page._frameManager.removeChildFramesRecursively(frame);
return;
}
if (parentFrameId && !this._page._frameManager.frame(parentFrameId)) {
// Parent frame may be gone already because some ancestor frame navigated and
// destroyed the whole subtree of some oopif, while oopif's process is still sending us events.
// Be careful to not confuse this with "main frame navigated cross-process" scenario
// where parentFrameId is null.
return;
}
this._page._frameManager.frameAttached(frameId, parentFrameId);
}

_onFrameNavigated(framePayload: Protocol.Page.Frame, initial: boolean) {
if (!this._page._frameManager.frame(framePayload.id))
return; // Subtree may be already gone because some ancestor navigation destroyed the oopif.
this._page._frameManager.frameCommittedNewDocumentNavigation(framePayload.id, framePayload.url + (framePayload.urlFragment || ''), framePayload.name || '', framePayload.loaderId, initial);
if (!initial)
this._firstNonInitialNavigationCommittedFulfill();
Expand Down Expand Up @@ -609,7 +620,9 @@ class FrameSession {
if (event.targetInfo.type === 'iframe') {
// Frame id equals target id.
const targetId = event.targetInfo.targetId;
const frame = this._page._frameManager.frame(targetId)!;
const frame = this._page._frameManager.frame(targetId);
if (!frame)
return; // Subtree may be already gone due to renderer/browser race.
this._page._frameManager.removeChildFramesRecursively(frame);
const frameSession = new FrameSession(this._crPage, session, targetId, this);
this._crPage._sessions.set(targetId, frameSession);
Expand Down Expand Up @@ -715,6 +728,8 @@ class FrameSession {
}

_onDialog(event: Protocol.Page.javascriptDialogOpeningPayload) {
if (!this._page._frameManager.frame(this._targetId))
return; // Our frame/subtree may be gone already.
this._page.emit(Page.Events.Dialog, new dialog.Dialog(
this._page,
event.type,
Expand Down Expand Up @@ -749,10 +764,18 @@ class FrameSession {
}

async _onFileChooserOpened(event: Protocol.Page.fileChooserOpenedPayload) {
const frame = this._page._frameManager.frame(event.frameId)!;
const utilityContext = await frame._utilityContext();
const handle = await this._adoptBackendNodeId(event.backendNodeId, utilityContext);
this._page._onFileChooserOpened(handle);
const frame = this._page._frameManager.frame(event.frameId);
if (!frame)
return;
let handle;
try {
const utilityContext = await frame._utilityContext();
handle = await this._adoptBackendNodeId(event.backendNodeId, utilityContext);
} catch (e) {
// During async processing, frame/context may go away. We should not throw.
return;
}
await this._page._onFileChooserOpened(handle);
}

_onDownloadWillBegin(payload: Protocol.Page.downloadWillBeginPayload) {
Expand Down
8 changes: 7 additions & 1 deletion src/server/page.ts
Expand Up @@ -226,7 +226,13 @@ export class Page extends EventEmitter {
}

async _onFileChooserOpened(handle: dom.ElementHandle) {
const multiple = await handle.evaluate(element => !!(element as HTMLInputElement).multiple);
let multiple;
try {
multiple = await handle.evaluate(element => !!(element as HTMLInputElement).multiple);
} catch (e) {
// Frame/context may be gone during async processing. Do not throw.
return;
}
if (!this.listenerCount(Page.Events.FileChooser)) {
handle.dispose();
return;
Expand Down
12 changes: 9 additions & 3 deletions src/server/webkit/wkPage.ts
Expand Up @@ -559,9 +559,15 @@ export class WKPage implements PageDelegate {
}

private async _onFileChooserOpened(event: {frameId: Protocol.Network.FrameId, element: Protocol.Runtime.RemoteObject}) {
const context = await this._page._frameManager.frame(event.frameId)!._mainContext();
const handle = context.createHandle(event.element).asElement()!;
this._page._onFileChooserOpened(handle);
let handle;
try {
const context = await this._page._frameManager.frame(event.frameId)!._mainContext();
handle = context.createHandle(event.element).asElement()!;
} catch (e) {
// During async processing, frame/context may go away. We should not throw.
return;
}
await this._page._onFileChooserOpened(handle);
}

private static async _setEmulateMedia(session: WKSession, mediaType: types.MediaType | null, colorScheme: types.ColorScheme | null): Promise<void> {
Expand Down
44 changes: 44 additions & 0 deletions test/page-set-input-files.spec.ts
Expand Up @@ -115,6 +115,50 @@ it('should work when file input is not attached to DOM', async ({page, server})
expect(chooser).toBeTruthy();
});

it('should not throw when filechooser belongs to iframe', (test, { browserName }) => {
test.skip(browserName === 'firefox', 'Firefox ignores filechooser from child frame');
}, async ({page, server}) => {
await page.goto(server.PREFIX + '/frames/one-frame.html');
const frame = page.mainFrame().childFrames()[0];
await frame.setContent(`
<div>Click me</div>
<script>
document.querySelector('div').addEventListener('click', () => {
const input = document.createElement('input');
input.type = 'file';
input.click();
window.parent.__done = true;
});
</script>
`);
await Promise.all([
page.waitForEvent('filechooser'),
frame.click('div')
]);
await page.waitForFunction(() => (window as any).__done);
});

it('should not throw when frame is detached immediately', async ({page, server}) => {
await page.goto(server.PREFIX + '/frames/one-frame.html');
const frame = page.mainFrame().childFrames()[0];
await frame.setContent(`
<div>Click me</div>
<script>
document.querySelector('div').addEventListener('click', () => {
const input = document.createElement('input');
input.type = 'file';
input.click();
window.parent.__done = true;
const iframe = window.parent.document.querySelector('iframe');
iframe.remove();
});
</script>
`);
page.on('filechooser', () => {}); // To ensure we handle file choosers.
await frame.click('div');
await page.waitForFunction(() => (window as any).__done);
});

it('should work with CSP', async ({page, server}) => {
server.setCSP('/empty.html', 'default-src "none"');
await page.goto(server.EMPTY_PAGE);
Expand Down

0 comments on commit de30ee0

Please sign in to comment.