summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorlazyboy <lazyboy@chromium.org>2014-10-20 10:16:04 -0700
committerCommit bot <commit-bot@chromium.org>2014-10-20 17:16:43 +0000
commit549b3b4f0cf5691d093823f285ffd437614af1f6 (patch)
treedf95982227c5aac7f101d7d9e187e4f811ef2768
parentadb045fd004834946c39bc11c24399ac4ebbac7c (diff)
downloadchromium_src-549b3b4f0cf5691d093823f285ffd437614af1f6.zip
chromium_src-549b3b4f0cf5691d093823f285ffd437614af1f6.tar.gz
chromium_src-549b3b4f0cf5691d093823f285ffd437614af1f6.tar.bz2
Fix RenderWidgetHostViewGuest leak.
This used to happen on view initiated destruction, as opposed to RenderWidgetHostImpl initiated ones. In RWH initiated destruction, the views gets deleted properly. "View initiated destruction" means the view is first deleted before the host, this is aura's RWHViewAura::OnWindowDestroyed() or mac's ~RenderWidgetHostViewMac(). In view(V1) initiated destruction, RWH::SetView(NULL) call makes the host(H) drop the pointer to the view(V2) it's holding, in cases where the view initiated the destruction is not the view of the host(V1!=V2), it will leak V2. This scenario happens when there's an interstitial page showing inside <webview> guest. The platform RWHView gets deleted but the RWHViewGuest leaks as the RWH's view pointer is cleared while clearing the platform view. BUG=321662 Test=No visible change, internal change. To check leak sanitizer test, my steps are: gyp: CC=clang CXX=clang++ builddir_name=./tmp-llvm GYP_PARALLEL=1 \ PATH=$PWD/third_party/llvm-build/Release+Asserts/bin:$PATH \ GYP_GENERATORS='ninja' ./build/gyp_chromium -Gconfig=Release \ -Goutput_dir=out_asan_new -D"asan=1" -D"clang=1" \ -D"component=static_library" -D"lsan=1" -D"target_arch=x64" \ -D"use_allocator=none" build: PATH=$PWD/third_party/llvm-build/Release+Asserts/bin:$PATH \ ninja -C out_asan_new/Release -j 2048 browser_tests run: ASAN_OPTIONS="detect_leaks=1" \ LSAN_OPTIONS="suppressions=./tools/lsan/suppressions.txt" \ ./out_asan_new/Release/browser_tests \ --gtest_filter=WebViewTest.InterstitialTeardown \ --disable-seccomp-sandbox Review URL: https://codereview.chromium.org/647613002 Cr-Commit-Position: refs/heads/master@{#300286}
-rw-r--r--chrome/browser/apps/web_view_browsertest.cc8
-rw-r--r--chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm8
-rw-r--r--content/browser/browser_plugin/browser_plugin_guest.cc2
-rw-r--r--content/browser/frame_host/interstitial_page_impl.cc2
-rw-r--r--content/browser/frame_host/render_widget_host_view_guest.cc7
-rw-r--r--content/browser/frame_host/render_widget_host_view_guest.h9
-rw-r--r--content/browser/frame_host/render_widget_host_view_guest_unittest.cc3
-rw-r--r--content/browser/renderer_host/DEPS2
-rw-r--r--content/browser/renderer_host/render_widget_host_unittest.cc2
-rw-r--r--content/browser/renderer_host/render_widget_host_view_aura.cc13
-rw-r--r--content/browser/renderer_host/render_widget_host_view_aura.h11
-rw-r--r--content/browser/renderer_host/render_widget_host_view_aura_unittest.cc80
-rw-r--r--content/browser/renderer_host/render_widget_host_view_mac.h11
-rw-r--r--content/browser/renderer_host/render_widget_host_view_mac.mm16
-rw-r--r--content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm2
-rw-r--r--content/browser/renderer_host/render_widget_host_view_mac_unittest.mm64
-rw-r--r--content/browser/web_contents/web_contents_impl.cc4
-rw-r--r--content/browser/web_contents/web_contents_view.h7
-rw-r--r--content/browser/web_contents/web_contents_view_android.cc2
-rw-r--r--content/browser/web_contents/web_contents_view_android.h2
-rw-r--r--content/browser/web_contents/web_contents_view_aura.cc6
-rw-r--r--content/browser/web_contents/web_contents_view_aura.h2
-rw-r--r--content/browser/web_contents/web_contents_view_guest.cc6
-rw-r--r--content/browser/web_contents/web_contents_view_guest.h2
-rw-r--r--content/browser/web_contents/web_contents_view_mac.h2
-rw-r--r--content/browser/web_contents/web_contents_view_mac.mm6
26 files changed, 212 insertions, 67 deletions
diff --git a/chrome/browser/apps/web_view_browsertest.cc b/chrome/browser/apps/web_view_browsertest.cc
index cf50c01..2627ef3 100644
--- a/chrome/browser/apps/web_view_browsertest.cc
+++ b/chrome/browser/apps/web_view_browsertest.cc
@@ -1217,13 +1217,7 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestResizeWebviewResizesContent) {
// This test makes sure we do not crash if app is closed while interstitial
// page is being shown in guest.
-// Disabled under LeakSanitizer due to memory leaks. http://crbug.com/321662
-#if defined(LEAK_SANITIZER)
-#define MAYBE_InterstitialTeardown DISABLED_InterstitialTeardown
-#else
-#define MAYBE_InterstitialTeardown InterstitialTeardown
-#endif
-IN_PROC_BROWSER_TEST_F(WebViewTest, MAYBE_InterstitialTeardown) {
+IN_PROC_BROWSER_TEST_F(WebViewTest, InterstitialTeardown) {
#if defined(OS_WIN)
// Flaky on XP bot http://crbug.com/297014
if (base::win::GetVersion() <= base::win::VERSION_XP)
diff --git a/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm b/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
index a1ea1e2..3f78f80 100644
--- a/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
+++ b/chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
@@ -76,8 +76,12 @@ class SpellCheckObserver : public content::WebContentsObserver {
self = [super init];
if (self) {
renderWidgetHost_ = renderWidgetHost;
- NSView* nativeView = renderWidgetHost_->GetView()->GetNativeView();
- view_id_util::SetID(nativeView, VIEW_ID_TAB_CONTAINER);
+ // if |renderWidgetHost_| belongs to a BrowserPluginGuest, then it won't
+ // have a view yet.
+ if (renderWidgetHost_->GetView()) {
+ NSView* nativeView = renderWidgetHost_->GetView()->GetNativeView();
+ view_id_util::SetID(nativeView, VIEW_ID_TAB_CONTAINER);
+ }
if (renderWidgetHost_->IsRenderView()) {
spellingObserver_.reset(
diff --git a/content/browser/browser_plugin/browser_plugin_guest.cc b/content/browser/browser_plugin/browser_plugin_guest.cc
index ffdf00d..efb76e0 100644
--- a/content/browser/browser_plugin/browser_plugin_guest.cc
+++ b/content/browser/browser_plugin/browser_plugin_guest.cc
@@ -544,7 +544,7 @@ void BrowserPluginGuest::Attach(
static_cast<WebContentsViewGuest*>(GetWebContents()->GetView());
if (!web_contents()->GetRenderViewHost()->GetView()) {
web_contents_view->CreateViewForWidget(
- web_contents()->GetRenderViewHost());
+ web_contents()->GetRenderViewHost(), true);
}
}
diff --git a/content/browser/frame_host/interstitial_page_impl.cc b/content/browser/frame_host/interstitial_page_impl.cc
index 4628c70..6899d42 100644
--- a/content/browser/frame_host/interstitial_page_impl.cc
+++ b/content/browser/frame_host/interstitial_page_impl.cc
@@ -581,7 +581,7 @@ WebContentsView* InterstitialPageImpl::CreateWebContentsView() {
WebContentsView* wcv =
static_cast<WebContentsImpl*>(web_contents())->GetView();
RenderWidgetHostViewBase* view =
- wcv->CreateViewForWidget(render_view_host_);
+ wcv->CreateViewForWidget(render_view_host_, false);
render_view_host_->SetView(view);
render_view_host_->AllowBindings(BINDINGS_POLICY_DOM_AUTOMATION);
diff --git a/content/browser/frame_host/render_widget_host_view_guest.cc b/content/browser/frame_host/render_widget_host_view_guest.cc
index 546cba0..20782e8 100644
--- a/content/browser/frame_host/render_widget_host_view_guest.cc
+++ b/content/browser/frame_host/render_widget_host_view_guest.cc
@@ -46,7 +46,7 @@ blink::WebGestureEvent CreateFlingCancelEvent(double time_stamp) {
RenderWidgetHostViewGuest::RenderWidgetHostViewGuest(
RenderWidgetHost* widget_host,
BrowserPluginGuest* guest,
- RenderWidgetHostViewBase* platform_view)
+ base::WeakPtr<RenderWidgetHostViewBase> platform_view)
: RenderWidgetHostViewChildFrame(widget_host),
// |guest| is NULL during test.
guest_(guest ? guest->AsWeakPtr() : base::WeakPtr<BrowserPluginGuest>()),
@@ -177,7 +177,8 @@ void RenderWidgetHostViewGuest::Destroy() {
// The RenderWidgetHost's destruction led here, so don't call it.
DestroyGuestView();
- platform_view_->Destroy();
+ if (platform_view_) // The platform view might have been destroyed already.
+ platform_view_->Destroy();
}
gfx::Size RenderWidgetHostViewGuest::GetPhysicalBackingSize() const {
@@ -419,7 +420,7 @@ void RenderWidgetHostViewGuest::ShowDefinitionForSelection() {
// Vertical offset from guest's top to embedder's bottom edge.
embedder_bounds.bottom() - guest_bounds.y());
- RenderWidgetHostViewMacDictionaryHelper helper(platform_view_);
+ RenderWidgetHostViewMacDictionaryHelper helper(platform_view_.get());
helper.SetTargetView(rwhv);
helper.set_offset(guest_offset);
helper.ShowDefinitionForSelection();
diff --git a/content/browser/frame_host/render_widget_host_view_guest.h b/content/browser/frame_host/render_widget_host_view_guest.h
index 697d922..30edf5f 100644
--- a/content/browser/frame_host/render_widget_host_view_guest.h
+++ b/content/browser/frame_host/render_widget_host_view_guest.h
@@ -39,9 +39,10 @@ class CONTENT_EXPORT RenderWidgetHostViewGuest
public ui::GestureConsumer,
public ui::GestureEventHelper {
public:
- RenderWidgetHostViewGuest(RenderWidgetHost* widget,
- BrowserPluginGuest* guest,
- RenderWidgetHostViewBase* platform_view);
+ RenderWidgetHostViewGuest(
+ RenderWidgetHost* widget,
+ BrowserPluginGuest* guest,
+ base::WeakPtr<RenderWidgetHostViewBase> platform_view);
virtual ~RenderWidgetHostViewGuest();
bool OnMessageReceivedFromEmbedder(const IPC::Message& message,
@@ -176,7 +177,7 @@ class CONTENT_EXPORT RenderWidgetHostViewGuest
// The platform view for this RenderWidgetHostView.
// RenderWidgetHostViewGuest mostly only cares about stuff related to
// compositing, the rest are directly forwared to this |platform_view_|.
- RenderWidgetHostViewBase* platform_view_;
+ base::WeakPtr<RenderWidgetHostViewBase> platform_view_;
#if defined(USE_AURA)
scoped_ptr<ui::GestureRecognizer> gesture_recognizer_;
#endif
diff --git a/content/browser/frame_host/render_widget_host_view_guest_unittest.cc b/content/browser/frame_host/render_widget_host_view_guest_unittest.cc
index 172be97..b658158 100644
--- a/content/browser/frame_host/render_widget_host_view_guest_unittest.cc
+++ b/content/browser/frame_host/render_widget_host_view_guest_unittest.cc
@@ -34,7 +34,8 @@ class RenderWidgetHostViewGuestTest : public testing::Test {
widget_host_ = new RenderWidgetHostImpl(
&delegate_, process_host, MSG_ROUTING_NONE, false);
view_ = new RenderWidgetHostViewGuest(
- widget_host_, NULL, new TestRenderWidgetHostView(widget_host_));
+ widget_host_, NULL,
+ (new TestRenderWidgetHostView(widget_host_))->GetWeakPtr());
}
virtual void TearDown() {
diff --git a/content/browser/renderer_host/DEPS b/content/browser/renderer_host/DEPS
index 55a8ec5..3afe25b 100644
--- a/content/browser/renderer_host/DEPS
+++ b/content/browser/renderer_host/DEPS
@@ -15,7 +15,7 @@ include_rules = [
]
specific_include_rules = {
- ".*_(unit|browser)test\.cc": [
+ ".*_(unit|browser)test\.(cc|mm)": [
"+content/browser/frame_host",
"+content/browser/web_contents",
"+content/public/browser/web_contents.h",
diff --git a/content/browser/renderer_host/render_widget_host_unittest.cc b/content/browser/renderer_host/render_widget_host_unittest.cc
index 0b1d47f..56e90a5 100644
--- a/content/browser/renderer_host/render_widget_host_unittest.cc
+++ b/content/browser/renderer_host/render_widget_host_unittest.cc
@@ -762,7 +762,7 @@ TEST_F(RenderWidgetHostTest, ResizeThenCrash) {
TEST_F(RenderWidgetHostTest, Background) {
scoped_ptr<RenderWidgetHostViewBase> view;
#if defined(USE_AURA)
- view.reset(new RenderWidgetHostViewAura(host_.get()));
+ view.reset(new RenderWidgetHostViewAura(host_.get(), false));
// TODO(derat): Call this on all platforms: http://crbug.com/102450.
view->InitAsChild(NULL);
#elif defined(OS_ANDROID)
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.cc b/content/browser/renderer_host/render_widget_host_view_aura.cc
index 81afd1b..a7eb8d6 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura.cc
@@ -428,7 +428,8 @@ class RenderWidgetHostViewAura::WindowObserver : public aura::WindowObserver {
////////////////////////////////////////////////////////////////////////////////
// RenderWidgetHostViewAura, public:
-RenderWidgetHostViewAura::RenderWidgetHostViewAura(RenderWidgetHost* host)
+RenderWidgetHostViewAura::RenderWidgetHostViewAura(RenderWidgetHost* host,
+ bool is_guest_view_hack)
: host_(RenderWidgetHostImpl::From(host)),
window_(new aura::Window(this)),
delegated_frame_host_(new DelegatedFrameHost(this)),
@@ -453,8 +454,11 @@ RenderWidgetHostViewAura::RenderWidgetHostViewAura(RenderWidgetHost* host)
#endif
has_snapped_to_boundary_(false),
touch_editing_client_(NULL),
+ is_guest_view_hack_(is_guest_view_hack),
weak_ptr_factory_(this) {
- host_->SetView(this);
+ if (!is_guest_view_hack_)
+ host_->SetView(this);
+
window_observer_.reset(new WindowObserver(this));
aura::client::SetTooltipText(window_, &tooltip_);
aura::client::SetActivationDelegate(window_, this);
@@ -1765,7 +1769,10 @@ void RenderWidgetHostViewAura::OnWindowDestroying(aura::Window* window) {
}
void RenderWidgetHostViewAura::OnWindowDestroyed(aura::Window* window) {
- host_->ViewDestroyed();
+ // Ask the RWH to drop reference to us.
+ if (!is_guest_view_hack_)
+ host_->ViewDestroyed();
+
delete this;
}
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.h b/content/browser/renderer_host/render_widget_host_view_aura.h
index c038a8a..0a6ba5f 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.h
+++ b/content/browser/renderer_host/render_widget_host_view_aura.h
@@ -130,7 +130,12 @@ class CONTENT_EXPORT RenderWidgetHostViewAura
touch_editing_client_ = client;
}
- explicit RenderWidgetHostViewAura(RenderWidgetHost* host);
+ // When |is_guest_view_hack| is true, this view isn't really the view for
+ // the |widget|, a RenderWidgetHostViewGuest is.
+ //
+ // TODO(lazyboy): Remove |is_guest_view_hack| once BrowserPlugin has migrated
+ // to use RWHVChildFrame (http://crbug.com/330264).
+ RenderWidgetHostViewAura(RenderWidgetHost* host, bool is_guest_view_hack);
// RenderWidgetHostView implementation.
virtual bool OnMessageReceived(const IPC::Message& msg) override;
@@ -619,6 +624,10 @@ class CONTENT_EXPORT RenderWidgetHostViewAura
scoped_ptr<aura::client::ScopedTooltipDisabler> tooltip_disabler_;
+ // True when this view acts as a platform view hack for a
+ // RenderWidgetHostViewGuest.
+ bool is_guest_view_hack_;
+
base::WeakPtrFactory<RenderWidgetHostViewAura> weak_ptr_factory_;
gfx::Rect disambiguation_target_rect_;
diff --git a/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc b/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
index 589135a..2153b2e 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
@@ -18,6 +18,7 @@
#include "content/browser/browser_thread_impl.h"
#include "content/browser/compositor/resize_lock.h"
#include "content/browser/compositor/test/no_transport_image_transport_factory.h"
+#include "content/browser/frame_host/render_widget_host_view_guest.h"
#include "content/browser/renderer_host/overscroll_controller.h"
#include "content/browser/renderer_host/overscroll_controller_delegate.h"
#include "content/browser/renderer_host/render_widget_host_delegate.h"
@@ -221,8 +222,10 @@ class FakeFrameSubscriber : public RenderWidgetHostViewFrameSubscriber {
class FakeRenderWidgetHostViewAura : public RenderWidgetHostViewAura {
public:
- FakeRenderWidgetHostViewAura(RenderWidgetHost* widget)
- : RenderWidgetHostViewAura(widget), has_resize_lock_(false) {}
+ FakeRenderWidgetHostViewAura(RenderWidgetHost* widget,
+ bool is_guest_view_hack)
+ : RenderWidgetHostViewAura(widget, is_guest_view_hack),
+ has_resize_lock_(false) {}
virtual ~FakeRenderWidgetHostViewAura() {}
@@ -325,7 +328,9 @@ class MockWindowObserver : public aura::WindowObserver {
class RenderWidgetHostViewAuraTest : public testing::Test {
public:
RenderWidgetHostViewAuraTest()
- : browser_thread_for_ui_(BrowserThread::UI, &message_loop_) {}
+ : widget_host_uses_shutdown_to_destroy_(false),
+ is_guest_view_hack_(false),
+ browser_thread_for_ui_(BrowserThread::UI, &message_loop_) {}
void SetUpEnvironment() {
ImageTransportFactory::InitializeForUnitTests(
@@ -343,7 +348,8 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
parent_host_ = new RenderWidgetHostImpl(
&delegate_, process_host_, MSG_ROUTING_NONE, false);
- parent_view_ = new RenderWidgetHostViewAura(parent_host_);
+ parent_view_ = new RenderWidgetHostViewAura(parent_host_,
+ is_guest_view_hack_);
parent_view_->InitAsChild(NULL);
aura::client::ParentWindowWithContext(parent_view_->GetNativeView(),
aura_test_helper_->root_window(),
@@ -352,7 +358,7 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
widget_host_ = new RenderWidgetHostImpl(
&delegate_, process_host_, MSG_ROUTING_NONE, false);
widget_host_->Init();
- view_ = new FakeRenderWidgetHostViewAura(widget_host_);
+ view_ = new FakeRenderWidgetHostViewAura(widget_host_, is_guest_view_hack_);
}
void TearDownEnvironment() {
@@ -360,7 +366,11 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
process_host_ = NULL;
if (view_)
view_->Destroy();
- delete widget_host_;
+
+ if (widget_host_uses_shutdown_to_destroy_)
+ widget_host_->Shutdown();
+ else
+ delete widget_host_;
parent_view_->Destroy();
delete parent_host_;
@@ -373,11 +383,20 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
ImageTransportFactory::Terminate();
}
- virtual void SetUp() { SetUpEnvironment(); }
+ virtual void SetUp() override { SetUpEnvironment(); }
- virtual void TearDown() { TearDownEnvironment(); }
+ virtual void TearDown() override { TearDownEnvironment(); }
+
+ void set_widget_host_uses_shutdown_to_destroy(bool use) {
+ widget_host_uses_shutdown_to_destroy_ = use;
+ }
protected:
+ // If true, then calls RWH::Shutdown() instead of deleting RWH.
+ bool widget_host_uses_shutdown_to_destroy_;
+
+ bool is_guest_view_hack_;
+
base::MessageLoopForUI message_loop_;
BrowserThreadImpl browser_thread_for_ui_;
scoped_ptr<aura::test::AuraTestHelper> aura_test_helper_;
@@ -401,6 +420,40 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostViewAuraTest);
};
+// Helper class to instantiate RenderWidgetHostViewGuest which is backed
+// by an aura platform view.
+class RenderWidgetHostViewGuestAuraTest : public RenderWidgetHostViewAuraTest {
+ public:
+ RenderWidgetHostViewGuestAuraTest() {
+ // Use RWH::Shutdown to destroy RWH, instead of deleting.
+ // This will ensure that the RenderWidgetHostViewGuest is not leaked and
+ // is deleted properly upon RWH going away.
+ set_widget_host_uses_shutdown_to_destroy(true);
+ }
+
+ // We explicitly invoke SetUp to allow gesture debounce customization.
+ virtual void SetUp() {
+ is_guest_view_hack_ = true;
+
+ RenderWidgetHostViewAuraTest::SetUp();
+
+ guest_view_weak_ = (new RenderWidgetHostViewGuest(
+ widget_host_, NULL, view_->GetWeakPtr()))->GetWeakPtr();
+ }
+
+ virtual void TearDown() {
+ // Internal override to do nothing, we clean up ourselves in the test body.
+ // This helps us test that |guest_view_weak_| does not leak.
+ }
+
+ protected:
+ base::WeakPtr<RenderWidgetHostViewBase> guest_view_weak_;
+
+ private:
+
+ DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostViewGuestAuraTest);
+};
+
class RenderWidgetHostViewAuraOverscrollTest
: public RenderWidgetHostViewAuraTest {
public:
@@ -1579,7 +1632,7 @@ TEST_F(RenderWidgetHostViewAuraTest, DiscardDelegatedFrames) {
hosts[i] = new RenderWidgetHostImpl(
&delegate_, process_host_, MSG_ROUTING_NONE, false);
hosts[i]->Init();
- views[i] = new FakeRenderWidgetHostViewAura(hosts[i]);
+ views[i] = new FakeRenderWidgetHostViewAura(hosts[i], false);
views[i]->InitAsChild(NULL);
aura::client::ParentWindowWithContext(
views[i]->GetNativeView(),
@@ -1741,7 +1794,7 @@ TEST_F(RenderWidgetHostViewAuraTest, DiscardDelegatedFramesWithLocking) {
hosts[i] = new RenderWidgetHostImpl(
&delegate_, process_host_, MSG_ROUTING_NONE, false);
hosts[i]->Init();
- views[i] = new FakeRenderWidgetHostViewAura(hosts[i]);
+ views[i] = new FakeRenderWidgetHostViewAura(hosts[i], false);
views[i]->InitAsChild(NULL);
aura::client::ParentWindowWithContext(
views[i]->GetNativeView(),
@@ -2853,4 +2906,11 @@ TEST_F(RenderWidgetHostViewAuraOverscrollTest, OverscrollResetsOnBlur) {
EXPECT_EQ(3U, sink_->message_count());
}
+// Tests that when view initiated shutdown happens (i.e. RWHView is deleted
+// before RWH), we clean up properly and don't leak the RWHVGuest.
+TEST_F(RenderWidgetHostViewGuestAuraTest, GuestViewDoesNotLeak) {
+ TearDownEnvironment();
+ ASSERT_FALSE(guest_view_weak_.get());
+}
+
} // namespace content
diff --git a/content/browser/renderer_host/render_widget_host_view_mac.h b/content/browser/renderer_host/render_widget_host_view_mac.h
index 120aaec..22e1092 100644
--- a/content/browser/renderer_host/render_widget_host_view_mac.h
+++ b/content/browser/renderer_host/render_widget_host_view_mac.h
@@ -210,7 +210,12 @@ class CONTENT_EXPORT RenderWidgetHostViewMac
// The view will associate itself with the given widget. The native view must
// be hooked up immediately to the view hierarchy, or else when it is
// deleted it will delete this out from under the caller.
- explicit RenderWidgetHostViewMac(RenderWidgetHost* widget);
+ //
+ // When |is_guest_view_hack| is true, this view isn't really the view for
+ // the |widget|, a RenderWidgetHostViewGuest is.
+ // TODO(lazyboy): Remove |is_guest_view_hack| once BrowserPlugin has migrated
+ // to use RWHVChildFrame (http://crbug.com/330264).
+ RenderWidgetHostViewMac(RenderWidgetHost* widget, bool is_guest_view_hack);
virtual ~RenderWidgetHostViewMac();
RenderWidgetHostViewCocoa* cocoa_view() const { return cocoa_view_; }
@@ -477,6 +482,10 @@ class CONTENT_EXPORT RenderWidgetHostViewMac
// The text to be shown in the tooltip, supplied by the renderer.
base::string16 tooltip_text_;
+ // True when this view acts as a platform view hack for a
+ // RenderWidgetHostViewGuest.
+ bool is_guest_view_hack_;
+
// Factory used to safely scope delayed calls to ShutdownHost().
base::WeakPtrFactory<RenderWidgetHostViewMac> weak_factory_;
diff --git a/content/browser/renderer_host/render_widget_host_view_mac.mm b/content/browser/renderer_host/render_widget_host_view_mac.mm
index 2c64dcd..12bccb3 100644
--- a/content/browser/renderer_host/render_widget_host_view_mac.mm
+++ b/content/browser/renderer_host/render_widget_host_view_mac.mm
@@ -521,7 +521,8 @@ void RenderWidgetHostViewBase::GetDefaultScreenInfo(
///////////////////////////////////////////////////////////////////////////////
// RenderWidgetHostViewMac, public:
-RenderWidgetHostViewMac::RenderWidgetHostViewMac(RenderWidgetHost* widget)
+RenderWidgetHostViewMac::RenderWidgetHostViewMac(RenderWidgetHost* widget,
+ bool is_guest_view_hack)
: render_widget_host_(RenderWidgetHostImpl::From(widget)),
text_input_type_(ui::TEXT_INPUT_TYPE_NONE),
can_compose_inline_(true),
@@ -529,6 +530,7 @@ RenderWidgetHostViewMac::RenderWidgetHostViewMac(RenderWidgetHost* widget)
new BrowserCompositorViewPlaceholderMac),
is_loading_(false),
allow_pause_for_resize_or_repaint_(true),
+ is_guest_view_hack_(is_guest_view_hack),
weak_factory_(this),
fullscreen_parent_host_view_(NULL) {
// |cocoa_view_| owns us and we will be deleted when |cocoa_view_|
@@ -552,7 +554,8 @@ RenderWidgetHostViewMac::RenderWidgetHostViewMac(RenderWidgetHost* widget)
gfx::Screen::GetScreenFor(cocoa_view_)->AddObserver(this);
- render_widget_host_->SetView(this);
+ if (!is_guest_view_hack_)
+ render_widget_host_->SetView(this);
}
RenderWidgetHostViewMac::~RenderWidgetHostViewMac() {
@@ -570,8 +573,13 @@ RenderWidgetHostViewMac::~RenderWidgetHostViewMac() {
// We are owned by RenderWidgetHostViewCocoa, so if we go away before the
// RenderWidgetHost does we need to tell it not to hold a stale pointer to
// us.
- if (render_widget_host_)
- render_widget_host_->SetView(NULL);
+ if (render_widget_host_) {
+ // If this is a RenderWidgetHostViewGuest's platform_view_, we're not the
+ // RWH's view, the RenderWidgetHostViewGuest is. So don't reset the RWH's
+ // view, the RenderWidgetHostViewGuest will do it.
+ if (!is_guest_view_hack_)
+ render_widget_host_->SetView(NULL);
+ }
}
void RenderWidgetHostViewMac::SetDelegate(
diff --git a/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm b/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm
index b444282..ab9c945 100644
--- a/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm
+++ b/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm
@@ -133,7 +133,7 @@ TEST_F(RenderWidgetHostViewMacEditCommandHelperTest,
// Owned by its |cocoa_view()|, i.e. |rwhv_cocoa|.
RenderWidgetHostViewMac* rwhv_mac = new RenderWidgetHostViewMac(
- render_widget);
+ render_widget, false);
base::scoped_nsobject<RenderWidgetHostViewCocoa> rwhv_cocoa(
[rwhv_mac->cocoa_view() retain]);
diff --git a/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm b/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
index 2df6c19..feea4ab 100644
--- a/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
+++ b/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
@@ -10,6 +10,7 @@
#include "base/strings/utf_string_conversions.h"
#include "content/browser/browser_thread_impl.h"
#include "content/browser/compositor/test/no_transport_image_transport_factory.h"
+#include "content/browser/frame_host/render_widget_host_view_guest.h"
#include "content/browser/gpu/compositor_util.h"
#include "content/browser/renderer_host/render_widget_host_delegate.h"
#include "content/common/gpu/gpu_messages.h"
@@ -179,15 +180,13 @@ class RenderWidgetHostViewMacTest : public RenderViewHostImplTestHarness {
old_rwhv_ = rvh()->GetView();
// Owned by its |cocoa_view()|, i.e. |rwhv_cocoa_|.
- rwhv_mac_ = new RenderWidgetHostViewMac(rvh());
+ rwhv_mac_ = new RenderWidgetHostViewMac(rvh(), false);
rwhv_cocoa_.reset([rwhv_mac_->cocoa_view() retain]);
}
virtual void TearDown() {
// Make sure the rwhv_mac_ is gone once the superclass's |TearDown()| runs.
rwhv_cocoa_.reset();
- pool_.Recycle();
- base::MessageLoop::current()->RunUntilIdle();
- pool_.Recycle();
+ RecycleAndWait();
// See comment in SetUp().
test_rvh()->SetView(static_cast<RenderWidgetHostViewBase*>(old_rwhv_));
@@ -196,6 +195,12 @@ class RenderWidgetHostViewMacTest : public RenderViewHostImplTestHarness {
ImageTransportFactory::Terminate();
RenderViewHostImplTestHarness::TearDown();
}
+
+ void RecycleAndWait() {
+ pool_.Recycle();
+ base::MessageLoop::current()->RunUntilIdle();
+ pool_.Recycle();
+ }
protected:
private:
// This class isn't derived from PlatformTest.
@@ -268,7 +273,7 @@ TEST_F(RenderWidgetHostViewMacTest, FullscreenCloseOnEscape) {
// Owned by its |cocoa_view()|.
RenderWidgetHostImpl* rwh = new RenderWidgetHostImpl(
&delegate, process_host, MSG_ROUTING_NONE, false);
- RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(rwh);
+ RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(rwh, false);
view->InitAsFullscreen(rwhv_mac_);
@@ -301,7 +306,7 @@ TEST_F(RenderWidgetHostViewMacTest, AcceleratorDestroy) {
// Owned by its |cocoa_view()|.
RenderWidgetHostImpl* rwh = new RenderWidgetHostImpl(
&delegate, process_host, MSG_ROUTING_NONE, false);
- RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(rwh);
+ RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(rwh, false);
view->InitAsFullscreen(rwhv_mac_);
@@ -655,7 +660,7 @@ TEST_F(RenderWidgetHostViewMacTest, BlurAndFocusOnSetActive) {
// Owned by its |cocoa_view()|.
MockRenderWidgetHostImpl* rwh = new MockRenderWidgetHostImpl(
&delegate, process_host, MSG_ROUTING_NONE);
- RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(rwh);
+ RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(rwh, false);
base::scoped_nsobject<CocoaTestHelperWindow> window(
[[CocoaTestHelperWindow alloc] init]);
@@ -701,7 +706,7 @@ TEST_F(RenderWidgetHostViewMacTest, ScrollWheelEndEventDelivery) {
MockRenderWidgetHostDelegate delegate;
MockRenderWidgetHostImpl* host = new MockRenderWidgetHostImpl(
&delegate, process_host, MSG_ROUTING_NONE);
- RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(host);
+ RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(host, false);
// Send an initial wheel event with NSEventPhaseBegan to the view.
NSEvent* event1 = MockScrollWheelEventWithPhase(@selector(phaseBegan), 0);
@@ -741,7 +746,7 @@ TEST_F(RenderWidgetHostViewMacTest, IgnoreEmptyUnhandledWheelEvent) {
MockRenderWidgetHostDelegate delegate;
MockRenderWidgetHostImpl* host = new MockRenderWidgetHostImpl(
&delegate, process_host, MSG_ROUTING_NONE);
- RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(host);
+ RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(host, false);
// Add a delegate to the view.
base::scoped_nsobject<MockRenderWidgetHostViewMacDelegate> view_delegate(
@@ -783,4 +788,45 @@ TEST_F(RenderWidgetHostViewMacTest, IgnoreEmptyUnhandledWheelEvent) {
host->Shutdown();
}
+// Tests that when view initiated shutdown happens (i.e. RWHView is deleted
+// before RWH), we clean up properly and don't leak the RWHVGuest.
+TEST_F(RenderWidgetHostViewMacTest, GuestViewDoesNotLeak) {
+ MockRenderWidgetHostDelegate delegate;
+ TestBrowserContext browser_context;
+ MockRenderProcessHost* process_host =
+ new MockRenderProcessHost(&browser_context);
+
+ // Owned by its |cocoa_view()|.
+ MockRenderWidgetHostImpl* rwh = new MockRenderWidgetHostImpl(
+ &delegate, process_host, MSG_ROUTING_NONE);
+ RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(rwh, true);
+
+ // Add a delegate to the view.
+ base::scoped_nsobject<MockRenderWidgetHostViewMacDelegate> view_delegate(
+ [[MockRenderWidgetHostViewMacDelegate alloc] init]);
+ view->SetDelegate(view_delegate.get());
+
+ base::WeakPtr<RenderWidgetHostViewBase> guest_rwhv_weak =
+ (new RenderWidgetHostViewGuest(
+ rwh, NULL, view->GetWeakPtr()))->GetWeakPtr();
+
+ // Remove the cocoa_view() so |view| also goes away before |rwh|.
+ {
+ base::scoped_nsobject<RenderWidgetHostViewCocoa> rwhv_cocoa;
+ rwhv_cocoa.reset([view->cocoa_view() retain]);
+ }
+ RecycleAndWait();
+
+ // Clean up.
+ rwh->Shutdown();
+
+ // Let |guest_rwhv_weak| have a chance to delete itself.
+ base::RunLoop run_loop;
+ content::BrowserThread::PostTask(
+ content::BrowserThread::UI, FROM_HERE, run_loop.QuitClosure());
+ run_loop.Run();
+
+ ASSERT_FALSE(guest_rwhv_weak.get());
+}
+
} // namespace content
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index a86d316..078356f 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -1531,7 +1531,7 @@ void WebContentsImpl::CreateNewWindow(
// TODO(brettw): It seems bogus that we have to call this function on the
// newly created object and give it one of its own member variables.
- new_view->CreateViewForWidget(new_contents->GetRenderViewHost());
+ new_view->CreateViewForWidget(new_contents->GetRenderViewHost(), false);
}
// Save the created window associated with the route so we can show it
// later.
@@ -4092,7 +4092,7 @@ bool WebContentsImpl::CreateRenderViewForRenderManager(
new RenderWidgetHostViewChildFrame(render_view_host);
rwh_view = rwh_view_child;
} else {
- rwh_view = view_->CreateViewForWidget(render_view_host);
+ rwh_view = view_->CreateViewForWidget(render_view_host, false);
}
// Now that the RenderView has been created, we need to tell it its size.
diff --git a/content/browser/web_contents/web_contents_view.h b/content/browser/web_contents/web_contents_view.h
index 700194b..47daee9 100644
--- a/content/browser/web_contents/web_contents_view.h
+++ b/content/browser/web_contents/web_contents_view.h
@@ -79,8 +79,13 @@ class WebContentsView {
// Sets up the View that holds the rendered web page, receives messages for
// it and contains page plugins. The host view should be sized to the current
// size of the WebContents.
+ //
+ // |is_guest_view_hack| is temporary hack and will be removed once
+ // RenderWidgetHostViewGuest is not dependent on platform view.
+ // TODO(lazyboy): Remove |is_guest_view_hack| once http://crbug.com/330264 is
+ // fixed.
virtual RenderWidgetHostViewBase* CreateViewForWidget(
- RenderWidgetHost* render_widget_host) = 0;
+ RenderWidgetHost* render_widget_host, bool is_guest_view_hack) = 0;
// Creates a new View that holds a popup and receives messages for it.
virtual RenderWidgetHostViewBase* CreateViewForPopupWidget(
diff --git a/content/browser/web_contents/web_contents_view_android.cc b/content/browser/web_contents/web_contents_view_android.cc
index 58ed0ef..b276b19 100644
--- a/content/browser/web_contents/web_contents_view_android.cc
+++ b/content/browser/web_contents/web_contents_view_android.cc
@@ -121,7 +121,7 @@ void WebContentsViewAndroid::CreateView(
}
RenderWidgetHostViewBase* WebContentsViewAndroid::CreateViewForWidget(
- RenderWidgetHost* render_widget_host) {
+ RenderWidgetHost* render_widget_host, bool is_guest_view_hack) {
if (render_widget_host->GetView()) {
// During testing, the view will already be set up in most cases to the
// test view, so we don't want to clobber it with a real one. To verify that
diff --git a/content/browser/web_contents/web_contents_view_android.h b/content/browser/web_contents/web_contents_view_android.h
index e4c9e60..c480c0e 100644
--- a/content/browser/web_contents/web_contents_view_android.h
+++ b/content/browser/web_contents/web_contents_view_android.h
@@ -44,7 +44,7 @@ class WebContentsViewAndroid : public WebContentsView,
virtual void CreateView(
const gfx::Size& initial_size, gfx::NativeView context) override;
virtual RenderWidgetHostViewBase* CreateViewForWidget(
- RenderWidgetHost* render_widget_host) override;
+ RenderWidgetHost* render_widget_host, bool is_guest_view_hack) override;
virtual RenderWidgetHostViewBase* CreateViewForPopupWidget(
RenderWidgetHost* render_widget_host) override;
virtual void SetPageTitle(const base::string16& title) override;
diff --git a/content/browser/web_contents/web_contents_view_aura.cc b/content/browser/web_contents/web_contents_view_aura.cc
index 433a338..3ba80e7 100644
--- a/content/browser/web_contents/web_contents_view_aura.cc
+++ b/content/browser/web_contents/web_contents_view_aura.cc
@@ -1113,7 +1113,7 @@ void WebContentsViewAura::CreateView(
}
RenderWidgetHostViewBase* WebContentsViewAura::CreateViewForWidget(
- RenderWidgetHost* render_widget_host) {
+ RenderWidgetHost* render_widget_host, bool is_guest_view_hack) {
if (render_widget_host->GetView()) {
// During testing, the view will already be set up in most cases to the
// test view, so we don't want to clobber it with a real one. To verify that
@@ -1126,7 +1126,7 @@ RenderWidgetHostViewBase* WebContentsViewAura::CreateViewForWidget(
}
RenderWidgetHostViewAura* view =
- new RenderWidgetHostViewAura(render_widget_host);
+ new RenderWidgetHostViewAura(render_widget_host, is_guest_view_hack);
view->InitAsChild(NULL);
GetNativeView()->AddChild(view->GetNativeView());
@@ -1155,7 +1155,7 @@ RenderWidgetHostViewBase* WebContentsViewAura::CreateViewForWidget(
RenderWidgetHostViewBase* WebContentsViewAura::CreateViewForPopupWidget(
RenderWidgetHost* render_widget_host) {
- return new RenderWidgetHostViewAura(render_widget_host);
+ return new RenderWidgetHostViewAura(render_widget_host, false);
}
void WebContentsViewAura::SetPageTitle(const base::string16& title) {
diff --git a/content/browser/web_contents/web_contents_view_aura.h b/content/browser/web_contents/web_contents_view_aura.h
index 84e91fa..00a5b37 100644
--- a/content/browser/web_contents/web_contents_view_aura.h
+++ b/content/browser/web_contents/web_contents_view_aura.h
@@ -117,7 +117,7 @@ class WebContentsViewAura
virtual void CreateView(
const gfx::Size& initial_size, gfx::NativeView context) override;
virtual RenderWidgetHostViewBase* CreateViewForWidget(
- RenderWidgetHost* render_widget_host) override;
+ RenderWidgetHost* render_widget_host, bool is_guest_view_hack) override;
virtual RenderWidgetHostViewBase* CreateViewForPopupWidget(
RenderWidgetHost* render_widget_host) override;
virtual void SetPageTitle(const base::string16& title) override;
diff --git a/content/browser/web_contents/web_contents_view_guest.cc b/content/browser/web_contents/web_contents_view_guest.cc
index 675cfae..6cc1581 100644
--- a/content/browser/web_contents/web_contents_view_guest.cc
+++ b/content/browser/web_contents/web_contents_view_guest.cc
@@ -140,7 +140,7 @@ void WebContentsViewGuest::CreateView(const gfx::Size& initial_size,
}
RenderWidgetHostViewBase* WebContentsViewGuest::CreateViewForWidget(
- RenderWidgetHost* render_widget_host) {
+ RenderWidgetHost* render_widget_host, bool is_guest_view_hack) {
if (render_widget_host->GetView()) {
// During testing, the view will already be set up in most cases to the
// test view, so we don't want to clobber it with a real one. To verify that
@@ -153,11 +153,11 @@ RenderWidgetHostViewBase* WebContentsViewGuest::CreateViewForWidget(
}
RenderWidgetHostViewBase* platform_widget =
- platform_view_->CreateViewForWidget(render_widget_host);
+ platform_view_->CreateViewForWidget(render_widget_host, true);
return new RenderWidgetHostViewGuest(render_widget_host,
guest_,
- platform_widget);
+ platform_widget->GetWeakPtr());
}
RenderWidgetHostViewBase* WebContentsViewGuest::CreateViewForPopupWidget(
diff --git a/content/browser/web_contents/web_contents_view_guest.h b/content/browser/web_contents/web_contents_view_guest.h
index ae72f5b..dfb76e7 100644
--- a/content/browser/web_contents/web_contents_view_guest.h
+++ b/content/browser/web_contents/web_contents_view_guest.h
@@ -59,7 +59,7 @@ class WebContentsViewGuest : public WebContentsView,
virtual void CreateView(const gfx::Size& initial_size,
gfx::NativeView context) override;
virtual RenderWidgetHostViewBase* CreateViewForWidget(
- RenderWidgetHost* render_widget_host) override;
+ RenderWidgetHost* render_widget_host, bool is_guest_view_hack) override;
virtual RenderWidgetHostViewBase* CreateViewForPopupWidget(
RenderWidgetHost* render_widget_host) override;
virtual void SetPageTitle(const base::string16& title) override;
diff --git a/content/browser/web_contents/web_contents_view_mac.h b/content/browser/web_contents/web_contents_view_mac.h
index 8051adb..57cd35e 100644
--- a/content/browser/web_contents/web_contents_view_mac.h
+++ b/content/browser/web_contents/web_contents_view_mac.h
@@ -82,7 +82,7 @@ class WebContentsViewMac : public WebContentsView,
virtual void CreateView(
const gfx::Size& initial_size, gfx::NativeView context) override;
virtual RenderWidgetHostViewBase* CreateViewForWidget(
- RenderWidgetHost* render_widget_host) override;
+ RenderWidgetHost* render_widget_host, bool is_guest_view_hack) override;
virtual RenderWidgetHostViewBase* CreateViewForPopupWidget(
RenderWidgetHost* render_widget_host) override;
virtual void SetPageTitle(const base::string16& title) override;
diff --git a/content/browser/web_contents/web_contents_view_mac.mm b/content/browser/web_contents/web_contents_view_mac.mm
index c27c992..6e883b6 100644
--- a/content/browser/web_contents/web_contents_view_mac.mm
+++ b/content/browser/web_contents/web_contents_view_mac.mm
@@ -284,7 +284,7 @@ void WebContentsViewMac::CreateView(
}
RenderWidgetHostViewBase* WebContentsViewMac::CreateViewForWidget(
- RenderWidgetHost* render_widget_host) {
+ RenderWidgetHost* render_widget_host, bool is_guest_view_hack) {
if (render_widget_host->GetView()) {
// During testing, the view will already be set up in most cases to the
// test view, so we don't want to clobber it with a real one. To verify that
@@ -297,7 +297,7 @@ RenderWidgetHostViewBase* WebContentsViewMac::CreateViewForWidget(
}
RenderWidgetHostViewMac* view = new RenderWidgetHostViewMac(
- render_widget_host);
+ render_widget_host, is_guest_view_hack);
if (delegate()) {
base::scoped_nsobject<NSObject<RenderWidgetHostViewMacDelegate> >
rw_delegate(
@@ -331,7 +331,7 @@ RenderWidgetHostViewBase* WebContentsViewMac::CreateViewForWidget(
RenderWidgetHostViewBase* WebContentsViewMac::CreateViewForPopupWidget(
RenderWidgetHost* render_widget_host) {
- return new RenderWidgetHostViewMac(render_widget_host);
+ return new RenderWidgetHostViewMac(render_widget_host, false);
}
void WebContentsViewMac::SetPageTitle(const base::string16& title) {