Re-enable SSLUITest.TestBadHTTPSDownload and get test exiting cleanly.

Re-enable SSLUITest.TestBadHTTPSDownload and get test exiting cleanly.

BUG=143953, 95331
TEST=browser_tests --gtest_filter=SSLUITest.TestBadHTTPSDownload

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152685

[pauljensen, 2012-08-15 16:35:57]

Scott,

Could you please review my change to re-enable a test and get it working again
(its been disabled almost a year).  To get it working again I just had to
complete the download to avoid a modal dialog box complaining about exiting
while a download is in progress.

Thanks,
Paul

[sky, 2012-08-15 16:56:31]

LGTM

[Randy Smith (Not in Mondays), 2012-08-15 18:27:05]

https://chromiumcodereview.appspot.com/10837262/diff/1/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://chromiumcodereview.appspot.com/10837262/diff/1/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:688:
DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_ACCEPT);
You want to split these two lines apart, and make sure the DTOT is created
before the navigation that creates the download.  Otherwise, you'll have the
potential race where the download completes before this line, and the observer
waits forever (no download will complete after it is created).

[pauljensen, 2012-08-15 18:59:01]

I addressed Randy's race condition possibility.

[Randy Smith (Not in Mondays), 2012-08-15 19:01:16]

https://chromiumcodereview.appspot.com/10837262/diff/2002/chrome/browser/ssl/...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://chromiumcodereview.appspot.com/10837262/diff/2002/chrome/browser/ssl/...
chrome/browser/ssl/ssl_browser_tests.cc:671:
tab->GetInterstitialPage()->Proceed();
I believe this is the line at which the download would actually be initiated,
but I'm not sure; Matt, could you check me on that?

[mmenke, 2012-08-15 19:59:22]

https://chromiumcodereview.appspot.com/10837262/diff/2002/chrome/browser/ssl/...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://chromiumcodereview.appspot.com/10837262/diff/2002/chrome/browser/ssl/...
chrome/browser/ssl/ssl_browser_tests.cc:671:
tab->GetInterstitialPage()->Proceed();
On 2012/08/15 19:01:16, rdsmith wrote:
> I believe this is the line at which the download would actually be initiated,
> but I'm not sure; Matt, could you check me on that?

Sorry, meant to response earlier, but got distracted.

I'm no expert, but...  Showing an interstitial pauses all requests, and the
blocked request never gets a socket assigned to it, so it can't have received
any headers, which is when we start the download.  Proceeding will resume the
blocked request (And all other paused requests as well), at which point the
download should actually start.

Not sure if it's correct to call this the line at which the download is
initiated, since it's actually initiated asynchronously on the IO thread, but
basically, yea, that's right.

[pauljensen, 2012-08-17 16:48:43]

I moved up the DownloadTestObserverTerminal so its created before the download
is initiated.  I'm going to spend some time today reading this code so I have a
more thorough understanding of what's going on.

[pauljensen, 2012-08-21 13:53:40]

Randy or Matt,

Could you take a look at my latest patch set?

Thank you!
Paul

[mmenke, 2012-08-21 14:07:50]

I'm not at all familiar with the test.  One comment, though:  The bug ID
referenced in the CL description is marked as a duplicate of a bug with
restricted access...

Either the bug should be de-duped, or that's the wrong bug ID.

[pauljensen, 2012-08-21 14:48:06]

95331 is a report of a crash in this test.  85408 is the root cause of the
crash.  I just added you to 85408.  I was marking this CL as addressing 95331 as
the disabling of the test was done referencing 95331.  Should I instead
reference 85408?

On 2012/08/21 14:07:50, Matt Menke wrote:
> I'm not at all familiar with the test.  One comment, though:  The bug ID
> referenced in the CL description is marked as a duplicate of a bug with
> restricted access...
> 
> Either the bug should be de-duped, or that's the wrong bug ID.

[mmenke, 2012-08-21 15:00:44]

On 2012/08/21 14:48:06, pauljensen wrote:
> 95331 is a report of a crash in this test.  85408 is the root cause of the
> crash.  I just added you to 85408.  I was marking this CL as addressing 95331
as
> the disabling of the test was done referencing 95331.  Should I instead
> reference 85408?

95331 is marked as a duplicate of a bug that's been fixed.  Therefore, this CL
addresses no open bug, which is somewhat weird, since it's doing more than just
re-enabling a test.

It looks to me like 95331 is just for a test fixture shutdown race (And looking
at this code seems to confirm that), which doesn't sound like to me like issue
85408.  Perhaps it should be marked blocked on 85408 instead - I'm not sure.

I don't want to harp too much on this; it's just strange, and leaves me
scratching my head over how the two bugs are related (Admittedly, I'm not
terribly familiar with the download code).

[Randy Smith (Not in Mondays), 2012-08-21 16:33:08]

On 2012/08/21 15:00:44, Matt Menke wrote:
> On 2012/08/21 14:48:06, pauljensen wrote:
> > 95331 is a report of a crash in this test.  85408 is the root cause of the
> > crash.  I just added you to 85408.  I was marking this CL as addressing
95331
> as
> > the disabling of the test was done referencing 95331.  Should I instead
> > reference 85408?
> 
> 95331 is marked as a duplicate of a bug that's been fixed.  Therefore, this CL
> addresses no open bug, which is somewhat weird, since it's doing more than
just
> re-enabling a test.
> 
> It looks to me like 95331 is just for a test fixture shutdown race (And
looking
> at this code seems to confirm that), which doesn't sound like to me like issue
> 85408.  Perhaps it should be marked blocked on 85408 instead - I'm not sure.
> 
> I don't want to harp too much on this; it's just strange, and leaves me
> scratching my head over how the two bugs are related (Admittedly, I'm not
> terribly familiar with the download code).

This test was disabled because of a crash whose root cause was 85408, but was
not re-enabled when that bug was fixed (given the somewhat
whimper-not-bang-fashion in which that bug was fixed, this doesn't surprise me).
 The test was also written in a fashion that didn't clean up after itself
(created a download and didn't cancel it) which resulted in a timeout as the
test prompted the user if it was ok to exit with pending downloads.  So when
Paul went to re-enable it, he had to do a little bit more work.

To my mind, the "most right" thing to do would be:
* Create a new bug that describes the problem that occurred when the test was
re-enabled.
* Make this CL point at that bug and 95331.  Using 95331 instead of 85408 is in
some sense wrong (as the root cause is 85408) but the connection with the actual
change is clearer,   Using a bug that's been closed (duplicate or fixed,
depending on which you pick) for a change that should have happened when that
bug was closed strikes me as a fine thing.

Having said all that, my care factor isn't high.  LGTM.  :-}  

(Matt: If based on the above you think I'm playing too fast and loose, please
raise a flag.)

[mmenke, 2012-08-21 16:36:03]

On 2012/08/21 16:33:08, rdsmith wrote:
> On 2012/08/21 15:00:44, Matt Menke wrote:
> > On 2012/08/21 14:48:06, pauljensen wrote:
> > > 95331 is a report of a crash in this test.  85408 is the root cause of the
> > > crash.  I just added you to 85408.  I was marking this CL as addressing
> 95331
> > as
> > > the disabling of the test was done referencing 95331.  Should I instead
> > > reference 85408?
> > 
> > 95331 is marked as a duplicate of a bug that's been fixed.  Therefore, this
CL
> > addresses no open bug, which is somewhat weird, since it's doing more than
> just
> > re-enabling a test.
> > 
> > It looks to me like 95331 is just for a test fixture shutdown race (And
> looking
> > at this code seems to confirm that), which doesn't sound like to me like
issue
> > 85408.  Perhaps it should be marked blocked on 85408 instead - I'm not sure.
> > 
> > I don't want to harp too much on this; it's just strange, and leaves me
> > scratching my head over how the two bugs are related (Admittedly, I'm not
> > terribly familiar with the download code).
> 
> This test was disabled because of a crash whose root cause was 85408, but was
> not re-enabled when that bug was fixed (given the somewhat
> whimper-not-bang-fashion in which that bug was fixed, this doesn't surprise
me).
>  The test was also written in a fashion that didn't clean up after itself
> (created a download and didn't cancel it) which resulted in a timeout as the
> test prompted the user if it was ok to exit with pending downloads.  So when
> Paul went to re-enable it, he had to do a little bit more work.
> 
> To my mind, the "most right" thing to do would be:
> * Create a new bug that describes the problem that occurred when the test was
> re-enabled.
> * Make this CL point at that bug and 95331.  Using 95331 instead of 85408 is
in
> some sense wrong (as the root cause is 85408) but the connection with the
actual
> change is clearer,   Using a bug that's been closed (duplicate or fixed,
> depending on which you pick) for a change that should have happened when that
> bug was closed strikes me as a fine thing.
> 
> Having said all that, my care factor isn't high.  LGTM.  :-}  
> 
> (Matt: If based on the above you think I'm playing too fast and loose, please
> raise a flag.)

That sounds reasonable to me.

[mmenke, 2012-08-21 16:38:03]

On 2012/08/21 16:36:03, Matt Menke wrote:
> On 2012/08/21 16:33:08, rdsmith wrote:
> > On 2012/08/21 15:00:44, Matt Menke wrote:
> > > On 2012/08/21 14:48:06, pauljensen wrote:
> > > > 95331 is a report of a crash in this test.  85408 is the root cause of
the
> > > > crash.  I just added you to 85408.  I was marking this CL as addressing
> > 95331
> > > as
> > > > the disabling of the test was done referencing 95331.  Should I instead
> > > > reference 85408?
> > > 
> > > 95331 is marked as a duplicate of a bug that's been fixed.  Therefore,
this
> CL
> > > addresses no open bug, which is somewhat weird, since it's doing more than
> > just
> > > re-enabling a test.
> > > 
> > > It looks to me like 95331 is just for a test fixture shutdown race (And
> > looking
> > > at this code seems to confirm that), which doesn't sound like to me like
> issue
> > > 85408.  Perhaps it should be marked blocked on 85408 instead - I'm not
sure.
> > > 
> > > I don't want to harp too much on this; it's just strange, and leaves me
> > > scratching my head over how the two bugs are related (Admittedly, I'm not
> > > terribly familiar with the download code).
> > 
> > This test was disabled because of a crash whose root cause was 85408, but
was
> > not re-enabled when that bug was fixed (given the somewhat
> > whimper-not-bang-fashion in which that bug was fixed, this doesn't surprise
> me).
> >  The test was also written in a fashion that didn't clean up after itself
> > (created a download and didn't cancel it) which resulted in a timeout as the
> > test prompted the user if it was ok to exit with pending downloads.  So when
> > Paul went to re-enable it, he had to do a little bit more work.
> > 
> > To my mind, the "most right" thing to do would be:
> > * Create a new bug that describes the problem that occurred when the test
was
> > re-enabled.
> > * Make this CL point at that bug and 95331.  Using 95331 instead of 85408 is
> in
> > some sense wrong (as the root cause is 85408) but the connection with the
> actual
> > change is clearer,   Using a bug that's been closed (duplicate or fixed,
> > depending on which you pick) for a change that should have happened when
that
> > bug was closed strikes me as a fine thing.
> > 
> > Having said all that, my care factor isn't high.  LGTM.  :-}  
> > 
> > (Matt: If based on the above you think I'm playing too fast and loose,
please
> > raise a flag.)
> 
> That sounds reasonable to me.

Oh, and just to add - I'm fine with a closed bug for CLs that enable tests, or
do minor refactorings.  But when fixing another bug that has nothing to do with
the bug the original bug was duped again, seems to be stretching things a bit.

[commit-bot: I haz the power, 2012-08-21 18:19:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/10837262/11001

[commit-bot: I haz the power, 2012-08-21 21:23:54]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-08-21 21:33:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/10837262/11001

[commit-bot: I haz the power, 2012-08-22 00:19:54]

Change committed as 152685

[pauljensen, 2012-08-27 15:50:52]

Please review my latest version of this test.  The test began failing again.  I
believe it was because after the test is run one hundred times and it downloads
dangerous.exe, dangerous (1).exe ... dangerous (100).exe, then Chrome prompts
the user to give a name for the downloaded file...which makes the test hang and
fail.  I implemented the simply fix that download_browsertest.cc uses to avoid
this problem.

[Randy Smith (Not in Mondays), 2012-08-27 15:52:24]

LGTM.  Sorry for not thinking of this on the last round.