Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Fix window.opener being null when nativeWindowOpen is used #9961

Merged
merged 3 commits into from
Aug 4, 2017
Merged

Fix window.opener being null when nativeWindowOpen is used #9961

merged 3 commits into from
Aug 4, 2017

Conversation

MarshallOfSound
Copy link
Member

Fixes #9581 #8100 #9533

Not 100% sure if this is the correct fix but (a) it works and (b) it looks right to me 😆

/cc @zcbenz @kevinsawicki Can you make sure this isn't doing something silly

@MarshallOfSound
Copy link
Member Author

This appears to cause the renderer process to crash after the second navigation. This is due to the webContents launched from window.open not maintaining the nativeWindowOpen: true property resulting in the above check being false 😕 Exploring how to detect this atm

@MarshallOfSound
Copy link
Member Author

Two additional things here.

  • Child windows now inherit the nativeWindowOpen setting of the opener
  • We now check directly against the web contents as at RenderProcessWillLaunch the web contents isn't actually ready so the webPreferences we save are just the defaults every time

@MarshallOfSound
Copy link
Member Author

The newly opened window appears to crash when switching domains in some cases, can't figure out why. If anyone has any ideas would be appreciated 👍

@gerges-zz
Copy link

gerges-zz commented Jul 9, 2017 •
edited
Loading

Haven't confirmed, but possibly related to if window.open is called directly from a <webview> or from an <iframe> inside a <webview>

@kevinsawicki
Copy link
Contributor

Child windows now inherit the nativeWindowOpen setting of the opener

Yeah, I was looking into this a bit as well via #9928, I think we need to get all the preferences supported that can overridden via new-window so that they properly take effect if set via that event, right now I believe any overrides are ignored when it is triggered via -add-new-contents.

@kevinsawicki
Copy link
Contributor

The newly opened window appears to crash when switching domains in some cases, can't figure out why.

Do you have the stack trace for this crash?

@MarshallOfSound
Copy link
Member Author

Do you have the stack trace for this crash?

@kevinsawicki As much as I try I can't get Electron to dump a crash stack on me. The renderer window just closes by itself just after a page navigation is started. Repro is as easy as window.open('https://www.google.com') then just search around and go to a different site, it should crash.

This was referenced Jul 10, 2017
Merged
Merged
@MarshallOfSound
Copy link
Member Author

Just figured out how to stop the random crash on navigation in the opened window

Pinging @electron/maintainers this is a pretty important fix for window.open compatibility, would be good to get some 👀 on it and hopefully some ✔️

poiru
poiru reviewed Jul 30, 2017
View reviewed changes
atom/browser/atom_browser_client.cc Outdated
// reuse the same site to allow cross-window scripting. We do
// not need to check urls / domains as native window open logic
// handles cross site scripting protection.
if (WebContentsPreferences::UsesNativeWindowOpen(web_contents)) {
Copy link
Contributor

@poiru poiru Jul 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like this is effectively if (true) since we already checked for if (!WebContentsPreferences::UsesNativeWindowOpen(web_contents)) above. Could you remove the unnecessary check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically yeah, thought it would be good to be explicit in code though. I'll change it to still be explicit but not fetch the preferences twice 👍

malept
malept reviewed Jul 31, 2017
View reviewed changes
atom/browser/atom_browser_client.cc Outdated
//
// NOTE: We know that nativeWindowOpen is enabled at this point
// because we check if it is NOT enabled above this point. We
// will only reach this return if sanbox is disabled but
Copy link
Member

Choose a reason for hiding this comment

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

sandbox

@zcbenz zcbenz merged commit 63d7aee into master Aug 4, 2017
@zcbenz zcbenz deleted the fix-window-opener branch August 4, 2017 12:18
@zcbenz zcbenz mentioned this pull request Aug 7, 2017
Merged
@alexstrat alexstrat mentioned this pull request Aug 23, 2017
Closed
@igk1972 igk1972 mentioned this pull request Nov 5, 2017
Closed
@PalmerAL PalmerAL mentioned this pull request Nov 5, 2017
Closed
@alexstrat alexstrat mentioned this pull request Jan 17, 2018
Closed
@hugomano hugomano mentioned this pull request Dec 27, 2018
Merged
2 tasks
@alexstrat alexstrat mentioned this pull request May 22, 2019
Merged
5 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@malept malept malept left review comments

@zcbenz zcbenz zcbenz approved these changes

@poiru poiru poiru approved these changes

@kevinsawicki kevinsawicki Awaiting requested review from kevinsawicki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

iframe window.open broken
6 participants
@MarshallOfSound @gerges-zz @kevinsawicki @malept @zcbenz @poiru