Fix SSL state in the URL bar being incorrect. Going to an EV site like https://www.verisign.com/ would not should the EV name in the URL bar unless you did something like switch tabs away and back because in my cleanup I removed the notification that this was depending on.

This patch adds a new NOTIFY_SSL_STATE_CHANGED notification which is broadcast by the various SSL components when they update the flags. The browser now listens for this notification and will update the URL bar.

BUG=1359547
TEST=see repro steps in bug

Review URL: http://codereview.chromium.org/436

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@1831 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 119 insertions, 53 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 1628952..b2ed3fa 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -234,6 +234,8 @@ Browser::Browser(const gfx::Rect& initial_bounds,
         AddObserver(this, NOTIFY_BOOKMARK_BAR_VISIBILITY_PREF_CHANGED,
                     NotificationService::AllSources());
   }
+  NotificationService::current()->AddObserver(
+      this, NOTIFY_SSL_STATE_CHANGED, NotificationService::AllSources());
 
   if (profile->HasSessionService()) {
     SessionService* session_service = profile->GetSessionService();
@@ -287,6 +289,8 @@ Browser::~Browser() {
         RemoveObserver(this, NOTIFY_BOOKMARK_BAR_VISIBILITY_PREF_CHANGED,
                        NotificationService::AllSources());
   }
+  NotificationService::current()->RemoveObserver(
+      this, NOTIFY_SSL_STATE_CHANGED, NotificationService::AllSources());
 
   // Stop hung plugin monitoring.
   ticker_.Stop();
@@ -646,14 +650,13 @@ void Browser::NavigationStateChanged(const TabContents* source,
   }
 
   // Only update the UI when something visible has changed.
-  if (changed_flags && changed_flags != TabContents::INVALIDATE_STATE)
+  if (changed_flags)
     ScheduleUIUpdate(source, changed_flags);
 
   // We don't schedule updates to the navigation commands since they will only
   // change once per navigation, so we don't have to worry about flickering.
-  if (changed_flags & TabContents::INVALIDATE_URL) {
+  if (changed_flags & TabContents::INVALIDATE_URL)
     UpdateNavigationCommands();
-  }
 }
 
 void Browser::ReplaceContents(TabContents* source, TabContents* new_contents) {
@@ -835,26 +838,40 @@ void Browser::ShowHtmlDialog(HtmlDialogContentsDelegate* delegate,
 void Browser::Observe(NotificationType type,
                       const NotificationSource& source,
                       const NotificationDetails& details) {
-  if (type == NOTIFY_BOOKMARK_BAR_VISIBILITY_PREF_CHANGED) {
-    DCHECK(!g_browser_process->IsUsingNewFrames());
-    TabContents* current_tab = GetSelectedTabContents();
-    if (current_tab) {
-      Profile* event_profile = Source<Profile>(source).ptr();
-      if (event_profile->IsSameProfile(current_tab->profile())) {
-        // This forces the browser to query for the BookmarkBar again.
-        window_->ShelfVisibilityChanged();
+  switch (type) {
+    case NOTIFY_BOOKMARK_BAR_VISIBILITY_PREF_CHANGED: {
+      DCHECK(!g_browser_process->IsUsingNewFrames());
+      TabContents* current_tab = GetSelectedTabContents();
+      if (current_tab) {
+        Profile* event_profile = Source<Profile>(source).ptr();
+        if (event_profile->IsSameProfile(current_tab->profile())) {
+          // This forces the browser to query for the BookmarkBar again.
+          window_->ShelfVisibilityChanged();
+        }
       }
+      break;
     }
-  } else if (type == NOTIFY_WEB_CONTENTS_DISCONNECTED) {
-    if (is_attempting_to_close_browser_) {
-      // Need to do this asynchronously as it will close the tab, which is
-      // currently on the call stack above us.
-      MessageLoop::current()->PostTask(FROM_HERE,
-          method_factory_.NewRunnableMethod(&Browser::ClearUnloadState,
-              Source<TabContents>(source).ptr()));
-    }
-  } else {
-    NOTREACHED() << "Got a notification we didn't register for.";
+
+    case NOTIFY_WEB_CONTENTS_DISCONNECTED:
+      if (is_attempting_to_close_browser_) {
+        // Need to do this asynchronously as it will close the tab, which is
+        // currently on the call stack above us.
+        MessageLoop::current()->PostTask(FROM_HERE,
+            method_factory_.NewRunnableMethod(&Browser::ClearUnloadState,
+                Source<TabContents>(source).ptr()));
+      }
+      break;
+
+    case NOTIFY_SSL_STATE_CHANGED:
+      // When the current tab's SSL state changes, we need to update the URL
+      // bar to reflect the new state.
+      if (GetSelectedTabContents()->controller() ==
+          Source<NavigationController>(source).ptr())
+        UpdateToolBar(false);
+      break;
+
+    default:
+      NOTREACHED() << "Got a notification we didn't register for.";
   }
 }
 
diff --git a/chrome/browser/navigation_controller.cc b/chrome/browser/navigation_controller.cc
index 2dfc9b3..df55f9b 100644
--- a/chrome/browser/navigation_controller.cc
+++ b/chrome/browser/navigation_controller.cc
@@ -537,7 +537,13 @@ void NavigationController::DidNavigateToEntry(NavigationEntry* entry,
   } else if ((existing_entry != pending_entry_) && pending_entry_ &&
              (pending_entry_->page_id() == -1) &&
              (pending_entry_->url() == existing_entry->url())) {
-    // Not a new navigation.
+    // In this case, we have a pending entry for a URL but WebCore didn't do a
+    // new navigation. This happens when you press enter in the URL bar to
+    // reload. We will create a pending entry, but WebCore will convert it to
+    // a reload since it's the same page and not create a new entry for it
+    // (the user doesn't want to have a new back/forward entry when they do
+    // this). In this case, we want to just ignore the pending entry and go back
+    // to where we were.
     existing_entry->set_unique_id(pending_entry_->unique_id());
     DiscardPendingEntry();
   } else {
diff --git a/chrome/browser/navigation_controller_unittest.cc b/chrome/browser/navigation_controller_unittest.cc
index bea08ac7..f9f22e9 100644
--- a/chrome/browser/navigation_controller_unittest.cc
+++ b/chrome/browser/navigation_controller_unittest.cc
@@ -375,7 +375,9 @@ TEST_F(NavigationControllerTest, LoadURL) {
 }
 
 // Tests what happens when the same page is loaded again.  Should not create a
-// new session history entry.
+// new session history entry. This is what happens when you press enter in the
+// URL bar to reload: a pending entry is created and then it is discarded when
+// the load commits (because WebCore didn't actually make a new entry).
 TEST_F(NavigationControllerTest, LoadURL_SamePage) {
   TestNotificationTracker notifications;
   RegisterForAllNavNotifications(&notifications, contents->controller());
diff --git a/chrome/browser/ssl_manager.cc b/chrome/browser/ssl_manager.cc
index 9089585..eabecb3 100644
--- a/chrome/browser/ssl_manager.cc
+++ b/chrome/browser/ssl_manager.cc
@@ -215,15 +215,18 @@ void SSLManager::ShowMessageWithLink(const std::wstring& msg,
 }
 
 // Delegate API method.
-void SSLManager::SetMaxSecurityStyle(SecurityStyle style) {
+bool SSLManager::SetMaxSecurityStyle(SecurityStyle style) {
   NavigationEntry* entry = controller_->GetActiveEntry();
   if (!entry) {
     NOTREACHED();
-    return;
+    return false;
   }
 
-  if (entry->ssl().security_style() > style)
+  if (entry->ssl().security_style() > style) {
     entry->ssl().set_security_style(style);
+    return true;
+  }
+  return false;
 }
 
 // Delegate API method.
@@ -609,6 +612,7 @@ void SSLManager::DidCommitProvisionalLoad(ProvisionalLoadDetails* details) {
   if (details->in_page_navigation())
     return;
 
+  bool changed = false;
   if (details->main_frame()) {
     // We are navigating to a new page, it's time to close the info bars.  They
     // will automagically disappear from the visible_info_bars_ list (when
@@ -629,6 +633,7 @@ void SSLManager::DidCommitProvisionalLoad(ProvisionalLoadDetails* details) {
       entry->ssl().set_cert_id(details->ssl_cert_id());
       entry->ssl().set_cert_status(details->ssl_cert_status());
       entry->ssl().set_security_bits(details->ssl_security_bits());
+      changed = true;
     }
 
     if (details->interstitial_page()) {
@@ -643,9 +648,17 @@ void SSLManager::DidCommitProvisionalLoad(ProvisionalLoadDetails* details) {
   // happens, use the unauthenticated (HTTP) rather than the authentication
   // broken security style so that we can detect this error condition.
   if (net::IsCertStatusError(details->ssl_cert_status()))
-    SetMaxSecurityStyle(SECURITY_STYLE_AUTHENTICATION_BROKEN);
+    changed |= SetMaxSecurityStyle(SECURITY_STYLE_AUTHENTICATION_BROKEN);
   else if (details->url().SchemeIsSecure() && !details->ssl_cert_id())
-    SetMaxSecurityStyle(SECURITY_STYLE_UNAUTHENTICATED);
+    changed |= SetMaxSecurityStyle(SECURITY_STYLE_UNAUTHENTICATED);
+
+  if (changed) {
+    // Only send the notification when something actually changed.
+    NotificationService::current()->Notify(
+        NOTIFY_SSL_STATE_CHANGED,
+        Source<NavigationController>(controller_),
+        Details<NavigationEntry>(controller_->GetActiveEntry()));
+  }
 }
 
 void SSLManager::DidFailProvisionalLoadWithError(
diff --git a/chrome/browser/ssl_manager.h b/chrome/browser/ssl_manager.h
index 419795f..5d088a7 100644
--- a/chrome/browser/ssl_manager.h
+++ b/chrome/browser/ssl_manager.h
@@ -287,7 +287,11 @@ class SSLManager : public NotificationObserver {
   // Sets the maximum security style for the page.  If the current security
   // style is lower than |style|, this will not have an effect on the security
   // indicators.
-  void SetMaxSecurityStyle(SecurityStyle style);
+  //
+  // It will return true if the navigation entry was updated or false if
+  // nothing changed. The caller is responsible for broadcasting
+  // NOTIFY_SSY_STATE_CHANGED if it returns true.
+  bool SetMaxSecurityStyle(SecurityStyle style);
 
   // Logs a message to the console of the page.
   void AddMessageToConsole(const std::wstring& msg,
diff --git a/chrome/browser/ssl_policy.cc b/chrome/browser/ssl_policy.cc
index 22d1bd7..a16e08b 100644
--- a/chrome/browser/ssl_policy.cc
+++ b/chrome/browser/ssl_policy.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/web_contents.h"
 #include "chrome/common/jstemplate_builder.h"
 #include "chrome/common/l10n_util.h"
+#include "chrome/common/notification_service.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/pref_service.h"
 #include "chrome/common/resource_bundle.h"
@@ -284,21 +285,27 @@ class DefaultPolicy : public SSLPolicy {
                                       error->request_url().host());
 
     switch (judgment) {
-    case net::X509Certificate::Policy::ALLOWED:
-      // We've been told to allow this certificate.
-      error->manager()->SetMaxSecurityStyle(
-          SECURITY_STYLE_AUTHENTICATION_BROKEN);
-      error->ContinueRequest();
-      break;
-    case net::X509Certificate::Policy::DENIED:
-      // For now we handle the DENIED as the UNKNOWN, which means a blocking
-      // page is shown to the user every time he comes back to the page.
-    case net::X509Certificate::Policy::UNKNOWN:
-      // We don't know how to handle this error.  Ask our sub-policies.
-      sub_policies_[index]->OnCertError(main_frame_url, error);
-      break;
-    default:
-      NOTREACHED();
+      case net::X509Certificate::Policy::ALLOWED:
+        // We've been told to allow this certificate.
+        if (error->manager()->SetMaxSecurityStyle(
+                SECURITY_STYLE_AUTHENTICATION_BROKEN)) {
+          NotificationService::current()->Notify(
+              NOTIFY_SSL_STATE_CHANGED,
+              Source<NavigationController>(error->manager()->controller()),
+              Details<NavigationEntry>(
+                  error->manager()->controller()->GetActiveEntry()));
+        }
+        error->ContinueRequest();
+        break;
+      case net::X509Certificate::Policy::DENIED:
+        // For now we handle the DENIED as the UNKNOWN, which means a blocking
+        // page is shown to the user every time he comes back to the page.
+      case net::X509Certificate::Policy::UNKNOWN:
+        // We don't know how to handle this error.  Ask our sub-policies.
+        sub_policies_[index]->OnCertError(main_frame_url, error);
+        break;
+      default:
+        NOTREACHED();
     }
   }
 
@@ -385,6 +392,7 @@ void SSLPolicy::OnRequestStarted(SSLManager* manager, const GURL& url,
   }
 
   NavigationEntry::SSLStatus& ssl = entry->ssl();
+  bool changed = false;
   if (!entry->url().SchemeIsSecure() ||  // Current page is not secure.
       resource_type == ResourceType::MAIN_FRAME ||  // Main frame load.
       net::IsCertStatusError(ssl.cert_status())) {  // There is already
@@ -405,6 +413,7 @@ void SSLPolicy::OnRequestStarted(SSLManager* manager, const GURL& url,
     if (net::IsCertStatusError(ssl_cert_status)) {
       // The resource is unsafe.
       if (!ssl.has_unsafe_content()) {
+        changed = true;
         ssl.set_has_unsafe_content();
         manager->SetMaxSecurityStyle(SECURITY_STYLE_AUTHENTICATION_BROKEN);
       }
@@ -419,13 +428,24 @@ void SSLPolicy::OnRequestStarted(SSLManager* manager, const GURL& url,
 
   // Now check for mixed content.
   if (entry->url().SchemeIsSecure() && !url.SchemeIsSecure()) {
-    ssl.set_has_mixed_content();
+    if (!ssl.has_mixed_content()) {
+      changed = true;
+      ssl.set_has_mixed_content();
+    }
     const std::wstring& msg = l10n_util::GetStringF(
         IDS_MIXED_CONTENT_LOG_MESSAGE,
         UTF8ToWide(entry->url().spec()),
         UTF8ToWide(url.spec()));
     manager->AddMessageToConsole(msg, MESSAGE_LEVEL_WARNING);
   }
+
+  if (changed) {
+    // Only send the notification when something actually changed.
+    NotificationService::current()->Notify(
+        NOTIFY_SSL_STATE_CHANGED,
+        Source<NavigationController>(manager->controller()),
+        Details<NavigationEntry>(entry));
+  }
 }
 
 SecurityStyle SSLPolicy::GetDefaultStyle(const GURL& url) {
diff --git a/chrome/browser/tab_contents.h b/chrome/browser/tab_contents.h
index 7d0f784..649de9f 100644
--- a/chrome/browser/tab_contents.h
+++ b/chrome/browser/tab_contents.h
@@ -65,8 +65,7 @@ class TabContents : public PageNavigator,
     INVALIDATE_URL = 1,      // The URL has changed.
     INVALIDATE_TITLE = 2,    // The title has changed.
     INVALIDATE_FAVICON = 4,  // The favicon has changed.
-    INVALIDATE_STATE = 8,    // Forms, scroll position, etc.) have changed.
-    INVALIDATE_LOAD = 16,    // The loading state has changed
+    INVALIDATE_LOAD = 8,     // The loading state has changed
 
     // Helper for forcing a refresh.
     INVALIDATE_EVERYTHING = 0xFFFFFFFF
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index 016a85c..e9c0837 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -15,7 +15,6 @@
 #include "chrome/browser/navigation_entry.h"
 #include "chrome/browser/page_info_window.h"
 #include "chrome/browser/profile.h"
-#include "chrome/browser/ssl_error_info.h"
 #include "chrome/browser/template_url.h"
 #include "chrome/browser/template_url_model.h"
 #include "chrome/browser/view_ids.h"
@@ -29,8 +28,6 @@
 #include "chrome/views/border.h"
 #include "chrome/views/root_view.h"
 #include "chrome/views/view_container.h"
-#include "googleurl/src/gurl.h"
-#include "googleurl/src/url_canon.h"
 #include "generated_resources.h"
 
 using ChromeViews::View;
diff --git a/chrome/browser/web_contents.cc b/chrome/browser/web_contents.cc
index 01cd7ae..971444d 100644
--- a/chrome/browser/web_contents.cc
+++ b/chrome/browser/web_contents.cc
@@ -1552,10 +1552,8 @@ void WebContents::UpdateState(RenderViewHost* rvh,
   }
 
   // Update the state (forms, etc.).
-  if (state != entry->content_state()) {
-    changed_flags |= INVALIDATE_STATE;
+  if (state != entry->content_state())
     entry->set_content_state(state);
-  }
 
   // Notify everybody of the changes (only when the current page changed).
   if (changed_flags && entry == controller()->GetActiveEntry())
diff --git a/chrome/common/notification_types.h b/chrome/common/notification_types.h
index 3983da5..9bf7170 100644
--- a/chrome/common/notification_types.h
+++ b/chrome/common/notification_types.h
@@ -124,6 +124,16 @@ enum NotificationType {
   // was issued.  Details in the form of a ResourceRedirectDetails are provided.
   NOTIFY_RESOURCE_RECEIVED_REDIRECT,
 
+  // The SSL state of a page has changed somehow. For example, if an insecure
+  // resource is loaded on a secure page. Note that a toplevel load commit
+  // will also update the SSL state (since the NavigationEntry is new) and this
+  // message won't always be sent in that case.
+  //
+  // The source will be the navigation controller associated with the load.
+  // There are no details. The entry changed will be the active entry of the
+  // controller.
+  NOTIFY_SSL_STATE_CHANGED,
+
   // Download start and stop notifications. Stop notifications can occur on both
   // normal completion or via a cancel operation.
   NOTIFY_DOWNLOAD_START,