arc: Reduce logspam when ARC is not enabled/available

chrome/browser/chromeos/arc/process/arc_process_service.cc

Index: chrome/browser/chromeos/arc/process/arc_process_service.cc
diff --git a/chrome/browser/chromeos/arc/process/arc_process_service.cc b/chrome/browser/chromeos/arc/process/arc_process_service.cc
index c91e741d8ccfd142901713a65d932b4fe8f4deb4..71d729ff148408ad1963cb33df21f60063676557 100644
--- a/chrome/browser/chromeos/arc/process/arc_process_service.cc
+++ b/chrome/browser/chromeos/arc/process/arc_process_service.cc
@@ -227,11 +227,6 @@ ArcProcessService* ArcProcessService::Get() {
   return g_arc_process_service;
 }
 
-void ArcProcessService::OnInstanceReady() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  GetTaskRunner()->PostTask(FROM_HERE, base::Bind(&Reset, nspid_to_pid_));
-}
-
 void ArcProcessService::RequestSystemProcessList(
     RequestProcessListCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -245,12 +240,19 @@ bool ArcProcessService::RequestAppProcessList(
     RequestProcessListCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  // Since several services call this class to get information about the ARC
+  // process list, it can produce a lot of logspam when the board is ARC-ready
+  // but the user has not opted into ARC. This redundant check avoids that
+  // logspam.
+  if (!instance_ready_)
+    return false;

[Yusuke Sato, 2016/11/30 23:29:39]

I think this would also remove legit logs when the user has opted in to ARC but
the instance is not ready for some reason.

Can we instead use ArcBridgeService::GetEnabled() here or even in
GetInstanceForMethod()?

[Luis Héctor Chávez, 2016/11/30 23:45:08]

ArcBridgeService::GetEnabled() unfortunately does not paint the whole picture :(
The two services that call this are already gated upon that, but still manage to
produce logspam.

The fundamental problem is that the calls to RequestAppProcessList()  shouldn't
be made in the first place unless |instance_ready_| is true. One of the callers
cannot be safely notified of this because it is a singleton, and outlives
ArcBridgeService, so it cannot register itself as an Observer (or rather, it
can, but then the observer list will DCHECK upon destruction since it won't be
empty). The other caller does behave correctly by only calling this after the
instance is ready, because it does observe the readiness of the process
instance.

[Yusuke Sato, 2016/11/30 23:59:30]

The DCHECK in ~ObserverList<> is actually an optional feature, and
components/arc/'s observer_list_ doesn't seem to use it.

 template <class ObserverType, bool check_empty = false>
 class ObserverList : public ObserverListBase<ObserverType> {

We can probably fix the caller and remove the instance_ready_ member variable? I
feel like that's probably a cleaner fix. WDYT?

[Yusuke Sato, 2016/12/01 02:29:21]

Chatted with Luis offline. Let's keep the member variable as-is. Since some of
the users of this function and ArcBridgeService are created roughly at the same
time in the Main() function, adding dependency on ABS seems like a bad idea.
Small modifications to Main() may break things.

As we chatted offline, to prevent future logspam issues, please consider adding
opt-in check to GetInstanceForMethod(). Doing it in a different CL is just fine.

+
   mojom::ProcessInstance* process_instance =
       arc_bridge_service()->process()->GetInstanceForMethod(
           "RequestProcessList");
-  if (!process_instance) {
+  if (!process_instance)
     return false;
-  }
+
   process_instance->RequestProcessList(
       base::Bind(&ArcProcessService::OnReceiveProcessList,
                  weak_ptr_factory_.GetWeakPtr(), callback));
@@ -273,6 +275,17 @@ scoped_refptr<base::SingleThreadTaskRunner> ArcProcessService::GetTaskRunner() {
   return heavy_task_thread_.task_runner();
 }
 
+void ArcProcessService::OnInstanceReady() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  GetTaskRunner()->PostTask(FROM_HERE, base::Bind(&Reset, nspid_to_pid_));
+  instance_ready_ = true;
+}
+
+void ArcProcessService::OnInstanceClosed() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  instance_ready_ = false;
+}
+
 inline ArcProcessService::NSPidToPidMap::NSPidToPidMap() {}
 
 inline ArcProcessService::NSPidToPidMap::~NSPidToPidMap() {}