Manage insecure content flags in SSLManager, not WebContentsImpl

Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when it makes sense
(i.e. on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests

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

[estark, 2016-10-12 06:36:14]

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

[commit-bot: I haz the power, 2016-10-12 06:36:30]

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

[estark, 2016-10-12 06:39:22]

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

[estark, 2016-10-12 06:39:22]

clamy, could you please review?

[estark, 2016-10-12 07:01:19]

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

https://codereview.chromium.org/2408393003/diff/1/content/browser/ssl/ssl_man...
content/browser/ssl/ssl_manager.cc:374: entry->GetSSL().content_status &=
~SSLStatus::DISPLAYED_INSECURE_CONTENT;
You might notice here that we used to clear flags in some cases in UpdateEntry()
and we no longer ever clear flags.

AFAICT this logic existed to deal with the odd navigation flow that I described
in the CL description. The flow before went like this:
1. Navigation commits, UpdateEntry() is called and uses the WebContentsImpl
flags from the previous navigation.
2. Post-commit tasks run, WebContentsImpl clears its flags, UpdateEntry() is
called again and now has to clear the flags that it set in step #1 because they
are no longer set on the WebContentsImpl.

Now, when a navigation commits, UpdateEntry() is called and does *not* read
WebContentsImpl flags from the previous navigation, so there is no need for a
step #2 to clear them.

(Other than that odd navigation flow, these flags never need to get cleared once
they are set on an SSLStatus.)

[commit-bot: I haz the power, 2016-10-12 07:36:44]

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

[commit-bot: I haz the power, 2016-10-12 07:36:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2016-10-12 08:12:07]

Oops. There was a test failure that made me realize that this might not make
sense... please hold off on reviewing, I'll have to take another look at this.

[estark, 2016-10-12 08:12:31]

Description was changed from

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

to

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

[estark, 2016-10-12 17:56:47]

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

[commit-bot: I haz the power, 2016-10-12 17:57:14]

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

[estark, 2016-10-12 18:53:21]

Description was changed from

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

to

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when necessary (i.e.
on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

[estark, 2016-10-12 19:15:04]

Description was changed from

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when necessary (i.e.
on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

to

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when it makes sense
(i.e. on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

[estark, 2016-10-12 19:15:18]

Description was changed from

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when it makes sense
(i.e. on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

to

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when it makes sense
(i.e. on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

[estark, 2016-10-12 19:19:32]

estark@chromium.org changed reviewers:
+ jam@chromium.org
- clamy@chromium.org

[estark, 2016-10-12 19:19:33]

Ok, I think this is ready for review now. (I had missed the fact that we
actually do want to preserve content status flags across navigations for
non-main-frame navigations.)

Kicking over to jam for time zone convenience, but clamy, please feel free to
review as well if you'd like to.

[estark, 2016-10-12 19:21:26]

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

https://codereview.chromium.org/2408393003/diff/40001/content/browser/ssl/ssl...
content/browser/ssl/ssl_manager.cc:410: void
SSLManager::NotifySSLInternalStateChanged(BrowserContext* context) {
A note about why this is private now: we were previously calling this for any
state changed, but I don't think that's necessary; we only need to call this for
state changes that can affect other tabs, namely |ssl_host_state_delegate|
changes. Since all relevant state changes go directly to SSLManager now,
SSLManager can internally decide when NotifySSLInternalStateChanged() needs to
be called.

[jam, 2016-10-12 21:30:44]

lgtm

[commit-bot: I haz the power, 2016-10-12 21:50:48]

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

[commit-bot: I haz the power, 2016-10-12 21:50:49]

Dry run: This issue passed the CQ dry run.

[estark, 2016-10-12 21:55:57]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-10-12 21:56:18]

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-10-12 22:03:33]

Description was changed from

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when it makes sense
(i.e. on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

to

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when it makes sense
(i.e. on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

[commit-bot: I haz the power, 2016-10-12 22:03:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-12 22:06:06]

Description was changed from

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when it makes sense
(i.e. on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests
==========

to

==========
Manage insecure content flags in SSLManager, not WebContentsImpl

Previously, WebContentsImpl managed various flags related to insecure
content, and SSLManager::UpdateEntry() read these flags and transferred
them on to the NavigationEntry's SSLStatus. Now, instead of managing
state itself, WebContentsImpl simply passes on the relevant signals to
SSLManager, which tweaks the flags on the SSLStatus directly rather than
by reading them off the WebContents.

This CL also does a related clean-up of making
SSLManager::NotifySSLInternalStateChanged() private; it should not need
to be called externally and I'm not sure why it ever was, since
SSLManager calls it itself after making relevant internal state changes.

The advantages of this are:
1. Simplifies the WebContents interface and removes unnecessary state on
WebContentsImpl.
2. Fixes an odd, arguably buggy navigation flow. Previously, when a
navigation committed, SSLManager::UpdateEntry() would read the flags
from the WebContentsImpl which were still corresponding to
the *previous* navigation entry. Only in a post-commit task would
WebContentsImpl clear the flags and call UpdateEntry() again. This was
probably never a visible bug because the oddity only existed between
DidCommitProvisionalLoad() and post-commit tasks, but it was still weird
to have any length of time in which the SSLStatus was influenced by the
previous navigation's state. Now, when a navigation commits, we copy over
the previous navigation's content status flags only when it makes sense
(i.e. on non-main-frame navigations), instead of copying them and then later
clearing them as we were doing before.

BUG=
TEST=essentially a refactor and should be covered by existing automated tests

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

[commit-bot: I haz the power, 2016-10-12 22:06:07]

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