Fix AutotestPrivateApiTest.AutotestPrivate

- Fix the AutotestPrivateGetPrinterListFunction threading issue;
- Update assistant disallowed state value from
    DISALLOWED_BY_ACCOUNT_TYPE (8) to
    DISALLOWED_BY_NONPRIMARY_USER (3)
  after https://crrev.com/c/2091649;
- Fix flaky acceleratorTest by adding a short delay before verifying
  a browser window is closed on accelerator Ctrl+W.
- Remove histogram tests because it is moved to metricsPrivate api
  in https://crrev.com/c/2108219;

Bug: 1032993
Change-Id: Icb53d6cbacf239ed1d1ccf6e2d29947534c98050
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2132683Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755879}

chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc

@@ -117,6 +117,7 @@
 #include "components/policy/core/browser/policy_conversions.h"
 #include "components/policy/core/common/policy_service.h"
 #include "components/user_manager/user_manager.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/tracing_controller.h"
 #include "content/public/browser/web_contents.h"
@@ -1970,10 +1971,13 @@ AutotestPrivateGetPrinterListFunction::AutotestPrivateGetPrinterListFunction()
 
 AutotestPrivateGetPrinterListFunction::
     ~AutotestPrivateGetPrinterListFunction() {
-  printers_manager_->RemoveObserver(this);
+  DCHECK(!printers_manager_);
 }
 
 ExtensionFunction::ResponseAction AutotestPrivateGetPrinterListFunction::Run() {
+  // |printers_manager_| should be created on UI thread.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   DVLOG(1) << "AutotestPrivateGetPrinterListFunction";
 
   Profile* profile = Profile::FromBrowserContext(browser_context());
@@ -1990,21 +1994,43 @@ ExtensionFunction::ResponseAction AutotestPrivateGetPrinterListFunction::Run() {
   return RespondLater();
 }
 
+void AutotestPrivateGetPrinterListFunction::DestroyPrintersManager() {
+  // |printers_manager_| should be destroyed on UI thread.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (!printers_manager_)
+    return;
+
+  printers_manager_->RemoveObserver(this);
+  printers_manager_.reset();
+}
+
 void AutotestPrivateGetPrinterListFunction::RespondWithTimeoutError() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   if (did_respond())
     return;
+
+  DestroyPrintersManager();
   Respond(
       Error("Timeout occurred before Enterprise printers were initialized"));
 }
 
 void AutotestPrivateGetPrinterListFunction::RespondWithSuccess() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   if (did_respond())
     return;
-  Respond(OneArgument(std::move(results_)));
+
   timeout_timer_.AbandonAndStop();
+  DestroyPrintersManager();
+  Respond(OneArgument(std::move(results_)));
 }
 
 void AutotestPrivateGetPrinterListFunction::OnEnterprisePrintersInitialized() {
+  // |printers_manager_| should call this on UI thread.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   constexpr PrinterClass kClassesToFetch[] = {
       PrinterClass::kEnterprise,
       PrinterClass::kSaved,
@@ -2023,9 +2049,10 @@ void AutotestPrivateGetPrinterListFunction::OnEnterprisePrintersInitialized() {
       results_->Append(std::move(result));
     }
   }
-  // We have to respond in separate task, because it will cause a destruction of
-  // CupsPrintersManager
-  PostTask(
+  // We have to respond in separate task on the same thread, because it will
+  // cause a destruction of CupsPrintersManager which needs to happen after
+  // we return and on the same thread.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&AutotestPrivateGetPrinterListFunction::RespondWithSuccess,
                      this));

chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.h

@@ -459,10 +459,14 @@ class AutotestPrivateGetPrinterListFunction
  private:
   ~AutotestPrivateGetPrinterListFunction() override;
   ResponseAction Run() override;
+
+  void DestroyPrintersManager();
   void RespondWithTimeoutError();
   void RespondWithSuccess();
+
   // chromeos::CupsPrintersManager::Observer
   void OnEnterprisePrintersInitialized() override;
+
   std::unique_ptr<base::Value> results_;
   std::unique_ptr<chromeos::CupsPrintersManager> printers_manager_;
   base::OneShotTimer timeout_timer_;

chrome/test/data/extensions/api_test/autotest_private/test.js

@@ -145,26 +145,6 @@ var defaultTests = [
       });
     });
   },
-  function getHistogramExists() {
-    // Request an arbitrary histogram that is reported once at startup and seems
-    // unlikely to go away.
-    chrome.autotestPrivate.getHistogram(
-        "Startup.BrowserProcessImpl_PreMainMessageLoopRunTime",
-        chrome.test.callbackPass(function(histogram) {
-          chrome.test.assertEq(typeof histogram, 'object');
-          chrome.test.assertEq(histogram.buckets.length, 1);
-          chrome.test.assertEq(histogram.buckets[0].count, 1);
-          chrome.test.assertTrue(histogram.sum <= histogram.buckets[0].max);
-          chrome.test.assertTrue(histogram.sum >= histogram.buckets[0].min);
-          chrome.test.assertTrue(
-              histogram.buckets[0].max > histogram.buckets[0].min);
-        }));
-  },
-  function getHistogramMissing() {
-    chrome.autotestPrivate.getHistogram(
-        'Foo.Nonexistent',
-        chrome.test.callbackFail('Histogram Foo.Nonexistent not found'));
-  },
   // This test verifies that Play Store window is not shown by default but
   // Chrome is shown.
   function isAppShown() {
@@ -244,29 +224,33 @@ var defaultTests = [
       chrome.test.succeed();
     });
   },
+  // The state dumped in Assistant API error message is AssistantAllowedState
+  // defined in ash/public/mojom/assistant_state_controller.mojom.
+  // 3 is DISALLOWED_BY_NONPRIMARY_USER from IsAssistantAllowedForProfile when
+  // running under test without setting up a primary account.
   function setAssistantEnabled() {
     chrome.autotestPrivate.setAssistantEnabled(true, 1000 /* timeout_ms */,
         chrome.test.callbackFail(
-            'Assistant not allowed - state: 8'));
+            'Assistant not allowed - state: 3'));
   },
   function sendAssistantTextQuery() {
     chrome.autotestPrivate.sendAssistantTextQuery(
         'what time is it?' /* query */,
         1000 /* timeout_ms */,
         chrome.test.callbackFail(
-            'Assistant not allowed - state: 8'));
+            'Assistant not allowed - state: 3'));
   },
   function waitForAssistantQueryStatus() {
     chrome.autotestPrivate.waitForAssistantQueryStatus(10 /* timeout_s */,
         chrome.test.callbackFail(
-            'Assistant not allowed - state: 8'));
+            'Assistant not allowed - state: 3'));
   },
   function setWhitelistedPref() {
     chrome.autotestPrivate.setWhitelistedPref(
         'settings.voice_interaction.hotword.enabled' /* pref_name */,
         true /* value */,
         chrome.test.callbackFail(
-            'Assistant not allowed - state: 8'));
+            'Assistant not allowed - state: 3'));
   },
   // This test verifies that getArcState returns provisioned False in case ARC
   // is not provisioned by default.
@@ -749,11 +733,19 @@ var defaultTests = [
                 closeWindow,
                 function(success) {
                   chrome.test.assertTrue(success);
-                  chrome.autotestPrivate.getAppWindowList(function(list) {
-                    chrome.test.assertEq(1, list.length);
-                    chrome.test.assertNoLastError();
-                    chrome.test.succeed();
-                  });
+                  // Actual window close might happen sometime later after the
+                  // accelerator. So keep trying until window count drops to 1.
+                  var timer = window.setInterval(() => {
+                    chrome.autotestPrivate.getAppWindowList(function(list) {
+                      chrome.test.assertNoLastError();
+
+                      if (list.length != 1)
+                        return;
+
+                      window.clearInterval(timer);
+                      chrome.test.succeed();
+                    });
+                  }, 100);
                 });
           });
         });