Add test case to see if HTTP auth interstitials work nicely with other interstitials

Add test case to see if HTTP auth interstitials work nicely with other interstitials

Cross-origin HTTP auth dialogs are shown over a blank interstitial.
This didn't work nicely with other interstitials, e.g. an HTTPS resource
that triggered a basic HTTP auth dialog would show the SSL warning, and when
the user clicked through the warning, the login interstitial would show
on top of the SSL interstitial, rather than a blank page.
crrev.com/410373003 accidentally fixed that issue. This CL adds tests
for the scenario described above.

BUG=393597
TEST=LoginPromptBrowserTest.LoginInterstitialShouldReplaceExistingInterstitial

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

[meacer, 2014-07-14 23:28:07]

Nasko, can you please take a look?

The problem is that this line assumes the current interstitial is always the
newly created login interstitial and it tries to proceed through:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

However, this line doesn't create the blank interstitial if the request isn't
cross origin (which is the case when  "Proceed" is clicked on an SSL
interstitial):
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[nasko, 2014-07-17 14:08:44]

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt.cc:515:
(parent_contents->GetInterstitialPage() ||
Is it correct that we hit this only after proceeding through the first
interstitial?

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt.cc:526: // replace existing interstitial,
if any.
I don't recall the ownership semantics of this. Would we leak the old
interstitial if we replace it with this one?

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1269: EXPECT_EQ("127.0.0.1",
contents->GetURL().host());
GetVisibleURL

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1283: EXPECT_EQ("127.0.0.1",
contents->GetURL().host());
GetVisibleURL

[meacer, 2014-07-17 22:08:18]

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt.cc:515:
(parent_contents->GetInterstitialPage() ||
On 2014/07/17 14:08:44, nasko (in Munich) wrote:
> Is it correct that we hit this only after proceeding through the first
> interstitial?

I can't think of any other state where an interstitial would already be shown.

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt.cc:526: // replace existing interstitial,
if any.
On 2014/07/17 14:08:44, nasko (in Munich) wrote:
> I don't recall the ownership semantics of this. Would we leak the old
> interstitial if we replace it with this one?

Interstitials clean up themselves on navigation. Since the login interstitial
creates a new navigation, the old one is cleaned up.

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1269: EXPECT_EQ("127.0.0.1",
contents->GetURL().host());
On 2014/07/17 14:08:44, nasko (in Munich) wrote:
> GetVisibleURL

Done.

https://codereview.chromium.org/397453002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1283: EXPECT_EQ("127.0.0.1",
contents->GetURL().host());
On 2014/07/17 14:08:44, nasko (in Munich) wrote:
> GetVisibleURL

Done.

[nasko, 2014-07-18 13:25:36]

lgtm

[meacer, 2014-07-18 17:10:23]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2014-07-18 17:11:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/meacer@chromium.org/397453002/40001

[meacer, 2014-07-18 19:14:28]

sky: Could you please take a look as OWNERS?

[sky, 2014-07-18 19:38:04]

I'm not familiar with this code. Can you find another owner that knows more
about it?

[commit-bot: I haz the power, 2014-07-18 19:55:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-18 19:58:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 19:58:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[meacer, 2014-07-18 20:03:29]

sky: Sure, though I added you since you reviewed crrev.com/276123002 earlier :)

pkasting: I think you reviewed a CL for login_prompt.cc earlier. Could you
please take a look?

[meacer, 2014-07-25 18:51:02]

On 2014/07/18 20:03:29, Mustafa Emre Acer wrote:
> sky: Sure, though I added you since you reviewed crrev.com/276123002 earlier
:)
> 
> pkasting: I think you reviewed a CL for login_prompt.cc earlier. Could you
> please take a look?

pkasting: Ping?

[meacer, 2014-07-30 21:47:50]

pkasting: Another ping?

[meacer, 2014-08-01 16:39:11]

-pkasting
+msw: Can you please take a look as owner? Thanks.

[msw, 2014-08-01 17:09:48]

https://codereview.chromium.org/397453002/diff/40001/chrome/browser/ui/login/...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/397453002/diff/40001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt.cc:513: // origin request or if there is an
existing interstitial.
It sounds like this could clobber or hide the first (potentially important)
interstitial, but the test seems to indicate that it'll show the interstitials
in sequence. Can you clarify this comment and potentially merge it with the
other two comments in the first block?

Can you also expand the CL description and have a second reviewer familiar with
interstitials take a look, since the owners here aren't terribly familiar with
the code and causes for concern? +CC creis, since he reviewed r273358.

Review from someone on the security team would be great too. +CC jschuh, in case
he can take a look or ping someone who ought to take a look.

Bonus points if you post a screencast of the repro steps with and without this
patch on the bug :)

[meacer, 2014-08-01 17:25:56]

https://codereview.chromium.org/397453002/diff/40001/chrome/browser/ui/login/...
File chrome/browser/ui/login/login_prompt.cc (right):

https://codereview.chromium.org/397453002/diff/40001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt.cc:513: // origin request or if there is an
existing interstitial.
On 2014/08/01 17:09:47, msw wrote:
> It sounds like this could clobber or hide the first (potentially important)
> interstitial, but the test seems to indicate that it'll show the interstitials
> in sequence. Can you clarify this comment and potentially merge it with the
> other two comments in the first block?

Ah, good point. The assumption at the time of writing the CL was that we could
only reach this point when the user proceeds through the first interstitial, and
that it wouldn't be possible to show a login prompt on top of an existing
interstitial. But after recent discussions, it's clear that when a malware
interstitial is displayed, the page still keeps running in the background, so it
might be possible to show an auth prompt on top of the existing interstitial.
That I believe can hide the malware interstitial. I'm going to test that.

> 
> Can you also expand the CL description and have a second reviewer familiar
with
> interstitials take a look, since the owners here aren't terribly familiar with
> the code and causes for concern? +CC creis, since he reviewed r273358.
> 
> Review from someone on the security team would be great too. +CC jschuh, in
case
> he can take a look or ping someone who ought to take a look.
> 
> Bonus points if you post a screencast of the repro steps with and without this
> patch on the bug :)

I'll give it a shot :)

[Peter Kasting, 2014-08-02 01:10:00]

FWIW, I'm sorry for never responding when you pinged me -- somehow I never saw
any of the pings.

If in the future you ever don't get a response when you try to ping me on a
codereview, please do send me email directly.

[meacer, 2014-08-02 01:12:31]

On 2014/08/02 01:10:00, Peter Kasting wrote:
> FWIW, I'm sorry for never responding when you pinged me -- somehow I never saw
> any of the pings.
> 
> If in the future you ever don't get a response when you try to ping me on a
> codereview, please do send me email directly.

No worries, will do so next time. Thanks!

[meacer, 2014-08-18 20:19:55]

Good news: https://codereview.chromium.org/410373003/ fixed this bug too.

msw: Can you please take a look at the browsertest?

[msw, 2014-08-19 01:16:39]

You should update the CL description and have this latest patch set reviewed by
someone familiar with interstitials and their testing patterns, but I don't see
anything objectionable offhand besides a couple nits.

https://codereview.chromium.org/397453002/diff/60001/chrome/browser/ui/login/...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

https://codereview.chromium.org/397453002/diff/60001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1256:
net::SpawnedTestServer::TYPE_HTTPS,
nit: indent four spaces for wrapped function arguments.

https://codereview.chromium.org/397453002/diff/60001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1260: );
nit: move this to the line above.

[meacer, 2014-08-20 20:13:45]

PTAL.

nasko: Can you please take another look at the test case? It didn't change much,
but just in case. Thanks.

https://codereview.chromium.org/397453002/diff/60001/chrome/browser/ui/login/...
File chrome/browser/ui/login/login_prompt_browsertest.cc (right):

https://codereview.chromium.org/397453002/diff/60001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1256:
net::SpawnedTestServer::TYPE_HTTPS,
On 2014/08/19 01:16:39, msw wrote:
> nit: indent four spaces for wrapped function arguments.

Done.

https://codereview.chromium.org/397453002/diff/60001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1260: );
On 2014/08/19 01:16:39, msw wrote:
> nit: move this to the line above.

Done.

[nasko, 2014-08-20 23:22:22]

Still LGTM

[meacer, 2014-08-21 03:10:24]

Thanks nasko.

Can I please get an lgtm from one of the owners? :)

[msw, 2014-08-21 03:17:39]

lgtm

[meacer, 2014-08-21 03:26:50]

Thanks!

[meacer, 2014-08-21 03:26:55]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2014-08-21 03:27:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/meacer@chromium.org/397453002/80001

[commit-bot: I haz the power, 2014-08-21 04:54:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-21 05:22:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-21 05:22:38]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[meacer, 2014-08-21 17:54:03]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2014-08-21 17:56:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/meacer@chromium.org/397453002/80001

[commit-bot: I haz the power, 2014-08-21 18:30:42]

Committed patchset #5 (80001) as 291137