Fix RenderWidgetHostViewGuest leak.

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

Committed: https://crrev.com/549b3b4f0cf5691d093823f285ffd437614af1f6
Cr-Commit-Position: refs/heads/master@{#300286}

[lazyboy, 2014-10-10 01:22:58]

lazyboy@chromium.org changed reviewers:
+ sky@chromium.org

[lazyboy, 2014-10-10 01:22:58]

+sky for review.

https://chromiumcodereview.appspot.com/647613002/diff/120001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/647613002/diff/120001/content/browser/...
content/browser/renderer_host/render_widget_host_view_mac.mm:575:
render_widget_host_->SetView(NULL);
It might be consistent to call
render_widget_host_->ViewDestroyed() instead.

That function has additional
RWH::RejectMouseLockOrUnlockIfNecessary() call though, so
not sure about the side effect.

[sky, 2014-10-10 02:18:46]

How about a test in the content side?

[lazyboy, 2014-10-10 02:29:04]

On 2014/10/10 02:18:46, sky wrote:
> How about a test in the content side?

OK, do you mean unit_test or browser_test?
browser_test is hard since we can't instantiate standalone *ViewGuest without
chrome/ layer. The test I'm enabling in this CL is a browser_test in chrome/.
If unit_test, then I have to think, any pointers would be helpful, thanks.

[sky, 2014-10-10 17:22:55]

Ideally unit test. How come you can't instantiate a *ViewGuest without
chrome? Seems like you need to write some better testing in the
content side.

On Thu, Oct 9, 2014 at 7:29 PM,  <lazyboy@chromium.org> wrote:
> On 2014/10/10 02:18:46, sky wrote:
>>
>> How about a test in the content side?
>
>
> OK, do you mean unit_test or browser_test?
> browser_test is hard since we can't instantiate standalone *ViewGuest
> without
> chrome/ layer. The test I'm enabling in this CL is a browser_test in
> chrome/.
> If unit_test, then I have to think, any pointers would be helpful, thanks.
>
> https://chromiumcodereview.appspot.com/647613002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lazyboy, 2014-10-11 01:22:40]

Hi, I've added content_unittest for aura and mac.
These tests correctly fail without these change.

[sky, 2014-10-13 17:04:13]

I don't feel that I have a good enough grasp on the ownership model to know if
this is right. Is there a better reviewer?

https://chromiumcodereview.appspot.com/647613002/diff/200001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/647613002/diff/200001/content/browser/...
content/browser/renderer_host/render_widget_host_view_aura.cc:1747: // Ask the
RWH to drop reference to us.
This comment isn't helpful. It doesn't describe why the view may not be this.

[lazyboy, 2014-10-13 21:54:00]

lazyboy@chromium.org changed reviewers:
+ piman@chromium.org

[lazyboy, 2014-10-13 21:54:01]

sky:
I don't feel that I have a good enough grasp on the ownership model to know if
this is right. Is there a better reviewer?

-> probably piman@?

https://chromiumcodereview.appspot.com/647613002/diff/200001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://chromiumcodereview.appspot.com/647613002/diff/200001/content/browser/...
content/browser/renderer_host/render_widget_host_view_aura.cc:1747: // Ask the
RWH to drop reference to us.
On 2014/10/13 17:04:13, sky wrote:
> This comment isn't helpful. It doesn't describe why the view may not be this.

I've added comment explaining when GetView() != this can happen.

[piman, 2014-10-13 22:12:55]

On 2014/10/13 21:54:01, lazyboy wrote:
> sky:
> I don't feel that I have a good enough grasp on the ownership model to know if
> this is right. Is there a better reviewer?
> 
> -> probably piman@?

TBH this is adding workarounds for a hack that should never have been. RWH
shouldn't have 2 views attached to it. RWHVG shouldn't use a "platform view"
also attached to the same RWH. RenderWidgetHostViewChildFrame doesn't have one,
doesn't need one.
What is the plan to move RenderWidgetHostViewGuest to
RenderWidgetHostViewChildFrame?


> 
>
https://chromiumcodereview.appspot.com/647613002/diff/200001/content/browser/...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://chromiumcodereview.appspot.com/647613002/diff/200001/content/browser/...
> content/browser/renderer_host/render_widget_host_view_aura.cc:1747: // Ask the
> RWH to drop reference to us.
> On 2014/10/13 17:04:13, sky wrote:
> > This comment isn't helpful. It doesn't describe why the view may not be
this.
> 
> I've added comment explaining when GetView() != this can happen.

[piman, 2014-10-13 22:34:23]

On Mon, Oct 13, 2014 at 3:12 PM, <piman@chromium.org> wrote:

> On 2014/10/13 21:54:01, lazyboy wrote:
>
>> sky:
>> I don't feel that I have a good enough grasp on the ownership model to
>> know if
>> this is right. Is there a better reviewer?
>>
>
>  -> probably piman@?
>>
>
> TBH this is adding workarounds for a hack that should never have been. RWH
> shouldn't have 2 views attached to it. RWHVG shouldn't use a "platform
> view"
> also attached to the same RWH. RenderWidgetHostViewChildFrame doesn't have
> one,
> doesn't need one.
> What is the plan to move RenderWidgetHostViewGuest to
> RenderWidgetHostViewChildFrame?


I think I would rather see an explicit "is_guest_view_hack_" in RWHVA (and
RWHVM) that is set in the constructor, which we would use so that we don't
call host_->SetView(this), nor host_->ViewDestroyed() (since we're not
host_'s view). But I would really like us to make progress to remove the
platform_view_ hack, and instead tear off platform-specific logic so that
it can be used in both the real RWHVA/M as well as the RWHVGuest (and
RWHVChildFrame?) - when it makes sense to do that logic locally instead of
forwarding to the parent renderer.


> https://chromiumcodereview.appspot.com/647613002/diff/
> 200001/content/browser/renderer_host/render_widget_host_view_aura.cc
>
>> File content/browser/renderer_host/render_widget_host_view_aura.cc
>> (right):
>>
>
>
> https://chromiumcodereview.appspot.com/647613002/diff/
> 200001/content/browser/renderer_host/render_widget_
> host_view_aura.cc#newcode1747
>
>> content/browser/renderer_host/render_widget_host_view_aura.cc:1747: //
>> Ask the
>> RWH to drop reference to us.
>> On 2014/10/13 17:04:13, sky wrote:
>> > This comment isn't helpful. It doesn't describe why the view may not be
>>
> this.
>
>  I've added comment explaining when GetView() != this can happen.
>>
>
>
> https://chromiumcodereview.appspot.com/647613002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lazyboy, 2014-10-14 22:02:14]

> TBH this is adding workarounds for a hack that should never have been. RWH
> shouldn't have 2 views attached to it. RWHVG shouldn't use a "platform
> view"
> also attached to the same RWH. RenderWidgetHostViewChildFrame doesn't have
> one,
> doesn't need one.
> What is the plan to move RenderWidgetHostViewGuest to
> RenderWidgetHostViewChildFrame?

This requires Guest views to start using OOP iframe/RWHVChildFrame and is
being tracked in http://crbug.com/330264.
However this means we are dependent on OOP iframe to be complete, we have
OKRs this Q to push the work more from <webview> team along with the
site isolation team.



I think I would rather see an explicit "is_guest_view_hack_" in RWHVA (and
RWHVM) that is set in the constructor, which we would use so that we don't
call host_->SetView(this), nor host_->ViewDestroyed() (since we're not
host_'s view). But I would really like us to make progress to remove the
platform_view_ hack, and instead tear off platform-specific logic so that
it can be used in both the real RWHVA/M as well as the RWHVGuest (and
RWHVChildFrame?) - when it makes sense to do that logic locally instead of
forwarding to the parent renderer.

I've added the "is_guest_view_hack_" in patchset #6 as you suggested.

[piman, 2014-10-14 23:48:06]

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
content/browser/renderer_host/render_widget_host_view_mac.mm:578: // typically a
RenderWidgetHostViewChildFrame), then host_->GetView() won't
nit: "typically a RenderWidgetHostViewGuest" ?
Maybe the comment would be clearer as something along the lines of "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." ?

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:831:
base::Unretained(this)));
nit: how about creating a RunLoop and posting its QuitClosure() ?

RunLoop run_loop;
content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
run_loop.QuitClosure());
run_loop.Run();

Otherwise, CheckGuestView could be static and not require binding
Unretained(this)

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
File content/browser/web_contents/web_contents_view_guest.cc (right):

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
content/browser/web_contents/web_contents_view_guest.cc:165: return
platform_view_->CreateViewForPopupWidget(render_widget_host, true);
I don't think we should pass true. This is called when we want to create a
popup, with a new top-level RWH, but that view doesn't become a
RenderWidgetHostViewGuest's platform_view, right?

I don't think we need to pass the is_guest_view_hack in all the
CreateViewForPopupWidget's.

[lazyboy, 2014-10-15 04:50:11]

Address comments in patchset #8

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
content/browser/renderer_host/render_widget_host_view_mac.mm:578: // typically a
RenderWidgetHostViewChildFrame), then host_->GetView() won't
On 2014/10/14 23:48:06, piman (Very slow to review) wrote:
> nit: "typically a RenderWidgetHostViewGuest" ?
> Maybe the comment would be clearer as something along the lines of "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." ?

Thanks, Done.

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
(right):

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:831:
base::Unretained(this)));
On 2014/10/14 23:48:06, piman (Very slow to review) wrote:
> nit: how about creating a RunLoop and posting its QuitClosure() ?
> 
> RunLoop run_loop;
> content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
> run_loop.QuitClosure());
> run_loop.Run();

Done this one.
> 
> Otherwise, CheckGuestView could be static and not require binding
> Unretained(this)

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
File content/browser/web_contents/web_contents_view_guest.cc (right):

https://chromiumcodereview.appspot.com/647613002/diff/530001/content/browser/...
content/browser/web_contents/web_contents_view_guest.cc:165: return
platform_view_->CreateViewForPopupWidget(render_widget_host, true);
On 2014/10/14 23:48:06, piman (Very slow to review) wrote:
> I don't think we should pass true. This is called when we want to create a
> popup, with a new top-level RWH, but that view doesn't become a
> RenderWidgetHostViewGuest's platform_view, right?
> 
> I don't think we need to pass the is_guest_view_hack in all the
> CreateViewForPopupWidget's.

Oh that's right, these views don't have a wrapped RWHVGuest.
Reverted CreateViewForPopupWidget changes.

[piman, 2014-10-16 02:13:59]

lgtm

[lazyboy, 2014-10-16 06:14:24]

https://chromiumcodereview.appspot.com/647613002/diff/610001/content/browser/...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://chromiumcodereview.appspot.com/647613002/diff/610001/content/browser/...
content/browser/web_contents/web_contents_view_mac.mm:301: if
(!is_guest_view_hack && delegate()) {
I needed to add this check here to fix mac crash.
Otherwise CreateRenderWidgetHostViewDelegate call below calls
initWithRenderWidgetHost, which in turn calls rwh->GetView()->GetNativeView()
and crashes.
PTAL.

[piman, 2014-10-16 20:47:48]

https://chromiumcodereview.appspot.com/647613002/diff/610001/content/browser/...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://chromiumcodereview.appspot.com/647613002/diff/610001/content/browser/...
content/browser/web_contents/web_contents_view_mac.mm:301: if
(!is_guest_view_hack && delegate()) {
On 2014/10/16 06:14:24, lazyboy wrote:
> I needed to add this check here to fix mac crash.
> Otherwise CreateRenderWidgetHostViewDelegate call below calls
> initWithRenderWidgetHost, which in turn calls rwh->GetView()->GetNativeView()
> and crashes.
> PTAL.

I'm no mac expert, not fully sure about everything the delegate is used for.
Looking at the code, it seems to do swipe history navigation (irrelevant for
guest), but spelling too, which seems like it would be important with the
current hookup.

Would it make more sense to fix initWithRenderWidgetHost? I'm not sure setting
VIEW_ID_TAB_CONTAINER as the view ID is right, it's not a tab container in the
case of a guest (i.e. if renderWidgetHost_->GetView() is NULL).
Another way is to pass the NSView explicitly.
A third way is to call this on the RWHVGuest after it has been created - but I
think it'd do the wrong thing (hooking up to the embedder's NSView).

[lazyboy, 2014-10-17 00:46:00]

Patchset #11

https://chromiumcodereview.appspot.com/647613002/diff/610001/content/browser/...
File content/browser/web_contents/web_contents_view_mac.mm (right):

https://chromiumcodereview.appspot.com/647613002/diff/610001/content/browser/...
content/browser/web_contents/web_contents_view_mac.mm:301: if
(!is_guest_view_hack && delegate()) {
On 2014/10/16 20:47:47, piman (Very slow to review) wrote:
> On 2014/10/16 06:14:24, lazyboy wrote:
> > I needed to add this check here to fix mac crash.
> > Otherwise CreateRenderWidgetHostViewDelegate call below calls
> > initWithRenderWidgetHost, which in turn calls
rwh->GetView()->GetNativeView()
> > and crashes.
> > PTAL.
> 
> I'm no mac expert, not fully sure about everything the delegate is used for.
> Looking at the code, it seems to do swipe history navigation (irrelevant for
> guest), but spelling too, which seems like it would be important with the
> current hookup.
> 
> Would it make more sense to fix initWithRenderWidgetHost? I'm not sure setting
> VIEW_ID_TAB_CONTAINER as the view ID is right, it's not a tab container in the
> case of a guest (i.e. if renderWidgetHost_->GetView() is NULL).
> Another way is to pass the NSView explicitly.
> A third way is to call this on the RWHVGuest after it has been created - but I
> think it'd do the wrong thing (hooking up to the embedder's NSView).

I've done #2, check for GetView() before using it.

re: spellchecking, I was checking type and highlight spell mistakes in inputbox,
which seemed to work fine w/o this delegate(), but it seems this spellchecking
is a different mac specific one.
However, I'm getting blink assertion failures with or without this CL when I use
this spellchecking from guests, need to fix that separately.

[piman, 2014-10-17 03:37:58]

LGTM, thanks!

[lazyboy, 2014-10-17 05:39:12]

Scott, can I get LGTM for:
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_delegate.mm
Thanks.

[sky, 2014-10-20 15:48:01]

LGTM

[lazyboy, 2014-10-20 16:51:20]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-10-20 16:52:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/647613002/630001

[commit-bot: I haz the power, 2014-10-20 17:16:22]

Committed patchset #11 (id:630001)

[commit-bot: I haz the power, 2014-10-20 17:17:20]

Patchset 11 (id:??) landed as
https://crrev.com/549b3b4f0cf5691d093823f285ffd437614af1f6
Cr-Commit-Position: refs/heads/master@{#300286}