arc: Reduce logspam when ARC is not enabled/available

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

 // Copyright 2016 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.
 //
 // The main point of this class is to cache ARC proc nspid<->pid mapping
 // globally. Since the calculation is costly, a dedicated worker thread is
 // used. All read/write of its internal data structure (i.e., the mapping)
 // should be on this thread.
 
 #include "chrome/browser/chromeos/arc/process/arc_process_service.h"
@@ skipping 209 matching lines @@
   g_arc_process_service = nullptr;
   arc_bridge_service()->process()->RemoveObserver(this);
 }
 
 // static
 ArcProcessService* ArcProcessService::Get() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   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);
 
   base::PostTaskAndReplyWithResult(GetTaskRunner().get(), FROM_HERE,
                                    base::Bind(&GetArcSystemProcessList),
                                    callback);
 }
 
 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));
   return true;
 }
 
 void ArcProcessService::OnReceiveProcessList(
     const RequestProcessListCallback& callback,
     std::vector<mojom::RunningAppProcessInfoPtr> instance_processes) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   base::PostTaskAndReplyWithResult(
       GetTaskRunner().get(), FROM_HERE,
       base::Bind(&UpdateAndReturnProcessList, nspid_to_pid_,
                  base::Passed(&instance_processes)),
       callback);
 }
 
 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() {}
 
 }  // namespace arc