fix: window.open doesn't work correctly in child window #25080
Conversation
|
@MarshallOfSound I've attached sample application to this ticket: #25079. I will try to create unit test for it. |
|
@MarshallOfSound I've pushed fixup with unit test. I couldn't use external page like www.google.com in the test but I noticed the the problem also occurs when we use url with invalid protocol in second call to window.open so I used it in the test. Unfortunately I used timeout as I didn't find a proper way to check if child window was destroyed without using it. If you have an idea how to achieve that please let me know. |
| @@ -1155,6 +1155,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>`); | |||
There was a problem hiding this comment.
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>); ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
@ppontes Can you take a look at this change? The related code was first added in #15821 and I'm not sure if this change would cause side effects.
Thanks for the notification, @zcbenz . I like this approach more as it's simpler and more explicit and can't recall why we didn't use it in the first place. I don't see side effects and the fact that the existing tests aren't broken hopefully confirms that.
| window.webContents.on('current-render-view-deleted' as any, renderViewDeletedHandler); | ||
| }); | ||
| parentWindow.loadURL(`${serverUrl}/first-window-open`); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
If speculative render view host is deleted during naviagation to page we try to open with call to window.open window is destroyed right after it is created. It may happen when naviagation triggers redirect from http to https. To void that we should emit event current-render-view-deleted only when render view host which was destroyed is the current webcontent's rvh.
CezaryKulakowski
force-pushed
the
issue_25079
branch
from
2172d07
to
1f5a697
Compare
|
I've pushed branch rebased to the newest master (there was a conflict in one file so it was necessary) and with all fixups squashed. It should be ready to be merged to master. |
|
@MarshallOfSound test was created. Could you close the change request if it is sufficient for you? |
|
Release Notes Persisted
|
|
I have automatically backported this PR to "11-x-y", please check out #25816 |
Description of Change
If speculative render view host is deleted during naviagation to
page we try to open with call to window.open window is destroyed
right after it is created. It may happen when naviagation triggers
redirect from http to https. To void that we should emit event
current-render-view-deleted only when render view host which was
destroyed is the current webcontent's rvh.
Checklist
Release Notes
Notes: Fixed window.open called from child window