diff options
Diffstat (limited to 'content/browser')
24 files changed, 205 insertions, 58 deletions
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) { |