Simplify PrinterDetector Observer APIs.

This cl does several things.

First, It removes the PrinterDetector::Observer::OnPrinterScanComplete
API hook.  That hook exists because we expected zeroconf printer
detection to take a non-trivial amount of time and we wanted to be
able to enable some UI indication that we were still looking for
printers until the scan was complete.  However, it turns out that
mdns/dns-sd traffic is all cached anyways by the ServiceDiscovery,
so we get initial printers back for zeroconf almost instantly, making
this hook pointless.

Second, the Observer API is changed so that it no longer guarantees an
OnPrintersFound callback to the caller after registration; instead the
client observing printer change events is responsible for calling
PrinterDetector::GetPrinters() to do the initial population of the
printer list.  This removes a lot of weird hacks to get thread safety
right.

This change also removes CupsPrintersManager::Start() and
PrinterDetector::StartObservers(), as these are no longer needed to
make startup safe.  These calls were added out of concerns that we
would encounter something like:

(in class Foo's constructor):
my_detector_->AddObserver(this);
   ->OBSERVER CALLBACK HAPPENS BEFORE Foo::Foo() IS FINISHED<-
	 ...rest of Foo's constructor...

However, this isn't a real problem for the API AFAICT.  Since the
observer callbacks are on the same sequence as the constructor, they
can't start until after the constructor finishes, meaning this is a
non-issue.

I think.  I would appreciate reviewers taking a bit of time to think
this through and see if they also believe this is now safe without the
Start() calls, as concurrency is sometimes hard to reason about.

This cl also removes some tests around StartObservers() that are no
longer relevant with the API changes.

Bug: 754834
Change-Id: Ia2e4c34b82ddf99a7551756c2bc037889f52df5b
Reviewed-on: https://chromium-review.googlesource.com/871660
Commit-Queue: Justin Carlson <justincarlson@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531673}

chrome/browser/chromeos/BUILD.gn

@@ -1962,8 +1962,6 @@ source_set("unit_tests") {
     "printing/printers_sync_bridge_unittest.cc",
     "printing/specifics_translation_unittest.cc",
     "printing/synced_printers_manager_unittest.cc",
-    "printing/usb_printer_detector_unittest.cc",
-    "printing/zeroconf_printer_detector_unittest.cc",
     "profiles/profile_list_chromeos_unittest.cc",
     "proxy_config_service_impl_unittest.cc",
     "resource_reporter/resource_reporter_unittest.cc",

chrome/browser/chromeos/printing/cups_printers_manager.cc

@@ -31,28 +31,35 @@ class CupsPrintersManagerImpl;
 // Since CupsPrintersManager listens to multiple PrinterDetectors, we need to
 // disambiguate incoming observer calls based on their source, and so can't
 // implement PrinterDetector::Observer directly in CupsPrintersManagerImpl.
+//
+// Note that at the time the Proxy is constructed, CupsPrintersManagerImpl's
+// construction may not be complete, so any callbacks into the parent need
+// to be deferred.
 class PrinterDetectorObserverProxy : public PrinterDetector::Observer {
  public:
   PrinterDetectorObserverProxy(CupsPrintersManagerImpl* parent,
                                int id,
                                PrinterDetector* detector)
       : parent_(parent), id_(id), observer_(this) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
+    // It's ok to Add() before construction is complete because callbacks are on
+    // the same sequence, therefore we will complete construction before any
+    // detection callback will be processed.
     observer_.Add(detector);
   }
-  ~PrinterDetectorObserverProxy() override = default;
+  ~PrinterDetectorObserverProxy() override {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
+  }
 
   // Defined out of line because we need the CupsPrintersManagerImpl
   // definition first.
   void OnPrintersFound(
       const std::vector<PrinterDetector::DetectedPrinter>& printers) override;
 
-  // We don't do anything with OnPrinterScanComplete() at the moment, so just
-  // stub it out.
-  void OnPrinterScanComplete() override {}
-
  private:
   CupsPrintersManagerImpl* parent_;
   int id_;
+  SEQUENCE_CHECKER(sequence_);
   ScopedObserver<PrinterDetector, PrinterDetector::Observer> observer_;
 };
 
@@ -87,7 +94,6 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
                           PrinterEventTracker* event_tracker)
       : synced_printers_manager_(synced_printers_manager),
         synced_printers_manager_observer_(this),
-        started_observing_(false),
         usb_detector_(std::move(usb_detector)),
         zeroconf_detector_(std::move(zeroconf_detector)),
         ppd_provider_(std::move(ppd_provider)),
@@ -105,9 +111,12 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
     usb_detector_observer_proxy_ =
         std::make_unique<PrinterDetectorObserverProxy>(this, kUsbDetector,
                                                        usb_detector_.get());
+    OnPrintersFound(kUsbDetector, usb_detector_->GetPrinters());
+
     zeroconf_detector_observer_proxy_ =
         std::make_unique<PrinterDetectorObserverProxy>(
             this, kZeroconfDetector, zeroconf_detector_.get());
+    OnPrintersFound(kZeroconfDetector, zeroconf_detector_->GetPrinters());
   }
 
   ~CupsPrintersManagerImpl() override = default;
@@ -161,15 +170,6 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
     observer_list_.RemoveObserver(observer);
   }
 
-  // Used to initiate the callbacks after the observers have been constructed.
-  void Start() override {
-    if (!started_observing_) {
-      usb_detector_->StartObservers();
-      zeroconf_detector_->StartObservers();
-      started_observing_ = true;
-    }
-  }
-
   // Public API function.
   void PrinterInstalled(const Printer& printer) override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
@@ -469,12 +469,6 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
   ScopedObserver<SyncedPrintersManager, SyncedPrintersManager::Observer>
       synced_printers_manager_observer_;
 
-  // Represents whether or not the Start() method has been called and
-  // CupsPrintersManager has started to observe the usb and zeroconf printer
-  // detectors. If this is true than subsequent calls to Start() will have no
-  // effect.
-  bool started_observing_;
-
   std::unique_ptr<PrinterDetector> usb_detector_;
   std::unique_ptr<PrinterDetectorObserverProxy> usb_detector_observer_proxy_;
 
@@ -516,6 +510,7 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
 
 void PrinterDetectorObserverProxy::OnPrintersFound(
     const std::vector<PrinterDetector::DetectedPrinter>& printers) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
   parent_->OnPrintersFound(id_, printers);
 }
 

chrome/browser/chromeos/printing/cups_printers_manager.h

@@ -78,16 +78,12 @@ class CupsPrintersManager {
   // the printer_id is not that of a configured printer.
   virtual void RemoveConfiguredPrinter(const std::string& printer_id) = 0;
 
-  // Add or remove observers.
+  // Add or remove observers.  Observers do not need to be on the same
+  // sequence as the CupsPrintersManager.  Callbacks for a given observer
+  // will be on the same sequence as was used to call AddObserver().
   virtual void AddObserver(Observer* observer) = 0;
   virtual void RemoveObserver(Observer* observer) = 0;
 
-  // Allows CupsPrinterManager to begin receiving callbacks from the
-  // PrinterDetectors that it is observing. This method is meant to be called
-  // only once after CupsPrintersManager has been added as an observer to the
-  // appropriate PrinterDetector objects.
-  virtual void Start() = 0;
-
   // Record that the given printers has been installed in CUPS for usage.  If
   // |printer| is not a configured or enterprise printer, this will have the
   // side effect of moving |printer| into the configured class.

chrome/browser/chromeos/printing/cups_printers_manager_unittest.cc

@@ -157,7 +157,7 @@ class FakeSyncedPrintersManager : public SyncedPrintersManager {
 
 class FakePrinterDetector : public PrinterDetector {
  public:
-  FakePrinterDetector() : start_observers_calls_(0) {}
+  FakePrinterDetector() {}
   ~FakePrinterDetector() override = default;
 
   void AddObserver(Observer* observer) override {
@@ -167,14 +167,6 @@ class FakePrinterDetector : public PrinterDetector {
     observers_.RemoveObserver(observer);
   }
 
-  void StartObservers() override {
-    ++start_observers_calls_;
-  }
-
-  int StartObserversCalls() const {
-    return start_observers_calls_;
-  }
-
   std::vector<DetectedPrinter> GetPrinters() override { return detections_; }
 
   void AddDetections(
@@ -200,7 +192,6 @@ class FakePrinterDetector : public PrinterDetector {
  private:
   std::vector<DetectedPrinter> detections_;
   base::ObserverList<PrinterDetector::Observer> observers_;
-  int start_observers_calls_;
 };
 
 // Fake PpdProvider backend.  This fake generates PpdReferences based on
@@ -483,22 +474,5 @@ TEST_F(CupsPrintersManagerTest, GetPrinter) {
   EXPECT_EQ(printer, nullptr);
 }
 
-// Test that the Start() method in CupsPrinterManager will only execute
-// a single time. In order to test this we check to see that the
-// StartObservers() method in each of the printer detectors has only been called
-// a single time even after calling Start() in CupsPrintersManager multiple
-// times.
-TEST_F(CupsPrintersManagerTest, StartObservingCalledOnce) {
-  manager_->Start();
-  scoped_task_environment_.RunUntilIdle();
-  EXPECT_EQ(1, usb_detector_->StartObserversCalls());
-  EXPECT_EQ(1, zeroconf_detector_->StartObserversCalls());
-
-  manager_->Start();
-  scoped_task_environment_.RunUntilIdle();
-  EXPECT_EQ(1, usb_detector_->StartObserversCalls());
-  EXPECT_EQ(1, zeroconf_detector_->StartObserversCalls());
-}
-
 }  // namespace
 }  // namespace chromeos

chrome/browser/chromeos/printing/printer_detector.h

@@ -44,26 +44,22 @@ class CHROMEOS_EXPORT PrinterDetector {
     // Called with a collection of printers as they are discovered.  On each
     // call |printers| is the full set of known printers; it is not
     // incremental; printers may be added or removed.
+    //
+    // To avoid race conditions of printer state changes, you should register
+    // your Observer and call PrinterDetector::GetPrinters() to populate the
+    // initial state in the same sequenced atom.
     virtual void OnPrintersFound(
         const std::vector<DetectedPrinter>& printers) = 0;
-
-    // Called when we are done with the initial scan for printers.  We may
-    // still call OnPrintersFound if the set of available printers
-    // changes, but the user can conclude that if a printer is currently
-    // available and not in the list, we're not still looking for it.
-    virtual void OnPrinterScanComplete() = 0;
   };
 
   virtual ~PrinterDetector() = default;
 
   // Observer management.  Observer callbacks will be performed on the calling
-  // sequence.
+  // sequence, but observers do not need to be on the same sequence as each
+  // other or the detector.
   virtual void AddObserver(Observer* observer) = 0;
   virtual void RemoveObserver(Observer* observer) = 0;
 
-  // Begins to issue the Notify callbacks for the attached observers.
-  virtual void StartObservers() = 0;
-
   // Get the current list of known printers.
   virtual std::vector<DetectedPrinter> GetPrinters() = 0;
 };

chrome/browser/chromeos/printing/printer_detector_test_util.h

-// Copyright 2017 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.
-
-#ifndef CHROME_BROWSER_CHROMEOS_PRINTING_PRINTER_DETECTOR_TEST_UTIL_H_
-#define CHROME_BROWSER_CHROMEOS_PRINTING_PRINTER_DETECTOR_TEST_UTIL_H_
-
-#include "chrome/browser/chromeos/printing/printer_detector.h"
-#include "content/public/test/test_browser_thread_bundle.h"
-#include "content/public/test/test_utils.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace chromeos {
-namespace {
-
-// Fake PrinterDetector::Observer which is used to test the PrinterDetector
-// class.
-class FakePrinterDetectorObserver : public PrinterDetector::Observer {
- public:
-  FakePrinterDetectorObserver() : on_printers_found_calls_(0) {}
-
-  void OnPrintersFound(
-      const std::vector<PrinterDetector::DetectedPrinter>& printers) override;
-
-  void OnPrinterScanComplete() override {}
-
-  int OnPrintersFoundCalls() const { return on_printers_found_calls_; }
-
- private:
-  int on_printers_found_calls_;
-};
-
-void FakePrinterDetectorObserver::OnPrintersFound(
-    const std::vector<PrinterDetector::DetectedPrinter>& printers) {
-  ++on_printers_found_calls_;
-}
-
-class PrinterDetectorTest : public testing::Test {
- public:
-  PrinterDetectorTest() {}
-
-  ~PrinterDetectorTest() override {}
-
-  void TestStartObservers(PrinterDetector* printer_detector) {
-    FakePrinterDetectorObserver fake_printer_detector_observer;
-    printer_detector->AddObserver(&fake_printer_detector_observer);
-    EXPECT_EQ(0, fake_printer_detector_observer.OnPrintersFoundCalls());
-
-    printer_detector->StartObservers();
-    content::RunAllPendingInMessageLoop();
-
-    EXPECT_EQ(1, fake_printer_detector_observer.OnPrintersFoundCalls());
-
-    printer_detector->RemoveObserver(&fake_printer_detector_observer);
-  }
-
- protected:
-  content::TestBrowserThreadBundle test_browser_thread_bundle_;
-};
-
-}  // namespace
-}  // namespace chromeos
-
-#endif  // CHROME_BROWSER_CHROMEOS_PRINTING_PRINTER_DETECTOR_TEST_UTIL_H_

chrome/browser/chromeos/printing/usb_printer_detector.cc

@@ -52,7 +52,7 @@ std::string GuessEffectiveMakeAndModel(const device::UsbDevice& device) {
 class UsbPrinterDetectorImpl : public UsbPrinterDetector,
                                public device::UsbService::Observer {
  public:
-  UsbPrinterDetectorImpl(device::UsbService* usb_service)
+  explicit UsbPrinterDetectorImpl(device::UsbService* usb_service)
       : usb_observer_(this),
         observer_list_(
             new base::ObserverListThreadSafe<UsbPrinterDetector::Observer>),
@@ -75,29 +75,6 @@ class UsbPrinterDetectorImpl : public UsbPrinterDetector,
     observer_list_->RemoveObserver(observer);
   }
 
-  void StartObservers() override {
-    // This is subtle.  We're scheduling a callback here, so we have to be
-    // careful about cancellation.  We can't just post the task to the sequence,
-    // because there's the possibility the observer will be deleted before the
-    // task executes.  We can't call the callbacks directly here because the
-    // common usage of observers involves having the observer object not be
-    // fully constructed when AddObserver is called.
-    //
-    // The easiest way to deal with this is to just let ThreadSafeObserverList
-    // sort it out.  It already handles the removed-before-callback-executes
-    // case, so makes things safe for us.  This means we may *over*-notify a
-    // bit, but since the number of observers is expected to be small (usually
-    // just 1) that's a price we're willing to pay.
-    observer_list_->Notify(
-        FROM_HERE, &PrinterDetector::Observer::OnPrintersFound, GetPrinters());
-
-    // In the case of the USB detector, we expect to always have up-to-date
-    // information, thus the scan is complete after the first OnPrintersFound
-    // callback.
-    observer_list_->Notify(FROM_HERE,
-                           &PrinterDetector::Observer::OnPrinterScanComplete);
-  }
-
   // PrinterDetector interface function.
   std::vector<DetectedPrinter> GetPrinters() override {
     base::AutoLock auto_lock(printers_lock_);

chrome/browser/chromeos/printing/usb_printer_detector_unittest.cc

-// Copyright 2017 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 "chrome/browser/chromeos/printing/printer_detector.h"
-#include "chrome/browser/chromeos/printing/printer_detector_test_util.h"
-#include "chrome/browser/chromeos/printing/usb_printer_detector.h"
-#include "device/usb/mock_usb_service.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace chromeos {
-namespace {
-
-class UsbPrinterDetectorTest : public PrinterDetectorTest {};
-
-TEST_F(UsbPrinterDetectorTest, UsbPrinterDetectorStartObservers) {
-  device::MockUsbService mock_usb_service;
-  std::unique_ptr<PrinterDetector> usb_printer_detector =
-      UsbPrinterDetector::CreateForTesting(&mock_usb_service);
-
-  TestStartObservers(usb_printer_detector.get());
-}
-
-}  // namespace
-}  // namespace chromeos

chrome/browser/chromeos/printing/zeroconf_printer_detector.cc

@@ -245,18 +245,6 @@ class ZeroconfPrinterDetectorImpl
     observer_list_->RemoveObserver(observer);
   }
 
-  // Schedules callbacks for the observers using ThreadSafeObserverList. This
-  // means that the callbacks are posted for later execution and not executed
-  // immediately.
-  void StartObservers() override {
-    observer_list_->Notify(
-        FROM_HERE, &PrinterDetector::Observer::OnPrintersFound, GetPrinters());
-    // TODO(justincarlson) - Figure out a more intelligent way to figure out
-    // when a scan is reasonably "done".
-    observer_list_->Notify(FROM_HERE,
-                           &PrinterDetector::Observer::OnPrinterScanComplete);
-  }
-
   // ServiceDiscoveryDeviceLister::Delegate implementation
   void OnDeviceChanged(bool added,
                        const ServiceDescription& service_description) override {

chrome/browser/chromeos/printing/zeroconf_printer_detector_unittest.cc

-// Copyright 2017 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 "chrome/browser/chromeos/printing/printer_detector.h"
-#include "chrome/browser/chromeos/printing/printer_detector_test_util.h"
-#include "chrome/browser/chromeos/printing/zeroconf_printer_detector.h"
-#include "chrome/test/base/testing_profile.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace chromeos {
-namespace {
-
-class ZeroconfPrinterDetectorTest : public PrinterDetectorTest {};
-
-// Flaky on ASAN. See http://crbug.com/777236.
-#if defined(ADDRESS_SANITIZER)
-#define MAYBE_ZeroconfPrinterDetectorStartObservers \
-  DISABLED_ZeroconfPrinterDetectorStartObservers
-#else
-#define MAYBE_ZeroconfPrinterDetectorStartObservers \
-  ZeroconfPrinterDetectorStartObservers
-#endif
-TEST_F(ZeroconfPrinterDetectorTest,
-       MAYBE_ZeroconfPrinterDetectorStartObservers) {
-  TestingProfile testing_profile;
-  std::unique_ptr<PrinterDetector> zeroconf_printer_detector =
-      ZeroconfPrinterDetector::Create(&testing_profile);
-
-  TestStartObservers(zeroconf_printer_detector.get());
-}
-
-}  // namespace
-}  // namespace chromeos

chrome/browser/ui/webui/print_preview/local_printer_handler_chromeos.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/webui/print_preview/local_printer_handler_chromeos.h"
 
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "base/bind_helpers.h"
@@ -78,8 +79,6 @@ LocalPrinterHandlerChromeos::LocalPrinterHandlerChromeos(
       printers_manager_(CupsPrintersManager::Create(profile)),
       printer_configurer_(chromeos::PrinterConfigurer::Create(profile)),
       weak_factory_(this) {
-  printers_manager_->Start();
-
   // Construct the CupsPrintJobManager to listen for printing events.
   chromeos::CupsPrintJobManagerFactory::GetForBrowserContext(profile);
 }

chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc

@@ -300,7 +300,6 @@ void CupsPrintersHandler::RegisterMessages() {
 
 void CupsPrintersHandler::OnJavascriptAllowed() {
   printers_manager_->AddObserver(this);
-  printers_manager_->Start();
 }
 
 void CupsPrintersHandler::OnJavascriptDisallowed() {