Stop revoking cert exceptions on resources loaded from cache

Stop revoking cert exceptions on resources loaded from cache

SSLPolicy currently forgets certiciate exceptions when it sees a good
certificate from the cache. This is weird since the cached valid cert
might have been there for longer than the exception. See bug for more
detailed description.

BUG=634553
R=jww@chromium.org

Committed: https://crrev.com/ee3b15d53bf6ca29c026da7153445b51cefceaa8
Cr-Commit-Position: refs/heads/master@{#411105}

[felt, 2016-08-05 21:48:43]

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

[felt, 2016-08-05 21:49:21]

jww, ptal?

[commit-bot: I haz the power, 2016-08-05 21:49:29]

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-05 23:21:28]

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

[commit-bot: I haz the power, 2016-08-05 23:21:29]

Dry run: This issue passed the CQ dry run.

[jww, 2016-08-09 02:42:18]

lgtm, with a few nits.

https://codereview.chromium.org/2218253002/diff/1/content/browser/ssl/ssl_man...
File content/browser/ssl/ssl_manager.cc (left):

https://codereview.chromium.org/2218253002/diff/1/content/browser/ssl/ssl_man...
content/browser/ssl/ssl_manager.cc:145: // Simulate loading this resource
through the usual path.
nit: Probably should update the comment ssl_policy.cc at
https://cs.chromium.org/chromium/src/content/browser/ssl/ssl_policy.cc?sq=pac...
to state that the subresource case is purposefully ignored (and why).

https://codereview.chromium.org/2218253002/diff/1/content/browser/ssl/ssl_man...
File content/browser/ssl/ssl_manager.h (left):

https://codereview.chromium.org/2218253002/diff/1/content/browser/ssl/ssl_man...
content/browser/ssl/ssl_manager.h:86: void DidLoadFromMemoryCache(const
LoadFromMemoryCacheDetails& details);
nit: Should also delete "struct LoadFromMemoryCacheDetails" predeclaration at
top of file (it's unused now, I believe).

https://codereview.chromium.org/2218253002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2218253002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:3467:
DidLoadResourceFromMemoryCache(details));
Side question: why isn't SSLManager a WebContentsObserver, given that it seems
to implement several methods that are called identically right before the
WebContentsObserver methods are called? Would it simplify future method changes
if we just made it a WebContentsObserver?

If the answer is 'yes,' definitely not a blocker, but we may want to file a bug.

[felt, 2016-08-10 14:17:18]

https://codereview.chromium.org/2218253002/diff/1/content/browser/ssl/ssl_man...
File content/browser/ssl/ssl_manager.cc (left):

https://codereview.chromium.org/2218253002/diff/1/content/browser/ssl/ssl_man...
content/browser/ssl/ssl_manager.cc:145: // Simulate loading this resource
through the usual path.
On 2016/08/09 02:42:18, jww wrote:
> nit: Probably should update the comment ssl_policy.cc at
>
https://cs.chromium.org/chromium/src/content/browser/ssl/ssl_policy.cc?sq=pac...
> to state that the subresource case is purposefully ignored (and why).

Done.

https://codereview.chromium.org/2218253002/diff/1/content/browser/ssl/ssl_man...
File content/browser/ssl/ssl_manager.h (left):

https://codereview.chromium.org/2218253002/diff/1/content/browser/ssl/ssl_man...
content/browser/ssl/ssl_manager.h:86: void DidLoadFromMemoryCache(const
LoadFromMemoryCacheDetails& details);
On 2016/08/09 02:42:18, jww wrote:
> nit: Should also delete "struct LoadFromMemoryCacheDetails" predeclaration at
> top of file (it's unused now, I believe).

Done.

https://codereview.chromium.org/2218253002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2218253002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:3467:
DidLoadResourceFromMemoryCache(details));
On 2016/08/09 02:42:18, jww wrote:
> Side question: why isn't SSLManager a WebContentsObserver, given that it seems
> to implement several methods that are called identically right before the
> WebContentsObserver methods are called? Would it simplify future method
changes
> if we just made it a WebContentsObserver?
> 
> If the answer is 'yes,' definitely not a blocker, but we may want to file a
bug.

That's a great question. I was wondering that too. I don't see any reason why.

[felt, 2016-08-10 14:19:05]

felt@chromium.org changed reviewers:
+ creis@chromium.org

[felt, 2016-08-10 14:19:06]

creis, can you pls review this small change to
content/browser/web_contents/web_contents_impl.cc ?

[felt, 2016-08-10 14:28:54]

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

[commit-bot: I haz the power, 2016-08-10 14:29:22]

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

[felt, 2016-08-10 14:39:30]

Patchset #3 (id:40001) has been deleted

[felt, 2016-08-10 14:39:35]

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

[commit-bot: I haz the power, 2016-08-10 14:40:01]

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-10 15:14:38]

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

[commit-bot: I haz the power, 2016-08-10 15:14:39]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Charlie Reis, 2016-08-10 16:22:41]

LGTM

[felt, 2016-08-10 16:23:19]

The CQ bit was checked by felt@chromium.org

[felt, 2016-08-10 16:23:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org
Link to the patchset: https://codereview.chromium.org/2218253002/#ps60001
(title: "Clearer comment")

[commit-bot: I haz the power, 2016-08-10 16:23:41]

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-10 18:48:40]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-10 18:52:09]

Description was changed from

==========
Stop revoking cert exceptions on resources loaded from cache

SSLPolicy currently forgets certiciate exceptions when it sees a good
certificate from the cache. This is weird since the cached valid cert
might have been there for longer than the exception. See bug for more
detailed description.

BUG=634553
R=jww@chromium.org
==========

to

==========
Stop revoking cert exceptions on resources loaded from cache

SSLPolicy currently forgets certiciate exceptions when it sees a good
certificate from the cache. This is weird since the cached valid cert
might have been there for longer than the exception. See bug for more
detailed description.

BUG=634553
R=jww@chromium.org

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

[commit-bot: I haz the power, 2016-08-10 18:52:10]

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