Skip to content

Commit

Permalink
fix: don't emit frame-created during cross-origin navigation
Browse files Browse the repository at this point in the history
  • Loading branch information
samuelmaddock committed Aug 24, 2021
1 parent 406b99c commit 84ee16e
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 16 deletions.
27 changes: 22 additions & 5 deletions shell/browser/api/electron_api_web_contents.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1393,11 +1393,28 @@ void WebContents::RenderFrameCreated(
content::RenderFrameHost* render_frame_host) {
HandleNewRenderFrame(render_frame_host);

v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
v8::HandleScope handle_scope(isolate);
gin_helper::Dictionary details = gin_helper::Dictionary::CreateEmpty(isolate);
details.SetGetter("frame", render_frame_host);
Emit("frame-created", details);
// RenderFrameCreated is called for speculative frames which may not be
// used in certain cross-origin navigations. Invoking
// RenderFrameHost::GetLifecycleState currently crashes when called for
// speculative frames so we need to filter it out for now. Check
// https://crbug.com/1183639 for details on when this can be removed.
auto* rfh_impl =
static_cast<content::RenderFrameHostImpl*>(render_frame_host);
if (rfh_impl->lifecycle_state() ==
content::RenderFrameHostImpl::LifecycleStateImpl::kSpeculative) {
return;
}

content::RenderFrameHost::LifecycleState lifecycle_state =
render_frame_host->GetLifecycleState();
if (lifecycle_state == content::RenderFrameHost::LifecycleState::kActive) {
v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
v8::HandleScope handle_scope(isolate);
gin_helper::Dictionary details =
gin_helper::Dictionary::CreateEmpty(isolate);
details.SetGetter("frame", render_frame_host);
Emit("frame-created", details);
}
}

void WebContents::RenderFrameDeleted(
Expand Down
24 changes: 13 additions & 11 deletions spec-main/api-web-frame-main-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -263,23 +263,25 @@ describe('webFrameMain module', () => {
expect(nestedDetails.frame).to.equal(w.webContents.mainFrame.frames[0]);
});

it('emits after navigating cross-domain', async () => {
const [serverA, serverB] = await Promise.all([createServer(), createServer()]);
it('is not emitted upon cross-origin navigation', async () => {
const server = await createServer();

// HACK: Use 'localhost' instead of '127.0.0.1' so Chromium treats it as
// a separate origin because differing ports aren't enough :thinking:
const secondUrl = `http://localhost:${new URL(serverB.url).port}`;
// a separate origin because differing ports aren't enough 🤔
const secondUrl = `http://localhost:${new URL(server.url).port}`;

const w = new BrowserWindow({ show: false });
await w.webContents.loadURL(serverA.url);
await w.webContents.loadURL(server.url);

let frameCreatedEmitted = false;

w.webContents.once('frame-created', () => {
frameCreatedEmitted = true;
});

// NOTE: Second navigation is cross-origin which leads to Chromium
// swapping RenderFrameHosts. This is maybe not what we want for this
// API...
const promise = emittedOnce(w.webContents, 'frame-created');
await w.webContents.loadURL(secondUrl);
const [, mainDetails] = await promise;
expect(mainDetails.frame).to.equal(w.webContents.mainFrame);

expect(frameCreatedEmitted).to.be.false();
});
});

Expand Down

0 comments on commit 84ee16e

Please sign in to comment.