Set SSL info when an HTTP auth dialog is triggered by direct navigation.

Set SSL info when an HTTP auth dialog is triggered by direct navigation.

This patch correctly sets the SSL information of a navigation entry when
an HTTP auth dialog is triggered. It only sets the information if the navigation
is initiated by the user (i.e. through the omnibox). It does not set SSL
information if the navigation ending up with the HTTP auth dialog was triggered
by a page redirect, as there is no SiteInstance to get the SSL information from
in that case.

BUG=395050

[meacer, 2014-07-22 21:42:53]

Charlie/Nasko, can you please take a look? Thanks.

[Charlie Reis, 2014-07-22 22:01:21]

I'm not a great reviewer for the SSL state/UI calls, but I agree that we should
show the correct SSL icon / page info for the destination URL while the auth
dialog is visible.  I wonder if Nasko or someone else might be able to review
the calls more carefully?

There was one comment about mixed content that caught my eye and concerned me.

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

https://codereview.chromium.org/403933002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_interstitial_delegate.cc:58: //
LoginHandler::UpdateSSLState doesn't set the mixed content state, but the
This concerns me.  Are you saying that there could be a mixed content warning
that is missed while the login prompt is showing?  That sounds bad.

You could imagine a case where something like window.opener is on the same HTTPS
origin and has mixed content.  Once the user logs into what looked like a safe
prompt, the mixed content opener reaches in and has access to the
now-authenticated page.

[meacer, 2014-07-22 22:07:10]

Thanks Charlie. Adding Chris for thoughts on SSL related bits.

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

https://codereview.chromium.org/403933002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_interstitial_delegate.cc:17: #include
"net/ssl/ssl_info.h"
Some of these will be removed.

https://codereview.chromium.org/403933002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_interstitial_delegate.cc:58: //
LoginHandler::UpdateSSLState doesn't set the mixed content state, but the
On 2014/07/22 22:01:21, Charlie Reis wrote:
> This concerns me.  Are you saying that there could be a mixed content warning
> that is missed while the login prompt is showing?  That sounds bad.
> 
> You could imagine a case where something like window.opener is on the same
HTTPS
> origin and has mixed content.  Once the user logs into what looked like a safe
> prompt, the mixed content opener reaches in and has access to the
> now-authenticated page.

Ah, I didn't consider the "mixed content on the same origin" case. I was
assuming that the check was per page rather than per origin, but it makes sense
for it to be per origin.

The main problem here is that I can't seem to use SSLManager from UI code. If I
can, then I won't need to duplicate the logic. Not sure if it's possible though.

[Charlie Reis, 2014-07-22 23:23:18]

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

https://codereview.chromium.org/403933002/diff/20001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_interstitial_delegate.cc:58: //
LoginHandler::UpdateSSLState doesn't set the mixed content state, but the
On 2014/07/22 22:07:09, Mustafa Emre Acer wrote:
> On 2014/07/22 22:01:21, Charlie Reis wrote:
> > This concerns me.  Are you saying that there could be a mixed content
warning
> > that is missed while the login prompt is showing?  That sounds bad.
> > 
> > You could imagine a case where something like window.opener is on the same
> HTTPS
> > origin and has mixed content.  Once the user logs into what looked like a
safe
> > prompt, the mixed content opener reaches in and has access to the
> > now-authenticated page.
> 
> Ah, I didn't consider the "mixed content on the same origin" case. I was
> assuming that the check was per page rather than per origin, but it makes
sense
> for it to be per origin.
> 
> The main problem here is that I can't seem to use SSLManager from UI code. If
I
> can, then I won't need to duplicate the logic. Not sure if it's possible
though.

Yeah, it would be good to find a way to expose the info that you need, because
duplicating it outside of content/ will certainly lead to unfixed bugs.  Not
sure what content/public surface is the right place for it, so adding John to
see if he has ideas.

[meacer, 2014-07-22 23:27:55]

On 2014/07/22 23:23:18, Charlie Reis wrote:
>
https://codereview.chromium.org/403933002/diff/20001/chrome/browser/ui/login/...
> File chrome/browser/ui/login/login_interstitial_delegate.cc (right):
> 
>
https://codereview.chromium.org/403933002/diff/20001/chrome/browser/ui/login/...
> chrome/browser/ui/login/login_interstitial_delegate.cc:58: //
> LoginHandler::UpdateSSLState doesn't set the mixed content state, but the
> On 2014/07/22 22:07:09, Mustafa Emre Acer wrote:
> > On 2014/07/22 22:01:21, Charlie Reis wrote:
> > > This concerns me.  Are you saying that there could be a mixed content
> warning
> > > that is missed while the login prompt is showing?  That sounds bad.
> > > 
> > > You could imagine a case where something like window.opener is on the same
> > HTTPS
> > > origin and has mixed content.  Once the user logs into what looked like a
> safe
> > > prompt, the mixed content opener reaches in and has access to the
> > > now-authenticated page.
> > 
> > Ah, I didn't consider the "mixed content on the same origin" case. I was
> > assuming that the check was per page rather than per origin, but it makes
> sense
> > for it to be per origin.
> > 
> > The main problem here is that I can't seem to use SSLManager from UI code.
If
> I
> > can, then I won't need to duplicate the logic. Not sure if it's possible
> though.
> 
> Yeah, it would be good to find a way to expose the info that you need, because
> duplicating it outside of content/ will certainly lead to unfixed bugs.  Not
> sure what content/public surface is the right place for it, so adding John to
> see if he has ideas.

There is also the chance that I can call into SSLManager from ResourceLoader,
before the auth dialog is called. I haven't looked in depth yet.

[jam, 2014-07-23 17:19:13]

i'm overloaded with reviews and CY, so please pick another owner from chrome

[meacer, 2014-07-24 00:05:02]

Thanks jam, I'll remove you from the reviewers.

Charlie/Nasko/Chris: The latest patchset updates the SSL info in resource
loader. PTAL.

[nasko, 2014-07-24 09:32:34]

This iteration seems much more intuitive and cleaner. Some comments posted.

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

https://codereview.chromium.org/403933002/diff/40001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1334: EXPECT_EQ("www.a.com",
contents->GetURL().host());
nit: GetVisibleURL

Also, at this point we either have LastCommitted nav entry which is different
than the Visible one or we don't have LastCommitted. It might be useful check to
add, but I'd leave the decision up to you.

https://codereview.chromium.org/403933002/diff/40001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1338: EXPECT_EQ("www.a.com",
contents->GetURL().host());
nit: GetVisibleURL

Here we should have the LastCommitted match the Visible, right?

https://codereview.chromium.org/403933002/diff/40001/content/browser/loader/r...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/403933002/diff/40001/content/browser/loader/r...
content/browser/loader/resource_loader.cc:296: // We should not have any SSL
state.
nit: indent 2 more spaces

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:96:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
This DCHECK seems useless because of the if statement above.

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:100:
static_cast<WebContentsImpl*>(WebContents::FromRenderFrameHost(host));
Why do you need to cast this? GetController() is available on the public
content/ API.

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:104: NavigationControllerImpl* controller =
&web_contents->GetController();
Ah, this is the place where a cast will be needed.

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:108: scoped_ptr<SSLManager> ssl_manager(new
SSLManager(controller));
Why do we need to construct a SSLManager? The nav controller should already have
one of those and it should be registered, so looking it up and calling
UpdateEntry on it seems to make sense.

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:156: void SSLManager::UpdateEntry(const
std::string& serialized_security_info,
nit: all params should be on new lines

[meacer, 2014-07-24 17:32:33]

Thanks Nasko.

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

https://codereview.chromium.org/403933002/diff/40001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1334: EXPECT_EQ("www.a.com",
contents->GetURL().host());
On 2014/07/24 09:32:34, nasko (in Munich) wrote:
> nit: GetVisibleURL
> 
> Also, at this point we either have LastCommitted nav entry which is different
> than the Visible one or we don't have LastCommitted. It might be useful check
to
> add, but I'd leave the decision up to you.

Done.

https://codereview.chromium.org/403933002/diff/40001/chrome/browser/ui/login/...
chrome/browser/ui/login/login_prompt_browsertest.cc:1338: EXPECT_EQ("www.a.com",
contents->GetURL().host());
On 2014/07/24 09:32:33, nasko (in Munich) wrote:
> nit: GetVisibleURL
> 
> Here we should have the LastCommitted match the Visible, right?

Done.

https://codereview.chromium.org/403933002/diff/40001/content/browser/loader/r...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/403933002/diff/40001/content/browser/loader/r...
content/browser/loader/resource_loader.cc:296: // We should not have any SSL
state.
On 2014/07/24 09:32:34, nasko (in Munich) wrote:
> nit: indent 2 more spaces

Done.

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:96:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/07/24 09:32:34, nasko (in Munich) wrote:
> This DCHECK seems useless because of the if statement above.

Done.

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:100:
static_cast<WebContentsImpl*>(WebContents::FromRenderFrameHost(host));
On 2014/07/24 09:32:34, nasko (in Munich) wrote:
> Why do you need to cast this? GetController() is available on the public
> content/ API.

I agree it's confusing. It's now casting NavigationController rather than
WebContents.

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:104: NavigationControllerImpl* controller =
&web_contents->GetController();
On 2014/07/24 09:32:34, nasko (in Munich) wrote:
> Ah, this is the place where a cast will be needed.

Acknowledged.

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:108: scoped_ptr<SSLManager> ssl_manager(new
SSLManager(controller));
On 2014/07/24 09:32:34, nasko (in Munich) wrote:
> Why do we need to construct a SSLManager? The nav controller should already
have
> one of those and it should be registered, so looking it up and calling
> UpdateEntry on it seems to make sense.

Done.

https://codereview.chromium.org/403933002/diff/40001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:156: void SSLManager::UpdateEntry(const
std::string& serialized_security_info,
On 2014/07/24 09:32:34, nasko (in Munich) wrote:
> nit: all params should be on new lines

Done.

[Charlie Reis, 2014-07-24 18:39:33]

Cool, this seems to work well.  Few nits below.

https://codereview.chromium.org/403933002/diff/60001/content/browser/loader/r...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/403933002/diff/60001/content/browser/loader/r...
content/browser/loader/resource_loader.cc:290: NOTREACHED();
NOTREACHED is debug only.  We'll need either a return or a CHECK(0) for release
builds, or else we'll have uninitialized reads below.

https://codereview.chromium.org/403933002/diff/60001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/403933002/diff/60001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:96: RenderFrameHost* host =
nit: render_frame_host (once we've renamed the id to render_frame_id)

https://codereview.chromium.org/403933002/diff/60001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_manager.h (right):

https://codereview.chromium.org/403933002/diff/60001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.h:63: int render_frame_host,
nit: render_frame_id
(render_frame_host would usually refer to the object itself.)

[meacer, 2014-07-24 18:48:32]

Thanks Charlie.

https://codereview.chromium.org/403933002/diff/60001/content/browser/loader/r...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/403933002/diff/60001/content/browser/loader/r...
content/browser/loader/resource_loader.cc:290: NOTREACHED();
On 2014/07/24 18:39:32, Charlie Reis wrote:
> NOTREACHED is debug only.  We'll need either a return or a CHECK(0) for
release
> builds, or else we'll have uninitialized reads below.

Done.

https://codereview.chromium.org/403933002/diff/60001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/403933002/diff/60001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.cc:96: RenderFrameHost* host =
On 2014/07/24 18:39:32, Charlie Reis wrote:
> nit: render_frame_host (once we've renamed the id to render_frame_id)

Done.

https://codereview.chromium.org/403933002/diff/60001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_manager.h (right):

https://codereview.chromium.org/403933002/diff/60001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_manager.h:63: int render_frame_host,
On 2014/07/24 18:39:32, Charlie Reis wrote:
> nit: render_frame_id
> (render_frame_host would usually refer to the object itself.)

Done.

[Charlie Reis, 2014-07-24 18:50:52]

content/ LGTM

[palmer, 2014-07-24 19:55:37]

LGTM. Nice!

I am using ssh -X to display ToT + this patch running on my machine at the
office on my laptop here at home, so that could be the problem, but: when I
bring up the Page Info Bubble and switch to the Connection tab, I can't get the
certificate view to come up when I click the "Certificate information" link.

[palmer, 2014-07-24 19:56:43]

> I am using ssh -X to display ToT + this patch running on my machine at the
> office on my laptop here at home, so that could be the problem, but: when I
> bring up the Page Info Bubble and switch to the Connection tab, I can't get
the
> certificate view to come up when I click the "Certificate information" link.

Ahh. When I closed the HTTP Auth dialog, the Certificate Viewer dialog finally
showed. So I take it we can't have 2 dialogs up at once. That sounds like a
separate bug?

[palmer, 2014-07-24 19:58:28]

> Ahh. When I closed the HTTP Auth dialog, the Certificate Viewer dialog finally
> showed. So I take it we can't have 2 dialogs up at once. That sounds like a
> separate bug?

In fact, every time I close the Certificate Viewer dialog, another one pops up,
because I tried like 10 times to bring up the CV dialog. :) So the clicks
registered, and were queued in some kind of dialogs-to-display queue. :)

[meacer, 2014-07-25 01:19:47]

On 2014/07/24 19:58:28, Chromium Palmer wrote:
> > Ahh. When I closed the HTTP Auth dialog, the Certificate Viewer dialog
finally
> > showed. So I take it we can't have 2 dialogs up at once. That sounds like a
> > separate bug?
> 
> In fact, every time I close the Certificate Viewer dialog, another one pops
up,
> because I tried like 10 times to bring up the CV dialog. :) So the clicks
> registered, and were queued in some kind of dialogs-to-display queue. :)

The UI indeed seems broken :/ It looks like the both the HTTP auth and CV
dialogs are modal, so CV dialog isn't showing until the other one closes. I
tried a couple of things like changing the modal style but it didn't work. We'll
probably have to file a separate bug for that.

There is also a problem with this patch: When a.com redirects to
https://b.com/auth and ResourceLoader::OnAuthDialog gets called, there won't yet
be any navigation entry (pending, last committed or visible) for b.com. So
SSLManager::OnAuthRequired will try to update the wrong navigation entry (the
one for a.com), and the HTTPS status for https://b.com won't be reflected in the
omnibox when the auth dialog shows.

Charlie, Nasko: For that case, I'm planning to create an empty navigation entry
and pass it to SSLManager::Update, and then read back the calculated security
style. But I think that means SiteInstance will have to be empty for the
navigation entry, and mixed content check won't be done. Is that correct?

[Charlie Reis, 2014-07-25 20:32:42]

On 2014/07/25 01:19:47, Mustafa Emre Acer wrote:
> On 2014/07/24 19:58:28, Chromium Palmer wrote:
> > > Ahh. When I closed the HTTP Auth dialog, the Certificate Viewer dialog
> finally
> > > showed. So I take it we can't have 2 dialogs up at once. That sounds like
a
> > > separate bug?
> > 
> > In fact, every time I close the Certificate Viewer dialog, another one pops
> up,
> > because I tried like 10 times to bring up the CV dialog. :) So the clicks
> > registered, and were queued in some kind of dialogs-to-display queue. :)
> 
> The UI indeed seems broken :/ It looks like the both the HTTP auth and CV
> dialogs are modal, so CV dialog isn't showing until the other one closes. I
> tried a couple of things like changing the modal style but it didn't work.
We'll
> probably have to file a separate bug for that.
> 
> There is also a problem with this patch: When a.com redirects to
> https://b.com/auth and ResourceLoader::OnAuthDialog gets called, there won't
yet
> be any navigation entry (pending, last committed or visible) for b.com. So
> SSLManager::OnAuthRequired will try to update the wrong navigation entry (the
> one for a.com), and the HTTPS status for https://b.com won't be reflected in
the
> omnibox when the auth dialog shows.

OnAuthDialog gets called before the visible NavigationEntry is updated?  That's
unfortunate.  Perhaps it's not the right place to do this?

> Charlie, Nasko: For that case, I'm planning to create an empty navigation
entry
> and pass it to SSLManager::Update, and then read back the calculated security
> style. But I think that means SiteInstance will have to be empty for the
> navigation entry, and mixed content check won't be done. Is that correct?

Creating a NavigationEntry from scratch here sounds problematic, since I thought
we wanted to update the real NavigationEntry which is being shown in the
omnibox.  I admit that I'm not fully following how this works, though.

[palmer, 2014-10-16 17:27:08]

Ping! Any progress on this old CL?

[meacer, 2014-10-16 17:31:26]

On 2014/10/16 17:27:08, Chromium Palmer wrote:
> Ping! Any progress on this old CL?

Unfortunately no :/ This fix is incomplete, in that it doesn't cover one half of
the problem. I tried to fix the other half but it turned out to be more
difficult than I thought (there was no SiteInstance to use with the SSL info by
the time the page did a redirect to an HTTP auth site). It's still on my radar
though.

[meacer, 2015-03-09 17:53:50]

Patchset #7 (id:120001) has been deleted

[meacer, 2015-03-12 00:37:20]

Patchset #7 (id:140001) has been deleted

[meacer, 2015-03-12 00:38:06]

Patchset #6 (id:100001) has been deleted

[meacer, 2015-03-12 20:37:48]

Back to this after 7 months :)

I finally fixed the issue with renderer-initiated navigations not properly
showing SSL status with HTTP auth.

Can you please take another look? Thanks!

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

https://codereview.chromium.org/403933002/diff/160001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:571: : pending_entry;
Not sure if this is the best way to check for renderer-initiated navigations.

[Charlie Reis, 2015-03-13 18:19:37]

Sorry for the delay-- things are crazy this week.  I've only looked at the place
you commented so far, but I left some questions there.

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

https://codereview.chromium.org/403933002/diff/160001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:571: : pending_entry;
On 2015/03/12 20:37:48, Mustafa Emre Acer wrote:
> Not sure if this is the best way to check for renderer-initiated navigations.

This won't work for determining renderer-initiated.  There's a
NavigationEntryImpl::is_renderer_initiated() for that, but we haven't had to
expose it outside content/ yet.

The new code here also feels fragile to me, since the conditions in which
GetVisibleEntry() and GetPendingEntry() differ are very subtle.  I haven't
re-read the whole CL, but was there a reason we couldn't just use the pending
entry in all cases (if that's what has the actual URL and SSL state for the auth
request)?

[meacer, 2015-03-17 17:30:44]

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

https://codereview.chromium.org/403933002/diff/160001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:571: : pending_entry;
On 2015/03/13 18:19:37, Charlie Reis wrote:
> On 2015/03/12 20:37:48, Mustafa Emre Acer wrote:
> > Not sure if this is the best way to check for renderer-initiated
navigations.
> 
> This won't work for determining renderer-initiated.  There's a
> NavigationEntryImpl::is_renderer_initiated() for that, but we haven't had to
> expose it outside content/ yet.

I initally used is_renderer_initiated() but realized the layering violation at
the last minute so resorted to these hacks.

> 
> The new code here also feels fragile to me, since the conditions in which
> GetVisibleEntry() and GetPendingEntry() differ are very subtle.  I haven't
> re-read the whole CL, but was there a reason we couldn't just use the pending
> entry in all cases (if that's what has the actual URL and SSL state for the
auth
> request)?

Interesting, using the pending entry indeed works! I'm almost sure it didn't
work back then, but it seems like a lot has changed (and in a good way)

[Charlie Reis, 2015-03-18 17:46:38]

Hmm, there's still some landmines here.  It may make sense to discuss this in
person at some point?

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

https://codereview.chromium.org/403933002/diff/180001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:562: content::NavigationEntry*
pending_entry =
Let's put a comment about why it's correct to use the pending entry here, since
it took us a while to realize it.  And just to sanity check, let's make sure
it's safe:

1) Browser initiated navigations will always start with a pending entry, but it
may get discarded or replaced before the navigation commits.  When does this
call happen?  (It may be possible for the pending entry to be gone by now, which
might have been why we couldn't use it.)

2) Renderer initiated navigations in the main frame will have a pending entry if
they make a network request, but it can get discarded as above.

3) Renderer initiated navigations in subframes will not have pending entries. 
Can they cause an auth dialog?

https://codereview.chromium.org/403933002/diff/180001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/403933002/diff/180001/content/browser/loader/...
content/browser/loader/resource_loader.cc:333: if
(!info->GetAssociatedRenderFrame(&render_process_id, &render_frame_id))
Random question: Why does this method return a boolean?  Looks like there's only
one implementation of it and it can't return false.

(No need to fix in this CL, but that might be a useful cleanup.)

https://codereview.chromium.org/403933002/diff/180001/content/browser/loader/...
content/browser/loader/resource_loader.cc:334: CHECK(0);
nit: CHECK(false) is more common.

https://codereview.chromium.org/403933002/diff/180001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/403933002/diff/180001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:87: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
Please have a separate method for the UI thread version (e.g. OnAuthDialogOnUI).
 The threading model gets confusing when we have methods that can be called from
multiple threads.

https://codereview.chromium.org/403933002/diff/180001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:102: NavigationEntryImpl* entry =
controller->GetPendingEntry();
If you've just done a PostTask to get here, then there's no guarantee that the
pending entry still exists or is for the same navigation.  (It may have been
discarded or replaced with a different one in the meantime.)

[palmer, 2015-09-29 00:03:33]

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

https://codereview.chromium.org/403933002/diff/180001/chrome/browser/ui/login...
chrome/browser/ui/login/login_prompt.cc:562: content::NavigationEntry*
pending_entry =
I'm following up with this at https://codereview.chromium.org/1368863002.

https://codereview.chromium.org/403933002/diff/180001/content/browser/loader/...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/403933002/diff/180001/content/browser/loader/...
content/browser/loader/resource_loader.cc:334: CHECK(0);
> nit: CHECK(false) is more common.

Done in https://codereview.chromium.org/1368863002.

https://codereview.chromium.org/403933002/diff/180001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_manager.cc (right):

https://codereview.chromium.org/403933002/diff/180001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:87: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2015/03/18 17:46:38, Charlie Reis wrote:
> Please have a separate method for the UI thread version (e.g.
OnAuthDialogOnUI).
>  The threading model gets confusing when we have methods that can be called
from
> multiple threads.

The sole call site is on the IO thread. Making the change you request and adding
DCHECK_CURRENTLY_ON(BrowserThread::IO); at
https://codereview.chromium.org/1368863002.

https://codereview.chromium.org/403933002/diff/180001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:102: NavigationEntryImpl* entry =
controller->GetPendingEntry();
On 2015/03/18 17:46:38, Charlie Reis wrote:
> If you've just done a PostTask to get here, then there's no guarantee that the
> pending entry still exists or is for the same navigation.  (It may have been
> discarded or replaced with a different one in the meantime.)

Added a comment on the other CL.

[palmer, 2016-04-04 20:18:48]

Can we close this old CL? I'm trying to clean out my to-review list. Thanks!

[meacer, 2016-04-04 21:10:40]

On 2016/04/04 20:18:48, palmer wrote:
> Can we close this old CL? I'm trying to clean out my to-review list. Thanks!

I'll close this. palmer: Note that you took over this CL as
https://codereview.chromium.org/1368863002/