Revert "Replace AppBannerManager::need_to_log_status_ with StatusReporter."

This reverts commit 05b21c67.

Reason for revert: This is causing crashes; see crbug/763891.

The crash implies that AppBannerManagerAndroid::ShowBannerUi() is being called without a preceding call to AppBannerManager::RequestAppBanner() or after a call to AppBannerManager::Stop().

Perhaps Stop is being called due to the webcontents being destroyed, and ShowBannerUi is being called after that. I'm not sure whether this is plausible. Reverting until I understand this better.

Original change's description:
> Replace AppBannerManager::need_to_log_status_ with StatusReporter.
>
> AppBannerManager treats need_to_log_status_ differently (in both
> RequestAppBanner and ReportStatus) depending on the return value of
> IsDebugMode(). This makes it harder to reason about the valid states for
> need_to_log_status_. This CL replaces need_to_log_status_ with a
> StatusReporter, which has an implemenation that is decided by consulting
> IsDebugMode() once in RequestAppBanner.
>
> Change-Id: I862fd76c9732cfcda0d5bf787a51161ef4fbfb66
> Reviewed-on: https://chromium-review.googlesource.com/630898
> Commit-Queue: Michael McGreevy <mcgreevy@chromium.org>
> Reviewed-by: Ben Wells <benwells@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#500199}

TBR=benwells@chromium.org,mcgreevy@chromium.org

Bug: 763891
Change-Id: Ia5a3dc666a2c27535aacc1abf2ea46f71c9eb94b
Reviewed-on: https://chromium-review.googlesource.com/665639Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Michael McGreevy <mcgreevy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502177}

chrome/browser/banners/app_banner_manager.cc

@@ -56,57 +56,6 @@ void AppBannerManager::SetTotalEngagementToTrigger(double engagement) {
   AppBannerSettingsHelper::SetTotalEngagementToTrigger(engagement);
 }
 
-}  // namespace banners
-
-namespace {
-
-// Returns a string parameter for a devtools console message corresponding to
-// |code|. Returns the empty string if |code| requires no parameter.
-std::string GetStatusParam(InstallableStatusCode code) {
-  if (code == NO_ACCEPTABLE_ICON || code == MANIFEST_MISSING_SUITABLE_ICON) {
-    return base::IntToString(InstallableManager::GetMinimumIconSizeInPx());
-  }
-
-  return std::string();
-}
-
-// Logs installable status codes to the console.
-class ConsoleStatusReporter : public banners::AppBannerManager::StatusReporter {
- public:
-  bool Waiting() const override { return false; }
-
-  // Logs an error message corresponding to |code| to the devtools console
-  // attached to |web_contents|.
-  void ReportStatus(content::WebContents* web_contents,
-                    InstallableStatusCode code) override {
-    LogErrorToConsole(web_contents, code, GetStatusParam(code));
-  }
-};
-
-// Tracks installable status codes via a UMA histogram.
-class TrackingStatusReporter
-    : public banners::AppBannerManager::StatusReporter {
- public:
-  TrackingStatusReporter() : done_(false) {}
-  bool Waiting() const override { return !done_; }
-
-  // Records code via a UMA histogram.
-  void ReportStatus(content::WebContents* web_contents,
-                    InstallableStatusCode code) override {
-    // Ensure that we haven't yet logged a status code for this page.
-    DCHECK(!done_);
-    banners::TrackInstallableStatusCode(code);
-    done_ = true;
-  }
-
- private:
-  bool done_;
-};
-
-}  // anonymous namespace
-
-namespace banners {
-
 void AppBannerManager::RequestAppBanner(const GURL& validated_url,
                                         bool is_debug_mode) {
   content::WebContents* contents = web_contents();
@@ -121,12 +70,10 @@ void AppBannerManager::RequestAppBanner(const GURL& validated_url,
   UpdateState(State::ACTIVE);
   triggered_by_devtools_ = is_debug_mode;
 
-  // We only need to use TrackingStatusReporter if we aren't in debug mode
-  // (this avoids skew from testing).
-  status_reporter_.reset(
-      IsDebugMode()
-          ? static_cast<StatusReporter*>(new ConsoleStatusReporter())
-          : static_cast<StatusReporter*>(new TrackingStatusReporter()));
+  // We only need to call ReportStatus if we aren't in debug mode (this avoids
+  // skew from testing).
+  DCHECK(!need_to_log_status_);
+  need_to_log_status_ = !IsDebugMode();
 
   // Exit if this is an incognito window, non-main frame, or insecure context.
   InstallableStatusCode code = NO_ERROR_DETECTED;
@@ -190,6 +137,7 @@ AppBannerManager::AppBannerManager(content::WebContents* web_contents)
       has_sufficient_engagement_(false),
       load_finished_(false),
       triggered_by_devtools_(false),
+      need_to_log_status_(false),
       weak_factory_(this) {
   DCHECK(manager_);
 
@@ -207,6 +155,14 @@ std::string AppBannerManager::GetBannerType() {
   return "web";
 }
 
+std::string AppBannerManager::GetStatusParam(InstallableStatusCode code) {
+  if (code == NO_ACCEPTABLE_ICON || code == MANIFEST_MISSING_SUITABLE_ICON) {
+    return base::IntToString(InstallableManager::GetMinimumIconSizeInPx());
+  }
+
+  return std::string();
+}
+
 int AppBannerManager::GetIdealPrimaryIconSizeInPx() {
   return InstallableManager::GetMinimumIconSizeInPx();
 }
@@ -318,8 +274,14 @@ void AppBannerManager::RecordDidShowBanner(const std::string& event_name) {
 
 void AppBannerManager::ReportStatus(content::WebContents* web_contents,
                                     InstallableStatusCode code) {
-  DCHECK(status_reporter_);
-  status_reporter_->ReportStatus(web_contents, code);
+  if (IsDebugMode()) {
+    LogErrorToConsole(web_contents, code, GetStatusParam(code));
+  } else {
+    // Ensure that we haven't yet logged a status code for this page.
+    DCHECK(need_to_log_status_);
+    TrackInstallableStatusCode(code);
+    need_to_log_status_ = false;
+  }
 }
 
 void AppBannerManager::ResetCurrentPageData() {
@@ -363,18 +325,16 @@ void AppBannerManager::Stop() {
     ReportStatus(web_contents(), code);
 
   // In every non-debug run through the banner pipeline, we should have called
-  // ReportStatus(). The only case where we don't is if we're still running and
-  // aren't blocked on the network. When running and blocked on the network the
-  // state should be logged.
-  // If the pipeline has not been started, status_reporter will be null.
+  // ReportStatus() and set need_to_log_status_ to false. The only case where
+  // we don't is if we're still running and aren't blocked on the network. When
+  // running and blocked on the network the state should be logged.
   // TODO(dominickn): log when the pipeline is fetching native app banner
   // details.
-  DCHECK(!status_reporter_ || !status_reporter_->Waiting() ||
-         (IsRunning() && !IsWaitingForData()));
+  DCHECK(!need_to_log_status_ || (IsRunning() && !IsWaitingForData()));
 
   ResetBindings();
   UpdateState(State::COMPLETE);
-  status_reporter_.reset(nullptr);
+  need_to_log_status_ = false;
   has_sufficient_engagement_ = false;
 }
 

chrome/browser/banners/app_banner_manager.h

@@ -5,7 +5,6 @@
 #ifndef CHROME_BROWSER_BANNERS_APP_BANNER_MANAGER_H_
 #define CHROME_BROWSER_BANNERS_APP_BANNER_MANAGER_H_
 
-#include <memory>
 #include <vector>
 
 #include "base/macros.h"
@@ -145,6 +144,10 @@ class AppBannerManager : public content::WebContentsObserver,
   // alerting websites that a banner is about to be created.
   virtual std::string GetBannerType();
 
+  // Returns a string parameter for a devtools console message corresponding to
+  // |code|. Returns the empty string if |code| requires no parameter.
+  std::string GetStatusParam(InstallableStatusCode code);
+
   // Returns the ideal and minimum primary icon size requirements.
   virtual int GetIdealPrimaryIconSizeInPx();
   virtual int GetMinimumPrimaryIconSizeInPx();
@@ -181,7 +184,8 @@ class AppBannerManager : public content::WebContentsObserver,
   // metric being recorded.
   void RecordDidShowBanner(const std::string& event_name);
 
-  // Reports |code| via a UMA histogram or logs it to the console.
+  // Logs an error message corresponding to |code| to the devtools console
+  // attached to |web_contents|. Does nothing if IsDebugMode() returns false.
   void ReportStatus(content::WebContents* web_contents,
                     InstallableStatusCode code);
 
@@ -298,23 +302,8 @@ class AppBannerManager : public content::WebContentsObserver,
   // Whether the current flow was begun via devtools.
   bool triggered_by_devtools_;
 
- public:
-  // A StatusReporter handles the reporting of |InstallableStatusCode|s.
-  class StatusReporter {
-   public:
-    virtual ~StatusReporter() {}
-
-    // Reports whether the StatusReporter is waiting for ReportStatus to be
-    // called.
-    virtual bool Waiting() const = 0;
-
-    // Reports |code| (via a mechanism which depends on the implementation).
-    virtual void ReportStatus(content::WebContents* web_contents,
-                              InstallableStatusCode code) = 0;
-  };
-
- private:
-  std::unique_ptr<StatusReporter> status_reporter_;
+  // Whether the installable status has been logged for this run.
+  bool need_to_log_status_;
 
   // The concrete subclasses of this class are expected to have their lifetimes
   // scoped to the WebContents which they are observing. This allows us to use

chrome/browser/banners/app_banner_manager_browsertest.cc

@@ -75,9 +75,7 @@ class AppBannerManagerTest : public AppBannerManager {
 
   State state() { return AppBannerManager::state(); }
 
-  bool waiting_to_log_status() {
-    return status_reporter_ && status_reporter_->Waiting();
-  }
+  bool need_to_log_status() { return need_to_log_status_; }
 
   void Prepare(base::Closure quit_closure) {
     quit_closure_ = quit_closure;
@@ -241,7 +239,7 @@ class AppBannerManagerBrowserTest : public InProcessBrowserTest {
                                   (manager->will_show() ? 1 : 0));
       histograms.ExpectUniqueSample(banners::kInstallableStatusCodeHistogram,
                                     expected_code_for_histogram, 1);
-      EXPECT_FALSE(manager->waiting_to_log_status());
+      EXPECT_FALSE(manager->need_to_log_status());
     }
   }
 
@@ -249,7 +247,7 @@ class AppBannerManagerBrowserTest : public InProcessBrowserTest {
                                        AppBannerManagerTest* manager,
                                        const GURL& url,
                                        bool expected_will_show,
-                                       bool expected_waiting_to_log_status,
+                                       bool expected_need_to_log_status,
                                        State expected_state) {
     // Use NavigateToURLWithDisposition as it isn't overloaded, so can be used
     // with Bind.
@@ -258,14 +256,14 @@ class AppBannerManagerBrowserTest : public InProcessBrowserTest {
         base::BindOnce(&ui_test_utils::NavigateToURLWithDisposition, browser,
                        url, WindowOpenDisposition::CURRENT_TAB,
                        ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION),
-        expected_will_show, expected_waiting_to_log_status, expected_state);
+        expected_will_show, expected_need_to_log_status, expected_state);
   }
 
   void TriggerBannerFlow(Browser* browser,
                          AppBannerManagerTest* manager,
                          base::OnceClosure trigger_task,
                          bool expected_will_show,
-                         bool expected_waiting_to_log_status,
+                         bool expected_need_to_log_status,
                          State expected_state) {
     base::RunLoop run_loop;
     manager->clear_will_show();
@@ -274,7 +272,7 @@ class AppBannerManagerBrowserTest : public InProcessBrowserTest {
     run_loop.Run();
 
     EXPECT_EQ(expected_will_show, manager->will_show());
-    EXPECT_EQ(expected_waiting_to_log_status, manager->waiting_to_log_status());
+    EXPECT_EQ(expected_need_to_log_status, manager->need_to_log_status());
     EXPECT_EQ(expected_state, manager->state());
   }
 };
@@ -510,7 +508,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
   // state, without showing a banner.
   TriggerBannerFlowWithNavigation(
       browser(), manager.get(), test_url, false /* expected_will_show */,
-      true /* expected_waiting_to_log_status */, State::PENDING_ENGAGEMENT);
+      true /* expected_need_to_log_status */, State::PENDING_ENGAGEMENT);
 
   // Trigger an engagement increase that signals observers and expect the banner
   // to be shown.
@@ -520,7 +518,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
                      base::Unretained(service),
                      browser()->tab_strip_model()->GetActiveWebContents(),
                      ui::PageTransition::PAGE_TRANSITION_TYPED),
-      true /* expected_will_show */, false /* expected_waiting_to_log_status */,
+      true /* expected_will_show */, false /* expected_need_to_log_status */,
       State::COMPLETE);
 
   histograms.ExpectTotalCount(banners::kMinutesHistogram, 1);
@@ -543,12 +541,12 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, CheckOnLoadThenNavigate) {
   // state, without showing a banner.
   TriggerBannerFlowWithNavigation(
       browser(), manager.get(), test_url, false /* expected_will_show */,
-      true /* expected_waiting_to_log_status */, State::PENDING_ENGAGEMENT);
+      true /* expected_need_to_log_status */, State::PENDING_ENGAGEMENT);
 
   // Navigate and expect Stop() to be called.
   TriggerBannerFlowWithNavigation(browser(), manager.get(), GURL("about:blank"),
                                   false /* expected_will_show */,
-                                  false /* expected_waiting_to_log_status */,
+                                  false /* expected_need_to_log_status */,
                                   State::INACTIVE);
 
   histograms.ExpectTotalCount(banners::kMinutesHistogram, 0);
@@ -573,12 +571,12 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
   // Navigate and expect the manager to end up waiting for prompt to be called.
   TriggerBannerFlowWithNavigation(
       browser(), manager.get(), test_url, false /* expected_will_show */,
-      true /* expected_waiting_to_log_status */, State::PENDING_PROMPT);
+      true /* expected_need_to_log_status */, State::PENDING_PROMPT);
 
   // Navigate and expect Stop() to be called.
   TriggerBannerFlowWithNavigation(browser(), manager.get(), GURL("about:blank"),
                                   false /* expected_will_show */,
-                                  false /* expected_waiting_to_log_status */,
+                                  false /* expected_need_to_log_status */,
                                   State::INACTIVE);
 
   histograms.ExpectTotalCount(banners::kMinutesHistogram, 0);
@@ -603,7 +601,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
   // Navigate to page and get the pipeline started.
   TriggerBannerFlowWithNavigation(
       browser(), manager.get(), test_url, false /* expected_will_show */,
-      true /* expected_waiting_to_log_status */, State::PENDING_PROMPT);
+      true /* expected_need_to_log_status */, State::PENDING_PROMPT);
 
   // Now let the page call prompt without a gesture, an error should be
   // generated.
@@ -611,8 +609,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
                     base::BindOnce(&ExecuteScript, browser(), "callPrompt();",
                                    false /* with_gesture */),
                     false /* expected_will_show */,
-                    false /* expected_waiting_to_log_status */,
-                    State::COMPLETE);
+                    false /* expected_need_to_log_status */, State::COMPLETE);
 
   histograms.ExpectTotalCount(banners::kMinutesHistogram, 0);
   histograms.ExpectUniqueSample(banners::kInstallableStatusCodeHistogram,
@@ -636,7 +633,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
   // Navigate to page and get the pipeline started.
   TriggerBannerFlowWithNavigation(
       browser(), manager.get(), test_url, false /* expected_will_show */,
-      true /* expected_waiting_to_log_status */, State::PENDING_PROMPT);
+      true /* expected_need_to_log_status */, State::PENDING_PROMPT);
 
   // Now let the page call prompt without a gesture, an error should be
   // generated.
@@ -644,8 +641,7 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest,
                     base::BindOnce(&ExecuteScript, browser(), "callPrompt();",
                                    true /* with_gesture */),
                     true /* expected_will_show */,
-                    false /* expected_waiting_to_log_status */,
-                    State::COMPLETE);
+                    false /* expected_need_to_log_status */, State::COMPLETE);
 
   histograms.ExpectTotalCount(banners::kMinutesHistogram, 1);
   histograms.ExpectUniqueSample(banners::kInstallableStatusCodeHistogram,