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.

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

:bug: Fix #9231: Don't load url when detached. #9819

Merged
merged 2 commits into from Jul 19, 2017

Conversation

Projects
None yet
2 participants
@ferreus
Contributor

ferreus commented Jun 21, 2017

I tracked down this crash to https://chromium.googlesource.com/experimental/chromium/src/+/58.0.3029.110/content/browser/browser_plugin/browser_plugin_guest.h#153

WebContentsImpl* embedder_web_contents() const {
    return attached_ ? owner_web_contents_ : nullptr;
}

Which results in null pointer access on LoadURL when the view is hidden (Because it's detached).
This is actually not a matter of race condition. As it's simply enough to hide the webview, then issue LoadURL to crash it.

This will avoid the crash by simply refusing to navigate when the view is not attached.

@@ -116,6 +120,9 @@ class WebViewGuestDelegate : public content::BrowserPluginGuestDelegate,
// Whether the guest view is inside a plugin document.
bool is_full_page_plugin_;
// Whether attached.
bool attached_;

This comment has been minimized.

@zcbenz

zcbenz Jul 17, 2017

Contributor

It should be initialized.

bool attached_ = false;
@zcbenz

zcbenz Jul 17, 2017

Contributor

It should be initialized.

bool attached_ = false;

This comment has been minimized.

@ferreus

ferreus Jul 17, 2017

Contributor

done

@ferreus

ferreus Jul 17, 2017

Contributor

done

Show outdated Hide outdated atom/browser/web_view_guest_delegate.h
@zcbenz

zcbenz approved these changes Jul 19, 2017

@zcbenz zcbenz merged commit 52c6c7e into electron:master Jul 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kontrollanten kontrollanten referenced this pull request Oct 3, 2017

Merged

Yarn implementation #124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment