Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: window.open doesn't work correctly in child window #25080

Merged
merged 1 commit into from Oct 7, 2020
Merged

fix: window.open doesn't work correctly in child window #25080

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 1 addition & 10 deletions shell/browser/api/electron_api_web_contents.cc
View file
Open in desktop
Expand Up @@ -1065,22 +1065,13 @@ void WebContents::RenderFrameCreated(
rwh_impl->disable_hidden_ = !background_throttling_;
}

void WebContents::RenderViewHostChanged(content::RenderViewHost* old_host,
content::RenderViewHost* new_host) {
currently_committed_process_id_ = new_host->GetProcess()->GetID();
}

void WebContents::RenderViewDeleted(content::RenderViewHost* render_view_host) {
// This event is necessary for tracking any states with respect to
// intermediate render view hosts aka speculative render view hosts. Currently
// used by object-registry.js to ref count remote objects.
Emit("render-view-deleted", render_view_host->GetProcess()->GetID());

if (-1 == currently_committed_process_id_ ||
render_view_host->GetProcess()->GetID() ==
currently_committed_process_id_) {
currently_committed_process_id_ = -1;

if (web_contents()->GetRenderViewHost() == render_view_host) {
deepak1556 marked this conversation as resolved.
Show resolved Hide resolved
// When the RVH that has been deleted is the current RVH it means that the
// the web contents are being closed. This is communicated by this event.
// Currently tracked by guest-window-manager.js to destroy the
Expand Down
6 changes: 0 additions & 6 deletions shell/browser/api/electron_api_web_contents.h
View file
Open in desktop
Expand Up @@ -555,8 +555,6 @@ class WebContents : public gin::Wrappable<WebContents>,
const base::TimeTicks& proceed_time) override;
void RenderViewCreated(content::RenderViewHost* render_view_host) override;
void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
void RenderViewHostChanged(content::RenderViewHost* old_host,
content::RenderViewHost* new_host) override;
void RenderViewDeleted(content::RenderViewHost*) override;
void RenderProcessGone(base::TerminationStatus status) override;
void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
Expand Down Expand Up @@ -695,10 +693,6 @@ class WebContents : public gin::Wrappable<WebContents>,

bool initially_shown_ = true;

// The ID of the process of the currently committed RenderViewHost.
// -1 means no speculative RVH has been committed yet.
int currently_committed_process_id_ = -1;

service_manager::BinderRegistryWithArgs<content::RenderFrameHost*> registry_;
mojo::ReceiverSet<mojom::ElectronBrowser, content::RenderFrameHost*>
receivers_;
Expand Down
25 changes: 25 additions & 0 deletions spec-main/api-web-contents-spec.ts
View file
Open in desktop
Expand Up @@ -1156,6 +1156,10 @@ describe('webContents module', () => {
res.end();
} else if (req.url === '/redirected') {
res.end('<html><script>window.localStorage</script></html>');
} else if (req.url === '/first-window-open') {
res.end(`<html><script>window.open('${serverUrl}/second-window-open', 'first child');</script></html>`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have to open another child window in /second-window-open to repro the issue, wouldn't it repro if you used res.end(<html><script>window.open('${serverUrl}/redirect-cross-site', 'first child');</script></html>); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that and it didn't work (i.e. problem doesn't reproduce). It should reproduce if there would be redirection from http to https (first reproduction and the most probable in normal usage was on http://www.google.com which redirects to https://www.google.com) but to achieve it in the test it would be necessary to start https server with valid certificate next to http server.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the protocol doesn't matter here http://www.google.com/ and https://www.google.com/ are different origins for chromium, it is required for a cross origin redirect to repro this issue. But weird, maybe the testing needs a network delay. Anyways the current test is good, wanted to see if we can simplify it. Thanks!

} else if (req.url === '/second-window-open') {
res.end('<html><script>window.open(\'wrong://url\', \'second child\');</script></html>');
} else {
res.end();
}
Expand Down Expand Up @@ -1192,6 +1196,27 @@ describe('webContents module', () => {
expect(currentRenderViewDeletedEmitted).to.be.false('current-render-view-deleted was emitted');
});

it('does not emit current-render-view-deleted when speculative RVHs are deleted and nativeWindowOpen is set to true', async () => {
const parentWindow = new BrowserWindow({ show: false, webPreferences: { nativeWindowOpen: true } });
let currentRenderViewDeletedEmitted = false;
let childWindow:BrowserWindow;
const destroyed = emittedOnce(parentWindow.webContents, 'destroyed');
const renderViewDeletedHandler = () => {
currentRenderViewDeletedEmitted = true;
};
app.once('browser-window-created', (event, window) => {
childWindow = window;
window.webContents.on('current-render-view-deleted' as any, renderViewDeletedHandler);
});
parentWindow.loadURL(`${serverUrl}/first-window-open`);
setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we listen on navigation events on the childWindow before closing the parentWindow ? In this case it would be did-fail-load since the second url is a invalid url. I am bit worried about setTimeout being a potential for flaky tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked that did-fail-load is not emitted for this url. I've tried to find another event which we could use but with no luck so far.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deepak1556 FWIW... it appears you are right as WOA is now failing on this test fairly regularly:
https://github.visualstudio.com/electron/_build/results?buildId=85266&view=logs&j=12f1170f-54f2-53f3-20dd-22fc7dff55f9&t=5caf77c8-9b10-50ef-b5c7-ca89c63e1c86&l=2139

childWindow.webContents.removeListener('current-render-view-deleted' as any, renderViewDeletedHandler);
parentWindow.close();
}, 500);
await destroyed;
expect(currentRenderViewDeletedEmitted).to.be.false('child window was destroyed');
});

it('emits current-render-view-deleted if the current RVHs are deleted', async () => {
const w = new BrowserWindow({ show: false });
let currentRenderViewDeletedEmitted = false;
Expand Down