Create content::WebContentsDestroyedWatcher, use it in many tests.

Create content::WebContentsDestroyedWatcher, use it in many tests.

BUG=170921
TEST=all the tests still work

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229059

[Avi (use Gerrit), 2013-10-15 14:44:18]

jam@chromium.org: content
thestig@chromium.org: chrome

This is a high-level enough change that I don't think I need more granular
owners. Let me know if you disagree.

[jam, 2013-10-15 16:50:52]

https://codereview.chromium.org/26277010/diff/39001/chrome/browser/printing/p...
File chrome/browser/printing/print_dialog_cloud_interative_uitest.cc (left):

https://codereview.chromium.org/26277010/diff/39001/chrome/browser/printing/p...
chrome/browser/printing/print_dialog_cloud_interative_uitest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
i dont think you meant to delete this?

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
content/public/test/browser_test_utils.h:227: class WebContentsDestroyedWatcher
: public WebContentsObserver {
i'm curious why this API isn't just a method? i.e.

CONTENT_EXPORT void WaitForWebContentsDestroyed(WebContents* web_contents)?

[Avi (use Gerrit), 2013-10-15 16:57:24]

https://codereview.chromium.org/26277010/diff/39001/chrome/browser/printing/p...
File chrome/browser/printing/print_dialog_cloud_interative_uitest.cc (left):

https://codereview.chromium.org/26277010/diff/39001/chrome/browser/printing/p...
chrome/browser/printing/print_dialog_cloud_interative_uitest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/10/15 16:50:52, jam wrote:
> i dont think you meant to delete this?

Yes I did. In discussions with Scott Byer, that seemed like the best option
since this is an obsolete test.

I will ask Scott to comment here.

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
content/public/test/browser_test_utils.h:227: class WebContentsDestroyedWatcher
: public WebContentsObserver {
On 2013/10/15 16:50:52, jam wrote:
> i'm curious why this API isn't just a method? i.e.
> 
> CONTENT_EXPORT void WaitForWebContentsDestroyed(WebContents* web_contents)?

Er... yeah. Good point, I hadn't thought about simplifying it that much.

[Avi (use Gerrit), 2013-10-15 16:58:04]

Scott re the printing test.

[Scott Byer, 2013-10-15 17:37:18]

On 2013/10/15 16:58:04, Avi wrote:
> Scott re the printing test.

Yeah, there's one test in that file, just tries to bring up the old-style print
dialog and shut it down. That dialog is used only when there isn't a PDF plug-in
present (otherwise printing goes through the preview path). There's no easy way
to get the web contents of the dialog without a bunch of extra plumbing AFAICT,
but the last time I was in there was 2 years ago, so that could have changed. If
there was a one-line way of getting the front dialog web contents, then it might
be worth keeping the test. Without that, it's a lot of work for very little
benefit. Ideally, the dialog code itself could go away, but that would require
an open-source PDF plug-in, which unfortunately hasn't happened yet.

[Avi (use Gerrit), 2013-10-15 20:16:31]

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
content/public/test/browser_test_utils.h:227: class WebContentsDestroyedWatcher
: public WebContentsObserver {
Actually, no, that's not a good design. That just begs for this to be flaky.

The flow should be:
- Start watching a WebContents
- Cause some action that will make it go away
- Wait for the destruction

There are two cases here.

One is that the test starts waiting for the destruction, and then the
destruction happens. Sure, if that were to be the only case, then yes,
WaitForWebContentsDestroyed would make sense.

But the other case is that the web contents gets destroyed before the waiting.
If watching the WebContents and waiting for it are all rolled up into one
function, then watching it comes waaaay to late and the test will hang.

This split between watching the WebContents and waiting for it to die is a Good
Thing.

[jam, 2013-10-15 20:47:14]

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
content/public/test/browser_test_utils.h:227: class WebContentsDestroyedWatcher
: public WebContentsObserver {
On 2013/10/15 20:16:32, Avi wrote:
> Actually, no, that's not a good design. That just begs for this to be flaky.
> 
> The flow should be:
> - Start watching a WebContents
> - Cause some action that will make it go away
> - Wait for the destruction
> 
> There are two cases here.
> 
> One is that the test starts waiting for the destruction, and then the
> destruction happens. Sure, if that were to be the only case, then yes,
> WaitForWebContentsDestroyed would make sense.
> 
> But the other case is that the web contents gets destroyed before the waiting.
> If watching the WebContents and waiting for it are all rolled up into one
> function, then watching it comes waaaay to late and the test will hang.
> 
> This split between watching the WebContents and waiting for it to die is a
Good
> Thing.

Yes I understand why we use that pattern in other places. I'm wondering about
this specific case of waiting for web contents destruction. Looking at the usage
of this method, it seems that none of them could happen before the Wait is
called.

[jam, 2013-10-15 20:48:04]

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/26277010/diff/39001/content/public/test/brows...
content/public/test/browser_test_utils.h:227: class WebContentsDestroyedWatcher
: public WebContentsObserver {
On 2013/10/15 20:47:15, jam wrote:
> On 2013/10/15 20:16:32, Avi wrote:
> > Actually, no, that's not a good design. That just begs for this to be flaky.
> > 
> > The flow should be:
> > - Start watching a WebContents
> > - Cause some action that will make it go away
> > - Wait for the destruction
> > 
> > There are two cases here.
> > 
> > One is that the test starts waiting for the destruction, and then the
> > destruction happens. Sure, if that were to be the only case, then yes,
> > WaitForWebContentsDestroyed would make sense.
> > 
> > But the other case is that the web contents gets destroyed before the
waiting.
> > If watching the WebContents and waiting for it are all rolled up into one
> > function, then watching it comes waaaay to late and the test will hang.
> > 
> > This split between watching the WebContents and waiting for it to die is a
> Good
> > Thing.
> 
> Yes I understand why we use that pattern in other places. I'm wondering about
> this specific case of waiting for web contents destruction. Looking at the
usage
> of this method, it seems that none of them could happen before the Wait is
> called.

also, in this situation, if the WC is destroyed before Wait is called, it would
be very trivial to see this because instead of the usual timeout, it would be a
crash when calling the Observe method on WC

[Avi (use Gerrit), 2013-10-15 21:06:38]

On 2013/10/15 20:48:04, jam wrote:
> > Yes I understand why we use that pattern in other places. I'm wondering
about
> > this specific case of waiting for web contents destruction. Looking at the
> usage
> > of this method, it seems that none of them could happen before the Wait is
> > called.

I'm not sure I agree. In the unload browsertest, I had to move the watcher to
before the action that preceded it to make it not hang.

> also, in this situation, if the WC is destroyed before Wait is called, it
would
> be very trivial to see this because instead of the usual timeout, it would be
a
> crash when calling the Observe method on WC

Well... only probably a crash. We hammer on dead memory all the time, as you can
see from the ASAN exception list. In any case, I'm not seeing the certainty that
you have.

[jam, 2013-10-16 01:12:50]

On 2013/10/15 21:06:38, Avi wrote:
> On 2013/10/15 20:48:04, jam wrote:
> > > Yes I understand why we use that pattern in other places. I'm wondering
> about
> > > this specific case of waiting for web contents destruction. Looking at the
> > usage
> > > of this method, it seems that none of them could happen before the Wait is
> > > called.
> 
> I'm not sure I agree. In the unload browsertest, I had to move the watcher to
> before the action that preceded it to make it not hang.

in that test, the code was actually not needed as that method will always
synchronously delete the WebContents..

> 
> > also, in this situation, if the WC is destroyed before Wait is called, it
> would
> > be very trivial to see this because instead of the usual timeout, it would
be
> a
> > crash when calling the Observe method on WC
> 
> Well... only probably a crash. We hammer on dead memory all the time, as you
can
> see from the ASAN exception list. In any case, I'm not seeing the certainty
that
> you have.

[Avi (use Gerrit), 2013-10-16 02:55:19]

On 2013/10/16 01:12:50, jam wrote:
> in that test, the code was actually not needed as that method will always
> synchronously delete the WebContents..

We're talking about fast-tab-closure. The whole point of fast-tab-closure is
that while tab removal is synchronous, deletion is *not*. The waiting for the
close is there for a reason.

If all these test writers wrote tests that implied that they needed nested
message loops to spin, why are you expecting me to second-guess them?

[jam, 2013-10-16 16:15:26]

On 2013/10/16 02:55:19, Avi wrote:
> On 2013/10/16 01:12:50, jam wrote:
> > in that test, the code was actually not needed as that method will always
> > synchronously delete the WebContents..
> 
> We're talking about fast-tab-closure. The whole point of fast-tab-closure is
> that while tab removal is synchronous, deletion is *not*. The waiting for the
> close is there for a reason.

I'm not sure what you mean? I was referring to the change in
unload_browsertest.cc which you mentioned as an example of why the waiting
object needs to be created before calling Wait. In that specific example, the
Wait() is not needed at all. If you trace the chrome::CloseTab(browser()); call
in that test, it always deletes the WebContents before it returns.

> 
> If all these test writers wrote tests that implied that they needed nested
> message loops to spin, why are you expecting me to second-guess them?

[Avi (use Gerrit), 2013-10-16 16:32:52]

On 2013/10/16 16:15:26, jam wrote:
> If you trace the chrome::CloseTab(browser()); call
> in that test, it always deletes the WebContents before it returns.

You're claiming that the comment is completely incorrect and the test is
completely broken. The problem is that the test is flaky, not failing. So is
this needed in some situations? I will admit to not fully understanding what's
going on here in the test, but...

I feel like switching a whole bunch of tests I don't fully understand from a
windowed observer to a non-windowed observer is presumptuous of me. I'm trying
to get off of notifications while retaining as much of the semantics as
possible. Again, I'm trying to not second-guess the authors of the tests (except
for that whole fast-tab-closure one).

[jam, 2013-10-16 19:52:12]

lgtm

[Lei Zhang, 2013-10-16 19:56:11]

chrome/ lgtm

[Scott Byer, 2013-10-16 19:57:14]

Printing LGTM

[commit-bot: I haz the power, 2013-10-16 21:25:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/26277010/39001

[commit-bot: I haz the power, 2013-10-17 01:34:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/26277010/39001

[commit-bot: I haz the power, 2013-10-17 05:51:45]

Change committed as 229059