Remove unneeded DCHECK for connection status.

Fix a DCHECK failure in WebsiteSettings::Init()

Replace DCHECK with conditions and set the connection status as
unencrypted, if the security_style is unknown / HTTPS without a
certificate / not HTTPS.

BUG=344891
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269560

[babu, 2014-02-21 22:29:43]

Markus, could you please review this CL?

[markusheintz_, 2014-02-24 14:20:36]

https://codereview.chromium.org/172173004/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings.cc (left):

https://codereview.chromium.org/172173004/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.cc:440:
DCHECK_EQ(ssl.security_style, content::SECURITY_STYLE_UNAUTHENTICATED);
This DCHECK can still verify a precondition that we want to enforce.

The else below can just ensures some behavior in none debug builds that do not
contain the DCHECK.

I guess wtc@ should have a look at this since he knows the ssl code better.

[wtc, 2014-02-24 22:56:05]

Review comments on patch set 1:

https://codereview.chromium.org/172173004/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/website_settings.cc (left):

https://codereview.chromium.org/172173004/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/website_settings.cc:440:
DCHECK_EQ(ssl.security_style, content::SECURITY_STYLE_UNAUTHENTICATED);

I haven't read this code for a long time, so I have to refresh my memory. Here
are my findings.

ssl.security_style is set in two places:

1. content/browser/ssl/ssl_policy.cc: If the URL scheme is not https, it sets
ssl.security_style to SECURITY_STYLE_UNAUTHENTICATED. If the URL scheme is https
but ssl.cert_id is 0, it sets ssl.security_style to
SECURITY_STYLE_UNAUTHENTICATED.

2. SSLBlockingPage::OverrideEntry (in chrome/browser/ssl/ssl_blocking_page.cc),
which sets ssl.security_style to SECURITY_STYLE_AUTHENTICATION_BROKEN. This is
only for SSL error interstitial pages.

Assuming that we only use an SSL error interstitial page for certificate errors,
it seems that !ssl.cert_id (which means no certificate) implies that
ssl.security_style is SECURITY_STYLE_UNAUTHENTICATED. However, the "Not HTTPS"
comment is wrong. It should say "HTTPS without a certificate, or not HTTPS".

The "HTTPS without a certificate" case can happen if we visit an HTTPS URL
through a proxy but we get an HTTP error response from the proxy server rather
than the destination server.

So, I'd rewrite this code as follows:

  if (!ssl.cert_id) {
    // HTTPS without a certificate, or not HTTPS.
    DCHECK_EQ(ssl.security_style, content::SECURITY_STYLE_UNAUTHENTICATED);
    site_connection_status_ = SITE_CONNECTION_STATUS_UNENCRYPTED;

    site_connection_details_.assign(l10n_util::GetStringFUTF16(
        IDS_PAGE_INFO_SECURITY_TAB_NOT_ENCRYPTED_CONNECTION_TEXT,
        subject_name));

or:

  if (ssl.security_style == content::SECURITY_STYLE_UNAUTHENTICATED) {
    // HTTPS without a certificate, or not HTTPS.
    DCHECK(!ssl.cert_id);
    site_connection_status_ = SITE_CONNECTION_STATUS_UNENCRYPTED;

    site_connection_details_.assign(l10n_util::GetStringFUTF16(
        IDS_PAGE_INFO_SECURITY_TAB_NOT_ENCRYPTED_CONNECTION_TEXT,
        subject_name));

I think the latter is more clear.

[babu, 2014-02-25 16:13:51]

Markus and wtc, thanks a lot for providing thorough information. I will update
the patch.

[babu, 2014-02-25 16:30:36]

Patch updated with @wtc's suggested fix.

[markusheintz_, 2014-02-25 19:18:17]

LGTM

Thanks a lot for fixing this.

[wtc, 2014-02-25 22:42:23]

Patch set 2 LGTM.

[wtc, 2014-02-25 22:55:27]

https://codereview.chromium.org/172173004/diff/60001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (left):

https://codereview.chromium.org/172173004/diff/60001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:440:
DCHECK_EQ(ssl.security_style, content::SECURITY_STYLE_UNAUTHENTICATED);

Babu:

I just looked at bug 344891 that you filed. I didn't know that
this DCHECK actually failed. That is important info.

What was the value of ssl.security_style when the DCHECK failed?
content::SECURITY_STYLE_UNKNOWN? If so, we may need a different
solution. Either we should not display the page infobubble while
the page is still loading, or we should add a new case for
content::SECURITY_STYLE_UNKNOWN, like this:

  if (ssl.security_style == content::SECURITY_STYLE_UNKNOWN) {
    // Page is still loading.
    ...
  } else if (ssl.security_style == content::SECURITY_STYLE_UNAUTHENTICATED) {
    // HTTPS without a certificate, or not HTTPS.
    ...
  } else if (ssl.security_bits < 0) {
    ...

Perhaps the content::SECURITY_STYLE_UNKNOWN case is already handled by
the ssl.security_bits < 0 check?

[babu, 2014-02-26 13:48:30]

wtc, thanks for taking a deeper look into this again.

The ssl values were same as we initialized in the ssl_status.cc
(ssl.security_style was content::SECURITY_STYLE_UNKNOWN, ssl.cert_id was 0 and
ssl.security_bits was -1) during the initial page load phase, and that was the
reason for hitting the DCHECK_EQ().

As you mentioned in the previous comment, with the Patch set 2, the
SECURITY_STYLE_UNKNOWN case is handled by the ssl.security_bits < 0 check and
set the status value as SITE_CONNECTION_STATUS_ENCRYPTED_ERROR. If you think
this is incorrect, we can add a new check for SECURITY_STYLE_UNKNOWN case and
handle it differently.

[wtc, 2014-02-27 02:43:55]

On 2014/02/26 13:48:30, babu wrote:
>
> The ssl values were same as we initialized in the ssl_status.cc
> (ssl.security_style was content::SECURITY_STYLE_UNKNOWN, ssl.cert_id was 0 and
> ssl.security_bits was -1) during the initial page load phase, and that was the
> reason for hitting the DCHECK_EQ().

I see... The best fix is to not display the page info bubble during
the initial page load phase. Do you know how to do that? I am not
familiar with this part of Chrome.

If none of us knows how to do that, we will do the next best thing
and handle the SECURITY_STYLE_UNKNOWN case explicitly. The DCHECK
that failed is based on the assumption that security_style has been
set and therefore is not the initial value SECURITY_STYLE_UNKNOWN.
If that assumption is wrong, we need to change the code. Thanks.

[wtc, 2014-02-27 22:30:29]

https://codereview.chromium.org/172173004/diff/60001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/172173004/diff/60001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:438: if
(ssl.security_style == content::SECURITY_STYLE_UNAUTHENTICATED) {

As a workaround for the inability to disable the page info bubble during the
initial page load phase, I suggest we add a new case before this case:

  if (ssl.security_style == content::SECURITY_STYLE_UNKNOWN) {
    // Page is still loading, so SSL status is not yet available. Say nothing.
    DCHECK_EQ(security_bits, -1);
    site_connection_status_ = SITE_CONNECTION_STATUS_UNENCRYPTED;
  } else if (ssl.security_style == content::SECURITY_STYLE_UNAUTHENTICATED) {
    ...

Ideally we should set site_connection_status_ SITE_CONNECTION_STATUS_UNKNOWN,
but I am worried we can't do that (see the DCHECK on line 645). Setting it to
SITE_CONNECTION_STATUS_ENCRYPTED_ERROR (if we fall through to line 448) seems
wrong because we really don't know.

[babu, 2014-02-27 22:44:25]

On 2014/02/27 22:30:29, wtc wrote:
>
https://codereview.chromium.org/172173004/diff/60001/chrome/browser/ui/websit...
> File chrome/browser/ui/website_settings/website_settings.cc (right):
> 
>
https://codereview.chromium.org/172173004/diff/60001/chrome/browser/ui/websit...
> chrome/browser/ui/website_settings/website_settings.cc:438: if
> (ssl.security_style == content::SECURITY_STYLE_UNAUTHENTICATED) {
> 
> As a workaround for the inability to disable the page info bubble during the
> initial page load phase, I suggest we add a new case before this case:

wtc, sorry for bit late response. 

I'm thinking add the following check in PageInfoHelper::ProcessEvent() before
calling ShowWebsiteSettings() to prevent showing the page info bubble during the
initial page load phase.

// Do not show page info, if the page is still loading and the security
// style is unknown.
if ((nav_entry != controller.GetLastCommittedEntry()) &&
    (nav_entry->GetSSL().security_style == content::SECURITY_STYLE_UNKNOWN))
  return;

[Peter Kasting, 2014-02-28 00:54:40]

On 2014/02/27 02:43:55, wtc wrote:
> On 2014/02/26 13:48:30, babu wrote:
> > The ssl values were same as we initialized in the ssl_status.cc
> > (ssl.security_style was content::SECURITY_STYLE_UNKNOWN, ssl.cert_id was 0
and
> > ssl.security_bits was -1) during the initial page load phase, and that was
the
> > reason for hitting the DCHECK_EQ().
> 
> I see... The best fix is to not display the page info bubble during
> the initial page load phase.

Why is this the best fix?  It seems like the best fix would be to show the
bubble with the security status showing something like "Page is loading..." and
update the bubble contents once the final status is achieved.

Just having some part of Chrome's UI randomly not respond to clicks at
particular times will feel broken.

[wtc, 2014-03-04 01:24:41]

On 2014/02/28 00:54:40, Peter Kasting wrote:
> On 2014/02/27 02:43:55, wtc wrote:
>
> > I see... The best fix is to not display the page info bubble during
> > the initial page load phase.
> 
> Why is this the best fix?  It seems like the best fix would be to show the
> bubble with the security status showing something like "Page is loading..."
and
> update the bubble contents once the final status is achieved.

What you described is much better than what I proposed!

[babu, 2014-03-06 16:20:11]

Thank you all for feedback. I also agree with Peter's proposal and it totally
make sense to update the bubble contents when the security status changes.

Just wondering, does it make sense to split this into two separate issues?

(1) Fix DCHECK failure in WebsiteSettings::Init(), when the status in unknown. 
  - To fix this, we can take wtc's proposal into consideration.

  if (ssl.security_style == content::SECURITY_STYLE_UNKNOWN) {
    // Page is still loading, so SSL status is not yet available. Say nothing.
    DCHECK_EQ(ssl.security_bits, -1);
    site_connection_status_ = SITE_CONNECTION_STATUS_UNENCRYPTED;
   
    site_connection_details_.assign(l10n_util::GetStringFUTF16(
    IDS_PAGE_INFO_SECURITY_TAB_NOT_ENCRYPTED_CONNECTION_TEXT,
    subject_name));
  } else if (ssl.security_style == content::SECURITY_STYLE_UNAUTHENTICATED) {

(2) Update the site info bubble contents when the security state changes. 
  - Instead of updating only the connection status, it would be nice to update
full content of the connection tab including identity, connection status, and
site information when the state changes. I assume we can do this by listening to
WebContentsObserver::DidChangeVisibleSSLState().

  void WebsiteSettings::DidChangeVisibleSSLState() {
    .....
    UpdateSiteIdentity(...)  // We can move most of the common code from
WebsiteSettings::Init() to this new function.
    PresentSiteIdentity()
  }

If it is okay to split this into two separate CLs, I will update the patch in
this CL for issue (1) and work on UI changes in a separate CL.

[markusheintz_, 2014-04-04 12:59:05]

On 2014/03/06 16:20:11, babu wrote:
> Thank you all for feedback. I also agree with Peter's proposal and it totally
> make sense to update the bubble contents when the security status changes.
> 
> Just wondering, does it make sense to split this into two separate issues?
> 
> (1) Fix DCHECK failure in WebsiteSettings::Init(), when the status in unknown.

>   - To fix this, we can take wtc's proposal into consideration.
> 
>   if (ssl.security_style == content::SECURITY_STYLE_UNKNOWN) {
>     // Page is still loading, so SSL status is not yet available. Say nothing.
>     DCHECK_EQ(ssl.security_bits, -1);
>     site_connection_status_ = SITE_CONNECTION_STATUS_UNENCRYPTED;
>    
>     site_connection_details_.assign(l10n_util::GetStringFUTF16(
>     IDS_PAGE_INFO_SECURITY_TAB_NOT_ENCRYPTED_CONNECTION_TEXT,
>     subject_name));
>   } else if (ssl.security_style == content::SECURITY_STYLE_UNAUTHENTICATED) {
> 
> (2) Update the site info bubble contents when the security state changes. 
>   - Instead of updating only the connection status, it would be nice to update
> full content of the connection tab including identity, connection status, and
> site information when the state changes. I assume we can do this by listening
to
> WebContentsObserver::DidChangeVisibleSSLState().
> 
>   void WebsiteSettings::DidChangeVisibleSSLState() {
>     .....
>     UpdateSiteIdentity(...)  // We can move most of the common code from
> WebsiteSettings::Init() to this new function.
>     PresentSiteIdentity()
>   }
> 
> If it is okay to split this into two separate CLs, I will update the patch in
> this CL for issue (1) and work on UI changes in a separate CL.

What's the status on this?

[babu, 2014-04-07 13:54:26]

On 2014/04/04 12:59:05, markusheintz_ wrote:
> What's the status on this?

Apologies for keeping the issue open for long time. I will update the patch for
DCHECK failure right away.

[markusheintz_, 2014-05-09 08:33:51]

LGTM since this fixes the DCHECK issue.

I'll look into showing a  "loading" message and updating the bubble once the
data is there.

[babu, 2014-05-09 08:58:47]

The CQ bit was checked by sudarsana.nagineni@intel.com

[commit-bot: I haz the power, 2014-05-09 09:04:30]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/sudarsana.nagineni@intel.com/172173004...

[commit-bot: I haz the power, 2014-05-09 10:40:47]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-10 11:40:02]

Change committed as 269560