Add tests for ShellWindowLinkDelegate::OpenURLFromTab

Add tests for ShellWindowLinkDelegate::OpenURLFromTab

This code relies on the WebContents implementation not dereferencing the source
WebContents after ShellWindowLinkDelegate frees it. Add a test that exercises
this code to prevent changes to the WebContents implementation causing it to
crash.

BUG=254260
TEST=browser_tests --gtest_filter=PlatformAppBrowserTest.ExternalOpenLink

[zhchbin, 2013-07-01 06:24:38]

https://codereview.chromium.org/18192003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/18192003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:321: if
(disable_external_open_for_testing_) {
Can we remove this "disable_external_open_for_testing_" and the following code,
it seems to be useful for the test case: PlatformAppBrowserTest.OpenLink only.
And here is some duplicated code.

[Ken Rockot(use gerrit already), 2013-07-01 15:02:47]

This does not address the bug in question, and it explicitly crafts a test we
don't want to be running.

On 2013/07/01 06:24:38, zhchbin wrote:
>
https://codereview.chromium.org/18192003/diff/1/chrome/browser/ui/extensions/...
> File chrome/browser/ui/extensions/shell_window.cc (right):
> 
>
https://codereview.chromium.org/18192003/diff/1/chrome/browser/ui/extensions/...
> chrome/browser/ui/extensions/shell_window.cc:321: if
> (disable_external_open_for_testing_) {
> Can we remove this "disable_external_open_for_testing_" and the following
code,
> it seems to be useful for the test case: PlatformAppBrowserTest.OpenLink only.
> And here is some duplicated code.

[Ken Rockot(use gerrit already), 2013-07-01 15:02:54]

https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:618: 
First, this is not the right test for the associated bug.

Second, this explicitly violates the purpose of the
disable_external_open_for_testing_ flag, which was to prevent OpenExternal from
being called by tests that open new-window links.

https://codereview.chromium.org/18192003/diff/4001/chrome/browser/ui/apps/chr...
File chrome/browser/ui/apps/chrome_shell_window_delegate.h (right):

https://codereview.chromium.org/18192003/diff/4001/chrome/browser/ui/apps/chr...
chrome/browser/ui/apps/chrome_shell_window_delegate.h:23: static void
EnableExternalOpenForTesting();
This is never needed.

[zhchbin, 2013-07-02 08:59:06]

https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:618: 
On 2013/07/01 15:02:54, rockot wrote:
> First, this is not the right test for the associated bug.
> 
> Second, this explicitly violates the purpose of the
> disable_external_open_for_testing_ flag, which was to prevent OpenExternal
from
> being called by tests that open new-window links.

Sorry for my mistake first of all. However, when
disable_external_open_for_testing_ is false, the test case will call
ShellWindowLinkDelegate::OpenURLFromTab. Why do you say "this is not the right
test for the associated bug."?

[Ken Rockot(use gerrit already), 2013-07-02 16:27:15]

On 2013/07/02 08:59:06, zhchbin wrote:
>
https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
> File chrome/browser/extensions/platform_app_browsertest.cc (right):
> 
>
https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
> chrome/browser/extensions/platform_app_browsertest.cc:618: 
> On 2013/07/01 15:02:54, rockot wrote:
> > First, this is not the right test for the associated bug.
> > 
> > Second, this explicitly violates the purpose of the
> > disable_external_open_for_testing_ flag, which was to prevent OpenExternal
> from
> > being called by tests that open new-window links.
> 
> Sorry for my mistake first of all. However, when
> disable_external_open_for_testing_ is false, the test case will call
> ShellWindowLinkDelegate::OpenURLFromTab. Why do you say "this is not the right
> test for the associated bug."?

The bug calls for a test to verify that it's safe to delete a WebContents from
its delegate's OpenURLFromTab impl. While the test you provided does accomplish
this, it does so by forcing ShellWindowLinkDelegate::OpenURLFromTab to be
called. This should never be called from a test, which is why
disable_external_open_for_testing_ exists.

Your effort is appreciated though! If you still want to try to tackle this, you
can do it with a simple unit test that uses a custom delegate on an empty
WebContents. Invoking WebContents::RequestOpenURL should trigger a call to the
delegate's OpenURLFromTab.

[zhchbin, 2013-07-08 06:35:44]

On 2013/07/02 16:27:15, rockot wrote:
> On 2013/07/02 08:59:06, zhchbin wrote:
> >
>
https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
> > File chrome/browser/extensions/platform_app_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
> > chrome/browser/extensions/platform_app_browsertest.cc:618: 
> > On 2013/07/01 15:02:54, rockot wrote:
> > > First, this is not the right test for the associated bug.
> > > 
> > > Second, this explicitly violates the purpose of the
> > > disable_external_open_for_testing_ flag, which was to prevent OpenExternal
> > from
> > > being called by tests that open new-window links.
> > 
> > Sorry for my mistake first of all. However, when
> > disable_external_open_for_testing_ is false, the test case will call
> > ShellWindowLinkDelegate::OpenURLFromTab. Why do you say "this is not the
right
> > test for the associated bug."?
> 
> The bug calls for a test to verify that it's safe to delete a WebContents from
> its delegate's OpenURLFromTab impl. While the test you provided does
accomplish
> this, it does so by forcing ShellWindowLinkDelegate::OpenURLFromTab to be
> called. This should never be called from a test, which is why
> disable_external_open_for_testing_ exists.
> 
> Your effort is appreciated though! If you still want to try to tackle this,
you
> can do it with a simple unit test that uses a custom delegate on an empty
> WebContents. Invoking WebContents::RequestOpenURL should trigger a call to the
> delegate's OpenURLFromTab.

Does the "disable_external_open_for_testing_" is used to prevent the
use-after-free error reported in https://chromiumcodereview.appspot.com/10915047
?

If yes, I want to share something that I have found. If anything wrong, hope all
of you can forgive me.
First of all, let's find the root cause of ASAN use-after-free errors in
PlatformAppBrowserTest.OpenLink test. When the patch (10915047) is landed, the
implementation of WebContentsImpl::OpenURL is as follow (You can find it has
been changed since https://chromiumcodereview.appspot.com/11464034)

WebContents* new_contents = delegate_->OpenURLFromTab(this, params);
// Notify observers.
FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                  DidOpenURL(params.url, params.referrer,
                             params.disposition, params.transition));
return new_contents;

The delegate_ delete "this" in his implementation of OpenURLFromTab. Notice that
"observers_", the member variable of the WebContentsImpl, is read after the
OpenURLFromTab. The use-after-free error is introduced from here.

So now the code is changed, does it still need to use
"disable_external_open_for_testing_"?

What's more, now the test case "PlatformAppBrowserTest.OpenLink" is testing
something that never happen in the user side? It seems to be meaningless.

And thanks for the method that @rockot offered me to test the
ShellWindowLinkDelegate::OpenURLFromTab. But, RequestOpenURL is the method of
WebContentsImpl, not the WebContents, which couldn't be call out of the content
module from my point of view. As well as the RequestTransferURL of
WebContentsImpl. I find this when I want to "git cl upload" the test case as
@rockot's instruction.

By the way, I find a memory leak related to ShellWindowLinkDelegate also. The
codereview: https://codereview.chromium.org/18051015/ fix it.

[zhchbin, 2013-07-15 08:21:30]

On 2013/07/08 06:35:44, zhchbin wrote:
> On 2013/07/02 16:27:15, rockot wrote:
> > On 2013/07/02 08:59:06, zhchbin wrote:
> > >
> >
>
https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
> > > File chrome/browser/extensions/platform_app_browsertest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
> > > chrome/browser/extensions/platform_app_browsertest.cc:618: 
> > > On 2013/07/01 15:02:54, rockot wrote:
> > > > First, this is not the right test for the associated bug.
> > > > 
> > > > Second, this explicitly violates the purpose of the
> > > > disable_external_open_for_testing_ flag, which was to prevent
OpenExternal
> > > from
> > > > being called by tests that open new-window links.
> > > 
> > > Sorry for my mistake first of all. However, when
> > > disable_external_open_for_testing_ is false, the test case will call
> > > ShellWindowLinkDelegate::OpenURLFromTab. Why do you say "this is not the
> right
> > > test for the associated bug."?
> > 
> > The bug calls for a test to verify that it's safe to delete a WebContents
from
> > its delegate's OpenURLFromTab impl. While the test you provided does
> accomplish
> > this, it does so by forcing ShellWindowLinkDelegate::OpenURLFromTab to be
> > called. This should never be called from a test, which is why
> > disable_external_open_for_testing_ exists.
> > 
> > Your effort is appreciated though! If you still want to try to tackle this,
> you
> > can do it with a simple unit test that uses a custom delegate on an empty
> > WebContents. Invoking WebContents::RequestOpenURL should trigger a call to
the
> > delegate's OpenURLFromTab.
> 
> Does the "disable_external_open_for_testing_" is used to prevent the
> use-after-free error reported in
https://chromiumcodereview.appspot.com/10915047
> ?
> 
> If yes, I want to share something that I have found. If anything wrong, hope
all
> of you can forgive me.
> First of all, let's find the root cause of ASAN use-after-free errors in
> PlatformAppBrowserTest.OpenLink test. When the patch (10915047) is landed, the
> implementation of WebContentsImpl::OpenURL is as follow (You can find it has
> been changed since https://chromiumcodereview.appspot.com/11464034)
> 
> WebContents* new_contents = delegate_->OpenURLFromTab(this, params);
> // Notify observers.
> FOR_EACH_OBSERVER(WebContentsObserver, observers_,
>                   DidOpenURL(params.url, params.referrer,
>                              params.disposition, params.transition));
> return new_contents;
> 
> The delegate_ delete "this" in his implementation of OpenURLFromTab. Notice
that
> "observers_", the member variable of the WebContentsImpl, is read after the
> OpenURLFromTab. The use-after-free error is introduced from here.
> 
> So now the code is changed, does it still need to use
> "disable_external_open_for_testing_"?
> 
> What's more, now the test case "PlatformAppBrowserTest.OpenLink" is testing
> something that never happen in the user side? It seems to be meaningless.
> 
> And thanks for the method that @rockot offered me to test the
> ShellWindowLinkDelegate::OpenURLFromTab. But, RequestOpenURL is the method of
> WebContentsImpl, not the WebContents, which couldn't be call out of the
content
> module from my point of view. As well as the RequestTransferURL of
> WebContentsImpl. I find this when I want to "git cl upload" the test case as
> @rockot's instruction.
> 
> By the way, I find a memory leak related to ShellWindowLinkDelegate also. The
> codereview: https://codereview.chromium.org/18051015/ fix it.

Ping again, any comment?

[benwells, 2013-07-16 00:31:00]

On 2013/07/15 08:21:30, zhchbin wrote:
> On 2013/07/08 06:35:44, zhchbin wrote:
> > On 2013/07/02 16:27:15, rockot wrote:
> > > On 2013/07/02 08:59:06, zhchbin wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
> > > > File chrome/browser/extensions/platform_app_browsertest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/18192003/diff/4001/chrome/browser/extensions/...
> > > > chrome/browser/extensions/platform_app_browsertest.cc:618: 
> > > > On 2013/07/01 15:02:54, rockot wrote:
> > > > > First, this is not the right test for the associated bug.
> > > > > 
> > > > > Second, this explicitly violates the purpose of the
> > > > > disable_external_open_for_testing_ flag, which was to prevent
> OpenExternal
> > > > from
> > > > > being called by tests that open new-window links.
> > > > 
> > > > Sorry for my mistake first of all. However, when
> > > > disable_external_open_for_testing_ is false, the test case will call
> > > > ShellWindowLinkDelegate::OpenURLFromTab. Why do you say "this is not the
> > right
> > > > test for the associated bug."?
> > > 
> > > The bug calls for a test to verify that it's safe to delete a WebContents
> from
> > > its delegate's OpenURLFromTab impl. While the test you provided does
> > accomplish
> > > this, it does so by forcing ShellWindowLinkDelegate::OpenURLFromTab to be
> > > called. This should never be called from a test, which is why
> > > disable_external_open_for_testing_ exists.
> > > 
> > > Your effort is appreciated though! If you still want to try to tackle
this,
> > you
> > > can do it with a simple unit test that uses a custom delegate on an empty
> > > WebContents. Invoking WebContents::RequestOpenURL should trigger a call to
> the
> > > delegate's OpenURLFromTab.
> > 
> > Does the "disable_external_open_for_testing_" is used to prevent the
> > use-after-free error reported in
> https://chromiumcodereview.appspot.com/10915047
> > ?
> > 
> > If yes, I want to share something that I have found. If anything wrong, hope
> all
> > of you can forgive me.
> > First of all, let's find the root cause of ASAN use-after-free errors in
> > PlatformAppBrowserTest.OpenLink test. When the patch (10915047) is landed,
the
> > implementation of WebContentsImpl::OpenURL is as follow (You can find it has
> > been changed since https://chromiumcodereview.appspot.com/11464034)
> > 
> > WebContents* new_contents = delegate_->OpenURLFromTab(this, params);
> > // Notify observers.
> > FOR_EACH_OBSERVER(WebContentsObserver, observers_,
> >                   DidOpenURL(params.url, params.referrer,
> >                              params.disposition, params.transition));
> > return new_contents;
> > 
> > The delegate_ delete "this" in his implementation of OpenURLFromTab. Notice
> that
> > "observers_", the member variable of the WebContentsImpl, is read after the
> > OpenURLFromTab. The use-after-free error is introduced from here.
> > 
> > So now the code is changed, does it still need to use
> > "disable_external_open_for_testing_"?
> > 
> > What's more, now the test case "PlatformAppBrowserTest.OpenLink" is testing
> > something that never happen in the user side? It seems to be meaningless.
> > 
> > And thanks for the method that @rockot offered me to test the
> > ShellWindowLinkDelegate::OpenURLFromTab. But, RequestOpenURL is the method
of
> > WebContentsImpl, not the WebContents, which couldn't be call out of the
> content
> > module from my point of view. As well as the RequestTransferURL of
> > WebContentsImpl. I find this when I want to "git cl upload" the test case as
> > @rockot's instruction.
> > 
> > By the way, I find a memory leak related to ShellWindowLinkDelegate also.
The
> > codereview: https://codereview.chromium.org/18051015/ fix it.
> 
> Ping again, any comment?

FYI I'm waiting for rockot's lg before looking at this CL as he wrote the code
you're testing.