Verify HostZoomMapImpl lives on the UI thread.

By spamming it with DCHECK_CURRENTLY_ON() everywhere. Since no DCHECKs
failed, delete the lock and all the related code that assume the class
can be accessed on other threads. Add threading checks to ZoomController
as well.

Change-Id: I04f8315eaebd5839889a771fd5c13bf43c4d8f31
Reviewed-on: https://chromium-review.googlesource.com/762529Reviewed-by: James MacLean <wjmaclean@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517891}

components/zoom/zoom_controller.cc

@@ -6,6 +6,7 @@
 
 #include "components/zoom/zoom_event_manager.h"
 #include "components/zoom/zoom_observer.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/host_zoom_map.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
@@ -20,6 +21,8 @@
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(zoom::ZoomController);
 
+using content::BrowserThread;
+
 namespace zoom {
 
 double ZoomController::GetZoomLevelForWebContents(
@@ -40,6 +43,7 @@ ZoomController::ZoomController(content::WebContents* web_contents)
       zoom_mode_(ZOOM_MODE_DEFAULT),
       zoom_level_(1.0),
       browser_context_(web_contents->GetBrowserContext()) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   host_zoom_map_ = content::HostZoomMap::GetForWebContents(web_contents);
   zoom_level_ = host_zoom_map_->GetDefaultZoomLevel();
 
@@ -49,43 +53,52 @@ ZoomController::ZoomController(content::WebContents* web_contents)
   UpdateState(std::string());
 }
 
-ZoomController::~ZoomController() {}
+ZoomController::~ZoomController() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+}
 
 bool ZoomController::IsAtDefaultZoom() const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return content::ZoomValuesEqual(GetZoomLevel(), GetDefaultZoomLevel());
 }
 
 ZoomController::RelativeZoom ZoomController::GetZoomRelativeToDefault() const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   double current_level = GetZoomLevel();
   double default_level = GetDefaultZoomLevel();
   if (content::ZoomValuesEqual(current_level, default_level))
     return ZOOM_AT_DEFAULT_ZOOM;
-  else if (current_level > default_level)
+  if (current_level > default_level)
     return ZOOM_ABOVE_DEFAULT_ZOOM;
   return ZOOM_BELOW_DEFAULT_ZOOM;
 }
 
 void ZoomController::AddObserver(ZoomObserver* observer) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observers_.AddObserver(observer);
 }
 
 void ZoomController::RemoveObserver(ZoomObserver* observer) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observers_.RemoveObserver(observer);
 }
 
 double ZoomController::GetZoomLevel() const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return zoom_mode_ == ZOOM_MODE_MANUAL
              ? zoom_level_
              : content::HostZoomMap::GetZoomLevel(web_contents());
 }
 
 int ZoomController::GetZoomPercent() const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   double zoom_factor = content::ZoomLevelToZoomFactor(GetZoomLevel());
   // Round double for return.
   return static_cast<int>(zoom_factor * 100 + 0.5);
 }
 
 bool ZoomController::SetZoomLevel(double zoom_level) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // A client did not initiate this zoom change.
   return SetZoomLevelByClient(zoom_level, nullptr);
 }
@@ -93,6 +106,7 @@ bool ZoomController::SetZoomLevel(double zoom_level) {
 bool ZoomController::SetZoomLevelByClient(
     double zoom_level,
     const scoped_refptr<const ZoomRequestClient>& client) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   content::NavigationEntry* entry =
       web_contents()->GetController().GetLastCommittedEntry();
   // Cannot zoom in disabled mode. Also, don't allow changing zoom level on
@@ -167,6 +181,7 @@ bool ZoomController::SetZoomLevelByClient(
 }
 
 void ZoomController::SetZoomMode(ZoomMode new_mode) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (new_mode == zoom_mode_)
     return;
 
@@ -260,6 +275,7 @@ void ZoomController::SetZoomMode(ZoomMode new_mode) {
 }
 
 void ZoomController::ResetZoomModeOnNavigationIfNeeded(const GURL& url) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (zoom_mode_ != ZOOM_MODE_ISOLATED && zoom_mode_ != ZOOM_MODE_MANUAL)
     return;
 
@@ -286,6 +302,7 @@ void ZoomController::ResetZoomModeOnNavigationIfNeeded(const GURL& url) {
 
 void ZoomController::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
     return;
 
@@ -302,6 +319,7 @@ void ZoomController::DidFinishNavigation(
 }
 
 void ZoomController::WebContentsDestroyed() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // At this point we should no longer be sending any zoom events with this
   // WebContents.
   observers_.Clear();
@@ -310,6 +328,7 @@ void ZoomController::WebContentsDestroyed() {
 void ZoomController::RenderFrameHostChanged(
     content::RenderFrameHost* old_host,
     content::RenderFrameHost* new_host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // If our associated HostZoomMap changes, update our event subscription.
   content::HostZoomMap* new_host_zoom_map =
       content::HostZoomMap::GetForWebContents(web_contents());
@@ -323,10 +342,12 @@ void ZoomController::RenderFrameHostChanged(
 
 void ZoomController::OnZoomLevelChanged(
     const content::HostZoomMap::ZoomLevelChange& change) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   UpdateState(change.host);
 }
 
 void ZoomController::UpdateState(const std::string& host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // If |host| is empty, all observers should be updated.
   if (!host.empty()) {
     // Use the navigation entry's URL instead of the WebContents' so virtual
@@ -374,6 +395,7 @@ void ZoomController::SetPageScaleFactorIsOneForTesting(bool is_one) {
 }
 
 bool ZoomController::PageScaleFactorIsOne() const {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return content::HostZoomMap::PageScaleFactorIsOne(web_contents());
 }
 

components/zoom/zoom_controller.h

@@ -39,7 +39,8 @@ class ZoomRequestClient : public base::RefCounted<ZoomRequestClient> {
   DISALLOW_COPY_AND_ASSIGN(ZoomRequestClient);
 };
 
-// Per-tab class to manage zoom changes and the Omnibox zoom icon.
+// Per-tab class to manage zoom changes and the Omnibox zoom icon. Lives on the
+// UI thread.
 class ZoomController : public content::WebContentsObserver,
                        public content::WebContentsUserData<ZoomController> {
  public:

content/browser/host_zoom_map_impl.h

@@ -13,15 +13,13 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/sequenced_task_runner_helpers.h"
-#include "base/synchronization/lock.h"
 #include "content/public/browser/host_zoom_map.h"
 
 namespace content {
 
 class WebContentsImpl;
 
-// HostZoomMap needs to be deleted on the UI thread because it listens
-// to notifications on there.
+// HostZoomMap lives on the UI thread.
 class CONTENT_EXPORT HostZoomMapImpl : public HostZoomMap {
  public:
   HostZoomMapImpl();
@@ -84,15 +82,11 @@ class CONTENT_EXPORT HostZoomMapImpl : public HostZoomMap {
   // Returns the temporary zoom level that's only valid for the lifetime of
   // the given WebContents (i.e. isn't saved and doesn't affect other
   // WebContentses) if it exists, the default zoom level otherwise.
-  //
-  // This may be called on any thread.
   double GetTemporaryZoomLevel(int render_process_id,
                                int render_view_id) const;
 
   // Returns the zoom level regardless of whether it's temporary, host-keyed or
   // scheme+host-keyed.
-  //
-  // This may be called on any thread.
   double GetZoomLevelForView(const GURL& url,
                              int render_process_id,
                              int render_view_id) const;
@@ -128,12 +122,6 @@ class CONTENT_EXPORT HostZoomMapImpl : public HostZoomMap {
 
   double GetZoomLevelForHost(const std::string& host) const;
 
-  // Non-locked versions for internal use. These should only be called within
-  // a scope where a lock has been acquired.
-  double GetZoomLevelForHostInternal(const std::string& host) const;
-  double GetZoomLevelForHostAndSchemeInternal(const std::string& scheme,
-                                              const std::string& host) const;
-
   // Set a zoom level for |host| and store the |last_modified| timestamp.
   // Use only to explicitly set a timestamp.
   void SetZoomLevelForHostInternal(const std::string& host,
@@ -151,7 +139,7 @@ class CONTENT_EXPORT HostZoomMapImpl : public HostZoomMap {
   base::CallbackList<void(const ZoomLevelChange&)>
       zoom_level_changed_callbacks_;
 
-  // Copy of the pref data, so that we can read it on the IO thread.
+  // Copy of the pref data.
   HostZoomLevels host_zoom_levels_;
   SchemeHostZoomLevels scheme_host_zoom_levels_;
   double default_zoom_level_;
@@ -161,11 +149,6 @@ class CONTENT_EXPORT HostZoomMapImpl : public HostZoomMap {
 
   TemporaryZoomLevels temporary_zoom_levels_;
 
-  // Used around accesses to |host_zoom_levels_|, |default_zoom_level_|,
-  // |temporary_zoom_levels_|, and |view_page_scale_factors_are_one_| to
-  // guarantee thread safety.
-  mutable base::Lock lock_;
-
   bool store_last_modified_;
 
   std::unique_ptr<base::Clock> clock_;