Use DCHECK instead of LOG(ERROR) in embedded_worker_registry.cc

content/browser/service_worker/embedded_worker_registry.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/embedded_worker_registry.h"
 
 #include "base/bind_helpers.h"
 #include "base/stl_util.h"
 #include "content/browser/renderer_host/render_widget_helper.h"
 #include "content/browser/service_worker/embedded_worker_instance.h"
@@ skipping 35 matching lines @@
     int process_id, int embedded_worker_id) {
   return Send(process_id,
               new EmbeddedWorkerMsg_StopWorker(embedded_worker_id));
 }
 
 bool EmbeddedWorkerRegistry::OnMessageReceived(const IPC::Message& message) {
   // TODO(kinuko): Move all EmbeddedWorker message handling from
   // ServiceWorkerDispatcherHost.
 
   WorkerInstanceMap::iterator found = worker_map_.find(message.routing_id());
-  if (found == worker_map_.end()) {
-    LOG(ERROR) << "Worker " << message.routing_id() << " not registered";
+  DCHECK(found != worker_map_.end());
+  if (found == worker_map_.end())

[falken, 2014/11/06 02:07:54]

Typically we should not have constructions of the form[1]:

DCHECK(foo);
if (!foo) { ... }

[1]
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/c2X0D1Z6X2o/ddkFL...

I would like to remove this handling of an impossible condition. However, this
change seemed motivated by log output spam, which would mean these are not
impossible conditions. That's a bug we better fix.

Do you remember what motivated this change? Do we see these DCHECKs occurring
still?

     return false;
-  }
   return found->second->OnMessageReceived(message);
 }
 
 void EmbeddedWorkerRegistry::Shutdown() {
   for (WorkerInstanceMap::iterator it = worker_map_.begin();
        it != worker_map_.end();
        ++it) {
     it->second->Stop();
   }
 }
 
 void EmbeddedWorkerRegistry::OnWorkerReadyForInspection(
     int process_id,
     int embedded_worker_id) {
   WorkerInstanceMap::iterator found = worker_map_.find(embedded_worker_id);
-  if (found == worker_map_.end()) {
-    LOG(ERROR) << "Worker " << embedded_worker_id << " not registered";
+  DCHECK(found != worker_map_.end());
+  DCHECK_EQ(found->second->process_id(), process_id);
+  if (found == worker_map_.end() || found->second->process_id() != process_id)
     return;
-  }
-  if (found->second->process_id() != process_id) {
-    LOG(ERROR) << "Incorrect embedded_worker_id";
-    return;
-  }
   found->second->OnReadyForInspection();
 }
 
 void EmbeddedWorkerRegistry::OnWorkerScriptLoaded(int process_id,
                                                   int embedded_worker_id) {
   WorkerInstanceMap::iterator found = worker_map_.find(embedded_worker_id);
-  if (found == worker_map_.end()) {
-    LOG(ERROR) << "Worker " << embedded_worker_id << " not registered";
+  DCHECK(found != worker_map_.end());
+  DCHECK_EQ(found->second->process_id(), process_id);
+  if (found == worker_map_.end() || found->second->process_id() != process_id)
     return;
-  }
-  if (found->second->process_id() != process_id) {
-    LOG(ERROR) << "Incorrect embedded_worker_id";
-    return;
-  }
   found->second->OnScriptLoaded();
 }
 
 void EmbeddedWorkerRegistry::OnWorkerScriptLoadFailed(int process_id,
                                                       int embedded_worker_id) {
   WorkerInstanceMap::iterator found = worker_map_.find(embedded_worker_id);
-  if (found == worker_map_.end()) {
-    LOG(ERROR) << "Worker " << embedded_worker_id << " not registered";
+  DCHECK(found != worker_map_.end());
+  DCHECK_EQ(found->second->process_id(), process_id);
+  if (found == worker_map_.end() || found->second->process_id() != process_id)
     return;
-  }
-  if (found->second->process_id() != process_id) {
-    LOG(ERROR) << "Incorrect embedded_worker_id";
-    return;
-  }
   found->second->OnScriptLoadFailed();
 }
 
 void EmbeddedWorkerRegistry::OnWorkerStarted(
     int process_id, int thread_id, int embedded_worker_id) {
   DCHECK(!ContainsKey(worker_process_map_, process_id) ||
          worker_process_map_[process_id].count(embedded_worker_id) == 0);
   WorkerInstanceMap::iterator found = worker_map_.find(embedded_worker_id);
-  if (found == worker_map_.end()) {
-    LOG(ERROR) << "Worker " << embedded_worker_id << " not registered";
+  DCHECK(found != worker_map_.end());
+  DCHECK_EQ(found->second->process_id(), process_id);
+  if (found == worker_map_.end() || found->second->process_id() != process_id)
     return;
-  }
-  if (found->second->process_id() != process_id) {
-    LOG(ERROR) << "Incorrect embedded_worker_id";
-    return;
-  }
   worker_process_map_[process_id].insert(embedded_worker_id);
   found->second->OnStarted(thread_id);
 }
 
 void EmbeddedWorkerRegistry::OnWorkerStopped(
     int process_id, int embedded_worker_id) {
   WorkerInstanceMap::iterator found = worker_map_.find(embedded_worker_id);
-  if (found == worker_map_.end()) {
-    LOG(ERROR) << "Worker " << embedded_worker_id << " not registered";
+  DCHECK(found != worker_map_.end());
+  DCHECK_EQ(found->second->process_id(), process_id);
+  if (found == worker_map_.end() || found->second->process_id() != process_id)
     return;
-  }
-  if (found->second->process_id() != process_id) {
-    LOG(ERROR) << "Incorrect embedded_worker_id";
-    return;
-  }
   worker_process_map_[process_id].erase(embedded_worker_id);
   found->second->OnStopped();
 }
 
 void EmbeddedWorkerRegistry::OnPausedAfterDownload(
     int process_id, int embedded_worker_id) {
   WorkerInstanceMap::iterator found = worker_map_.find(embedded_worker_id);
-  if (found == worker_map_.end()) {
-    LOG(ERROR) << "Worker " << embedded_worker_id << " not registered";
+  DCHECK(found != worker_map_.end());
+  DCHECK_EQ(found->second->process_id(), process_id);
+  if (found == worker_map_.end() || found->second->process_id() != process_id)
     return;
-  }
-  if (found->second->process_id() != process_id) {
-    LOG(ERROR) << "Incorrect embedded_worker_id";
-    return;
-  }
   found->second->OnPausedAfterDownload();
 }
 
 void EmbeddedWorkerRegistry::OnReportException(
     int embedded_worker_id,
     const base::string16& error_message,
     int line_number,
     int column_number,
     const GURL& source_url) {
   WorkerInstanceMap::iterator found = worker_map_.find(embedded_worker_id);
-  if (found == worker_map_.end()) {
-    LOG(ERROR) << "Worker " << embedded_worker_id << " not registered";
+  DCHECK(found != worker_map_.end());
+  if (found == worker_map_.end())
     return;
-  }
   found->second->OnReportException(
       error_message, line_number, column_number, source_url);
 }
 
 void EmbeddedWorkerRegistry::OnReportConsoleMessage(
     int embedded_worker_id,
     int source_identifier,
     int message_level,
     const base::string16& message,
     int line_number,
     const GURL& source_url) {
   WorkerInstanceMap::iterator found = worker_map_.find(embedded_worker_id);
-  if (found == worker_map_.end()) {
-    LOG(ERROR) << "Worker " << embedded_worker_id << " not registered";
+  DCHECK(found != worker_map_.end());
+  if (found == worker_map_.end())
     return;
-  }
   found->second->OnReportConsoleMessage(
       source_identifier, message_level, message, line_number, source_url);
 }
 
 void EmbeddedWorkerRegistry::AddChildProcessSender(
     int process_id, IPC::Sender* sender) {
   process_sender_map_[process_id] = sender;
   DCHECK(!ContainsKey(worker_process_map_, process_id));
 }
 
@@ skipping 67 matching lines @@
 }
 
 void EmbeddedWorkerRegistry::RemoveWorker(int process_id,
                                           int embedded_worker_id) {
   DCHECK(ContainsKey(worker_map_, embedded_worker_id));
   worker_map_.erase(embedded_worker_id);
   worker_process_map_.erase(process_id);
 }
 
 }  // namespace content