Make SSLErrorHandler UI-thread-only and not-refcounted

Make SSLErrorHandler UI-thread-only and not-refcounted

This CL makes the SSLErrorHandler class UI-thread-only
(instead of shared between UI and IO as it was previously) and not
refcounted. When a decision about an error is made synchronously, the
SSLErrorHandler is owned by SSLPolicy. When SSLPolicy calls out to the
embedder to make an asynchronous decision about what to do with the
error, the callback passed to the embedder takes ownership of the
SSLErrorHandler.

This CL is based on top of two cleanup CLs:
1. https://codereview.chromium.org/2220603003: Remove unnecessary
|result| argument from AllowCertificateError()
2. https://codereview.chromium.org/2222003002: Combine SSLCertErrorHandler
and SSLErrorHandler

BUG=558491
Committed: https://crrev.com/dc874b5d1b42f7747fa7b75b8cc2f322f877d11a
Cr-Commit-Position: refs/heads/master@{#410682}

[estark, 2016-08-06 00:30:22]

Patchset #1 (id:1) has been deleted

[estark, 2016-08-08 17:38:31]

Description was changed from

==========
Combine SSLCertErrorHandler and SSLErrorHandler and make UI-thread-only

This CL combines SSLErrorHandler and SSLCertErrorHandler into one class,
since SSLCertErrorHandler is the only subclass of SSLErrorHandler and
there doesn't seem to be a reason for the split.

This CL also makes the resulting SSLErrorHandler class UI-thread-only
(instead of shared between UI and IO as it was previously) and not
refcounted. When a decision about an error is made synchronously, the
SSLErrorHandler is owned by SSLPolicy. When SSLPolicy calls out to the
embedder to make an asynchronous decision about what to do with the
error, the callback passed to the embedder takes ownership of the
SSLErrorHandler.

This CL is based on top of https://codereview.chromium.org/2220603003/.

BUG=558491
==========

to

==========
Make SSLErrorHandler UI-thread-only and not-refcounted

This CL also makes the SSLErrorHandler class UI-thread-only
(instead of shared between UI and IO as it was previously) and not
refcounted. When a decision about an error is made synchronously, the
SSLErrorHandler is owned by SSLPolicy. When SSLPolicy calls out to the
embedder to make an asynchronous decision about what to do with the
error, the callback passed to the embedder takes ownership of the
SSLErrorHandler.

This CL is based on top of two cleanup CLs:
1. https://codereview.chromium.org/2220603003: Remove unnecessary
|result| argument from AllowCertificateError()
2. https://codereview.chromium.org/2222003002: Combine SSLCertErrorHandler
and SSLErrorHandler

BUG=558491
==========

[estark, 2016-08-08 17:38:39]

Description was changed from

==========
Make SSLErrorHandler UI-thread-only and not-refcounted

This CL also makes the SSLErrorHandler class UI-thread-only
(instead of shared between UI and IO as it was previously) and not
refcounted. When a decision about an error is made synchronously, the
SSLErrorHandler is owned by SSLPolicy. When SSLPolicy calls out to the
embedder to make an asynchronous decision about what to do with the
error, the callback passed to the embedder takes ownership of the
SSLErrorHandler.

This CL is based on top of two cleanup CLs:
1. https://codereview.chromium.org/2220603003: Remove unnecessary
|result| argument from AllowCertificateError()
2. https://codereview.chromium.org/2222003002: Combine SSLCertErrorHandler
and SSLErrorHandler

BUG=558491
==========

to

==========
Make SSLErrorHandler UI-thread-only and not-refcounted

This CL makes the SSLErrorHandler class UI-thread-only
(instead of shared between UI and IO as it was previously) and not
refcounted. When a decision about an error is made synchronously, the
SSLErrorHandler is owned by SSLPolicy. When SSLPolicy calls out to the
embedder to make an asynchronous decision about what to do with the
error, the callback passed to the embedder takes ownership of the
SSLErrorHandler.

This CL is based on top of two cleanup CLs:
1. https://codereview.chromium.org/2220603003: Remove unnecessary
|result| argument from AllowCertificateError()
2. https://codereview.chromium.org/2222003002: Combine SSLCertErrorHandler
and SSLErrorHandler

BUG=558491
==========

[estark, 2016-08-08 17:44:20]

estark@chromium.org changed reviewers:
+ nasko@chromium.org

[estark, 2016-08-08 17:44:20]

And this 3/3 of making SSLErrorHandler live on the UI thread only.

[nasko, 2016-08-08 20:57:06]

I like! LGTM with a few nits.

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_error_handler.h:98: // A net error code describing the
error that occurred.
nit: Empty lines before comments.

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

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:53: bool fatal) {
nit: Isn't fatal also const?

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:55: std::unique_ptr<SSLErrorHandler>
handler(new SSLErrorHandler(
nit: I'd construct the handler first, since it is done unconditionally and the
WebContents is later checked, so the assignment and check can follow one after
the other.

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_policy.cc (right):

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.cc:46: // active
nit: Let's reflow the comment since it is moving anyway. No need to waste the
rest of that empty line.

[estark, 2016-08-09 05:35:01]

Thanks Nasko!

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_error_handler.h:98: // A net error code describing the
error that occurred.
On 2016/08/08 20:57:06, nasko wrote:
> nit: Empty lines before comments.

Done.

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

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:53: bool fatal) {
On 2016/08/08 20:57:06, nasko wrote:
> nit: Isn't fatal also const?

n/a per your hangout :)

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:55: std::unique_ptr<SSLErrorHandler>
handler(new SSLErrorHandler(
On 2016/08/08 20:57:06, nasko wrote:
> nit: I'd construct the handler first, since it is done unconditionally and the
> WebContents is later checked, so the assignment and check can follow one after
> the other.

Ah, unfortunately no can do, since I'm passing it in the SSLErrorHandler
constructor.

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_policy.cc (right):

https://codereview.chromium.org/2213193005/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_policy.cc:46: // active
On 2016/08/08 20:57:06, nasko wrote:
> nit: Let's reflow the comment since it is moving anyway. No need to waste the
> rest of that empty line.

Done.

[estark, 2016-08-09 05:35:22]

The CQ bit was checked by estark@chromium.org

[estark, 2016-08-09 05:35:23]

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

[commit-bot: I haz the power, 2016-08-09 05:35:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 06:12:12]

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

[commit-bot: I haz the power, 2016-08-09 06:12:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2016-08-09 06:30:16]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 06:30:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 08:23:48]

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

[commit-bot: I haz the power, 2016-08-09 08:23:49]

Dry run: This issue passed the CQ dry run.

[estark, 2016-08-09 15:36:08]

The CQ bit was checked by estark@chromium.org

[estark, 2016-08-09 15:36:08]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2213193005/#ps100001
(title: "remove UI-thread dereference for DCHECK")

[commit-bot: I haz the power, 2016-08-09 15:36:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 15:43:19]

Description was changed from

==========
Make SSLErrorHandler UI-thread-only and not-refcounted

This CL makes the SSLErrorHandler class UI-thread-only
(instead of shared between UI and IO as it was previously) and not
refcounted. When a decision about an error is made synchronously, the
SSLErrorHandler is owned by SSLPolicy. When SSLPolicy calls out to the
embedder to make an asynchronous decision about what to do with the
error, the callback passed to the embedder takes ownership of the
SSLErrorHandler.

This CL is based on top of two cleanup CLs:
1. https://codereview.chromium.org/2220603003: Remove unnecessary
|result| argument from AllowCertificateError()
2. https://codereview.chromium.org/2222003002: Combine SSLCertErrorHandler
and SSLErrorHandler

BUG=558491
==========

to

==========
Make SSLErrorHandler UI-thread-only and not-refcounted

This CL makes the SSLErrorHandler class UI-thread-only
(instead of shared between UI and IO as it was previously) and not
refcounted. When a decision about an error is made synchronously, the
SSLErrorHandler is owned by SSLPolicy. When SSLPolicy calls out to the
embedder to make an asynchronous decision about what to do with the
error, the callback passed to the embedder takes ownership of the
SSLErrorHandler.

This CL is based on top of two cleanup CLs:
1. https://codereview.chromium.org/2220603003: Remove unnecessary
|result| argument from AllowCertificateError()
2. https://codereview.chromium.org/2222003002: Combine SSLCertErrorHandler
and SSLErrorHandler

BUG=558491
==========

[commit-bot: I haz the power, 2016-08-09 15:43:20]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-08-09 15:49:39]

Description was changed from

==========
Make SSLErrorHandler UI-thread-only and not-refcounted

This CL makes the SSLErrorHandler class UI-thread-only
(instead of shared between UI and IO as it was previously) and not
refcounted. When a decision about an error is made synchronously, the
SSLErrorHandler is owned by SSLPolicy. When SSLPolicy calls out to the
embedder to make an asynchronous decision about what to do with the
error, the callback passed to the embedder takes ownership of the
SSLErrorHandler.

This CL is based on top of two cleanup CLs:
1. https://codereview.chromium.org/2220603003: Remove unnecessary
|result| argument from AllowCertificateError()
2. https://codereview.chromium.org/2222003002: Combine SSLCertErrorHandler
and SSLErrorHandler

BUG=558491
==========

to

==========
Make SSLErrorHandler UI-thread-only and not-refcounted

This CL makes the SSLErrorHandler class UI-thread-only
(instead of shared between UI and IO as it was previously) and not
refcounted. When a decision about an error is made synchronously, the
SSLErrorHandler is owned by SSLPolicy. When SSLPolicy calls out to the
embedder to make an asynchronous decision about what to do with the
error, the callback passed to the embedder takes ownership of the
SSLErrorHandler.

This CL is based on top of two cleanup CLs:
1. https://codereview.chromium.org/2220603003: Remove unnecessary
|result| argument from AllowCertificateError()
2. https://codereview.chromium.org/2222003002: Combine SSLCertErrorHandler
and SSLErrorHandler

BUG=558491

Committed: https://crrev.com/dc874b5d1b42f7747fa7b75b8cc2f322f877d11a
Cr-Commit-Position: refs/heads/master@{#410682}
==========

[commit-bot: I haz the power, 2016-08-09 15:49:41]

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