Skip to content

Commit

Permalink
Remove dead logic for handling HTTP auth empty responses
Browse files Browse the repository at this point in the history
LoginTabHelper had some complex logic for handling HTTP 401/407
responses that serve empty response bodies (which needed special
handling because they show up as double-commits from the browser
process's perspective). This double-commit issue no longer happens
because we now commit a blank page underneath the auth prompt, whereas
previously we were committing whatever the server sent.

Bug: 963307
Change-Id: Ie0e83f1c52bc2d6ed17f8eb3506f87ad3a1700c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1891761
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712017}
  • Loading branch information
estark37 authored and Commit Bot committed Nov 3, 2019
1 parent 5d756ff commit bad4986
Showing 1 changed file with 0 additions and 39 deletions.
39 changes: 0 additions & 39 deletions chrome/browser/ui/login/login_tab_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,39 +37,6 @@ void LoginTabHelper::DidStartNavigation(
if (!delegate_)
return;

// TODO(https://crbug.com/943610): this is a very hacky special-case for a
// particular issue that arises when the server sends an empty body for the
// 401 or 407 response. In this case, the renderer commits an error page for
// the HTTP status code (see
// ChromeContentRendererClient::PrepareErrorPageForHttpStatusError). Error
// pages are a second commit from the browser's perspective, which runs
// DidStartNavigation again and therefore would dismiss the prompt if this
// special case weren't here. To make matters worse, at the error page's
// second commit, the browser process does not have a NavigationRequest
// available (see |is_commit_allowed_to_proceed| in
// RenderFrameHostImpl::DidCommitNavigationInternal), and therefore no
// AuthChallengeInfo to use to re-show the prompt after it has been
// dismissed. This whole mess should be fixed in https://crbug.com/943610,
// which is about enforcing that the browser always has a NavigationRequest
// available at commit time; once error pages no longer have second commits
// for which NavigationRequests are manufactured, this special case will no
// longer be needed.
//
// For now, we can hack around it by preserving the login prompt when starting
// a navigation to the same URL for which we are currently showing a login
// prompt. There is a possibility that the starting navigation is actually due
// to the user refreshing rather than the error page committing, and when the
// user refreshes the server might no longer serve an auth challenge. But we
// can't distinguish the two scenarios (error page committing vs user
// refreshing) at this point, so we clear the prompt in DidFinishNavigation if
// it's not an error page. This behavior could lead to a slight oddness where
// the prompt lingers around for a bit too long, but this should only happen
// in the perfect storm where a server's auth response has an empty body,
// the user refreshes when the prompt is showing, and the server no longer
// requires auth on the refresh.
if (navigation_handle->GetURL() == url_for_delegate_)
return;

delegate_.reset();
url_for_delegate_ = GURL();
}
Expand All @@ -84,12 +51,6 @@ void LoginTabHelper::DidFinishNavigation(
return;
}

// See TODO(https://crbug.com/943610) in DidStartNavigation().
if (delegate_ && !navigation_handle->IsErrorPage()) {
delegate_.reset();
url_for_delegate_ = GURL();
}

// LoginTabHelper stores the navigation entry ID and navigation handle ID
// corresponding to the refresh that occurs when a user cancels a prompt. (The
// refresh is to retrieve the error page body from the server so that it can
Expand Down

0 comments on commit bad4986

Please sign in to comment.