Destroy SSLErrorHandler on new navigations so that it can properly be recreated.

Destroy SSLErrorHandler on new navigations so that it can properly be recreated.

Since |SSLErrorHandler| was a |WebContentsUserData| tied to the lifetime of a
|WebContents|, it wasn't properly deleted when the |WebContents| was reloaded
or navigated to another page. This CL removes the error handler explicitly and
adds browser tests to assert reload/navigate behavior.

BUG=453875
Committed: https://crrev.com/d2c91b4a8ba7e7b715a70f4deaafee3b468349c0
Cr-Commit-Position: refs/heads/master@{#321603}

[meacer, 2015-03-17 21:04:27]

meacer@chromium.org changed reviewers:
+ mmenke@chromium.org

[meacer, 2015-03-17 21:04:27]

mmenke: Can you PTAL? Thanks.

[mmenke, 2015-03-20 13:33:50]

LGTM

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:820: // Waits until
the interstitial delay timer in SSLErrorHandler is fired.
While you're here, can you replace all these "fired" with "started".  Had to
trace my way back through the code to figure out that this didn't mean what it
sounded like.

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:2220: // instantiate
an |SSLErrorHandler| and end up showing an SSL.
"showing an SSL interstitial"?

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:2234:
EXPECT_TRUE(broken_tab_contents->ShowingInterstitialPage());
ASSERT_TRUE?

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:2288:
EXPECT_TRUE(broken_tab_contents->ShowingInterstitialPage());
ASSERT_TRUE?

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:238: callback_.Reset();
base::ResetAndReturn(callback_).Run(false)?  That allows the callback to set a
new callback.  Not currently needed, but seems best to be safe.

[mmenke, 2015-03-20 13:34:08]

And sorry for the delay.  Been sick.  :(

[meacer, 2015-03-20 17:46:02]

Thanks, hope you are feeling better!

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:820: // Waits until
the interstitial delay timer in SSLErrorHandler is fired.
On 2015/03/20 13:33:50, mmenke wrote:
> While you're here, can you replace all these "fired" with "started".  Had to
> trace my way back through the code to figure out that this didn't mean what it
> sounded like.

Done.

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:2220: // instantiate
an |SSLErrorHandler| and end up showing an SSL.
On 2015/03/20 13:33:50, mmenke wrote:
> "showing an SSL interstitial"?

Done.

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:2234:
EXPECT_TRUE(broken_tab_contents->ShowingInterstitialPage());
On 2015/03/20 13:33:50, mmenke wrote:
> ASSERT_TRUE?

Done.

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:2288:
EXPECT_TRUE(broken_tab_contents->ShowingInterstitialPage());
On 2015/03/20 13:33:50, mmenke wrote:
> ASSERT_TRUE?

Done.

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/1004283004/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.cc:238: callback_.Reset();
On 2015/03/20 13:33:50, mmenke wrote:
> base::ResetAndReturn(callback_).Run(false)?  That allows the callback to set a
> new callback.  Not currently needed, but seems best to be safe.

Done.

[meacer, 2015-03-20 17:46:22]

The CQ bit was checked by meacer@chromium.org

[meacer, 2015-03-20 17:46:22]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1004283004/#ps40001
(title: "mmenke comments")

[commit-bot: I haz the power, 2015-03-20 17:46:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1004283004/40001

[commit-bot: I haz the power, 2015-03-20 18:34:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-20 18:35:46]

Patchset 3 (id:??) landed as
https://crrev.com/d2c91b4a8ba7e7b715a70f4deaafee3b468349c0
Cr-Commit-Position: refs/heads/master@{#321603}