Refactor the ParentPermissionDialog to prevent memory management issues

This refactor was motivated by revert 71f516df.

Background:

There was an ASAN error that occurred in certain test cases where
the ParentPermissionDialog was deleted from beneath itself before
it was able to set its dialog_closed_callback_ member, causing a
use-after-free.

The use-after-free was caused by the way this feature was originally
implemented:
2 objects with distinct lifetimes separately managing the view and
workflow of the ParentPermissionDialog.   The 2 objects
spanned sections of the codebase (ui and views) which have dependency
restrictions that made it very challenging to coordinate the lifetime
of objects across them.  Furthermore, the flows in this code are full
of asynchronous sections that necessitated an extra callback-based
message passing interface between the 2 objects.

This CL consolidates all the existing to a single object:
ParentPermissionDialogView, thereby removing most of the aforementioned
dependency and object lifetime management complexity.

The API for creating the dialog remains in parent_permission_dialog.h,
however it is now in a much simplified form, consisting of a simple
control  interface and Show() functions.

Original change's description:
> Revert "Changes to Webstore Private API to support child extension installation"
>
> This reverts commit 99ffda8e.
>
> Reason for revert: browser_tests failing on https://ci.chromium.org/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/37129 and https://ci.chromium.org/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/18174
>
> Original change's description:
> > Changes to Webstore Private API to support child extension installation
> >
> > These changes are required in order to prompt a child user to get
> > parent permission when they attempt to install an extension in the
> > Chrome Webstore.
> >
> > This CL also enables the feature by default.
> >
> > Bug: 957832
> > Change-Id: I3a2011b418e31dd491fd35e37d976b492eef197b
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2079620
> > Commit-Queue: Dan S <danan@chromium.org>
> > Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#749436}
>
> TBR=karandeepb@chromium.org,danan@chromium.org
>
> Change-Id: If262ccd3dad279dc60e849f9780e340b18d7384c
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 957832
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2100891
> Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
> Commit-Queue: Oksana Zhuravlova <oksamyt@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#749726}

Change-Id: Ia217f68502a6fe8719614b1322af74aeb02f3319
No-Presubmit: false
No-Tree-Checks: false
No-Try: false
Bug: 957832
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2100548
Commit-Queue: Dan S <danan@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#753468}

chrome/browser/supervised_user/supervised_user_service_management_api_delegate.cc

@@ -11,11 +11,11 @@
 #include "chrome/browser/supervised_user/supervised_user_service.h"
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
 #include "chrome/browser/ui/supervised_user/parent_permission_dialog.h"
+#include "content/public/browser/web_contents.h"
 
 namespace {
 
 void OnParentPermissionDialogComplete(
-    std::unique_ptr<ParentPermissionDialog> dialog,
     extensions::SupervisedUserServiceDelegate::
         ParentPermissionDialogDoneCallback delegate_done_callback,
     ParentPermissionDialog::Result result) {
@@ -73,25 +73,15 @@ void SupervisedUserServiceManagementAPIDelegate::
         content::BrowserContext* context,
         content::WebContents* contents,
         ParentPermissionDialogDoneCallback done_callback) {
-  std::unique_ptr<ParentPermissionDialog> dialog =
-      std::make_unique<ParentPermissionDialog>(
-          Profile::FromBrowserContext(context));
-
-  // Cache the pointer so we can show the dialog after we pass
-  // ownership to the callback.
-  ParentPermissionDialog* dialog_ptr = dialog.get();
-
-  // Ownership of the dialog passes to the callback.  This allows us
-  // to have as many instances of the dialog as calls to the management
-  // API.
-  ParentPermissionDialog::DoneCallback inner_done_callback =
-      base::BindOnce(&::OnParentPermissionDialogComplete, std::move(dialog),
-                     std::move(done_callback));
-
-  // This is safe because moving a unique_ptr doesn't change the underlying
-  // object's address.
-  dialog_ptr->ShowPromptForExtensionInstallation(
-      contents, &extension, SkBitmap(), std::move(inner_done_callback));
+  ParentPermissionDialog::DoneCallback inner_done_callback = base::BindOnce(
+      &::OnParentPermissionDialogComplete, std::move(done_callback));
+
+  parent_permission_dialog_ =
+      ParentPermissionDialog::CreateParentPermissionDialogForExtension(
+          Profile::FromBrowserContext(context), contents,
+          contents->GetTopLevelNativeWindow(), gfx::ImageSkia(), &extension,
+          std::move(inner_done_callback));
+  parent_permission_dialog_->ShowDialog();
 }
 
 }  // namespace extensions

chrome/browser/supervised_user/supervised_user_service_management_api_delegate.h

@@ -11,6 +11,8 @@ namespace content {
 class BrowserContext;
 }
 
+class ParentPermissionDialog;
+
 namespace extensions {
 
 class SupervisedUserServiceManagementAPIDelegate
@@ -34,6 +36,9 @@ class SupervisedUserServiceManagementAPIDelegate
       content::WebContents* contents,
       extensions::SupervisedUserServiceDelegate::
           ParentPermissionDialogDoneCallback done_callback) override;
+
+ private:
+  std::unique_ptr<ParentPermissionDialog> parent_permission_dialog_;
 };
 
 }  // namespace extensions

chrome/browser/ui/BUILD.gn

@@ -3957,7 +3957,6 @@ jumbo_static_library("ui") {
       "ash/launcher/shelf_spinner_controller.h",
       "ash/launcher/shelf_spinner_item_controller.cc",
       "ash/launcher/shelf_spinner_item_controller.h",
-      "supervised_user/parent_permission_dialog.cc",
       "supervised_user/parent_permission_dialog.h",
       "views/apps/app_dialog/app_block_dialog_view.cc",
       "views/apps/app_dialog/app_block_dialog_view.h",

chrome/browser/ui/views/parent_permission_dialog_view.h

@@ -11,59 +11,47 @@
 #include "base/macros.h"
 #include "chrome/browser/extensions/install_prompt_permissions.h"
 #include "chrome/browser/ui/supervised_user/parent_permission_dialog.h"
+#include "components/signin/public/identity_manager/access_token_info.h"
+#include "google_apis/gaia/gaia_auth_consumer.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
 #include "ui/views/view.h"
 
-class Profile;
+class GaiaAuthFetcher;
 
 namespace extensions {
 class Extension;
 }  // namespace extensions
 
+namespace signin {
+class AccessTokenFetcher;
+struct AccessTokenInfo;
+class IdentityManager;
+}  // namespace signin
+
+namespace views {
+class Label;
+}
+
 class ParentPermissionSection;
 
 // Modal dialog that shows a dialog that prompts a parent for permission by
-// asking them to enter their google account credentials.
-class ParentPermissionDialogView : public views::BubbleDialogDelegateView {
+// asking them to enter their google account credentials.  This is created only
+// when the dialog is ready to be shown (after the state has been
+// asynchronously fetched).
+class ParentPermissionDialogView : public views::BubbleDialogDelegateView,
+                                   public GaiaAuthConsumer {
  public:
-  using DoneCallback = base::OnceCallback<void(
-      internal::ParentPermissionDialogViewResult result)>;
-
-  struct Params {
-    Params();
-    explicit Params(const Params& params);
-    ~Params();
-
-    // List of email addresses of parents for whom parent permission for
-    // installation should be requested.  These should be the emails of parent
-    // accounts that are permitted to approve extension installations
-    // for the current child user.
-    std::vector<base::string16> parent_permission_email_addresses;
-
-    // If true, shows a message in the parent permission dialog that the
-    // password entered was incorrect.
-    bool show_parent_password_incorrect = true;
-
-    // The icon to be displayed.
-    gfx::ImageSkia icon;
-
-    // The message to show.
-    base::string16 message;
-
-    // An optional extension whose permissions should be displayed
-    const extensions::Extension* extension = nullptr;
-
-    // The user's profile
-    Profile* profile = nullptr;
-
-    // The parent window to this window.
-    gfx::NativeWindow window = nullptr;
+  class Observer {
+   public:
+    // Tells observers that their references to the view are becoming invalid.
+    virtual void OnParentPermissionDialogViewDestroyed() = 0;
   };
+  struct Params;
 
   ParentPermissionDialogView(std::unique_ptr<Params> params,
-                             DoneCallback done_callback);
+                             Observer* observer);
 
   ~ParentPermissionDialogView() override;
 
@@ -71,9 +59,15 @@ class ParentPermissionDialogView : public views::BubbleDialogDelegateView {
   ParentPermissionDialogView& operator=(const ParentPermissionDialogView&) =
       delete;
 
+  // Closes the dialog.
+  void CloseDialog();
+
   // Shows the parent permission dialog.
   void ShowDialog();
 
+  // Removes the observer reference.
+  void RemoveObserver();
+
   void set_selected_parent_permission_email_address(
       const base::string16& email_address) {
     selected_parent_permission_email_ = email_address;
@@ -82,24 +76,19 @@ class ParentPermissionDialogView : public views::BubbleDialogDelegateView {
   void set_parent_permission_credential(const base::string16& credential) {
     parent_permission_credential_ = credential;
   }
-  void CloseDialogView();
 
-  // TODO(https://crbug.com/1058501):  Instead of doing this with closures, use
-  // an interface shared with the views code whose implementation wraps the
-  // underlying view, and delete the instance of that interface in the dtor of
-  // this class.
-  base::OnceClosure GetCloseDialogClosure();
+  bool invalid_credential_received() { return invalid_credential_received_; }
+  void SetIdentityManagerForTesting(signin::IdentityManager* identity_manager);
+  void SetRepromptAfterIncorrectCredential(bool reprompt);
 
  private:
-  const Params& params() const { return *params_; }
-
   base::string16 GetActiveUserFirstName() const;
 
   // views::View:
   gfx::Size CalculatePreferredSize() const override;
   void AddedToWidget() override;
 
-  // views::DialogDeleate:
+  // views::DialogDelegate:
   bool Cancel() override;
   bool Accept() override;
 
@@ -117,6 +106,41 @@ class ParentPermissionDialogView : public views::BubbleDialogDelegateView {
 
   void ShowDialogInternal();
 
+  void AddInvalidCredentialLabel();
+  void LoadParentEmailAddresses();
+  void OnExtensionIconLoaded(const gfx::Image& image);
+  void LoadExtensionIcon();
+  void CloseWithReason(views::Widget::ClosedReason reason);
+
+  // Given an email address of the child's parent, return the parents'
+  // obfuscated gaia id.
+  std::string GetParentObfuscatedGaiaID(
+      const base::string16& parent_email) const;
+
+  // Starts the Reauth-scoped OAuth access token fetch process.
+  void StartReauthAccessTokenFetch(const std::string& parent_obfuscated_gaia_id,
+                                   const std::string& parent_credential);
+
+  // Handles the result of the access token
+  void OnAccessTokenFetchComplete(const std::string& parent_obfuscated_gaia_id,
+                                  const std::string& parent_credential,
+                                  GoogleServiceAuthError error,
+                                  signin::AccessTokenInfo access_token_info);
+
+  // Starts the Parent Reauth proof token fetch process.
+  void StartParentReauthProofTokenFetch(
+      const std::string& child_access_token,
+      const std::string& parent_obfuscated_gaia_id,
+      const std::string& credential);
+
+  // GaiaAuthConsumer
+  void OnReAuthProofTokenSuccess(
+      const std::string& reauth_proof_token) override;
+  void OnReAuthProofTokenFailure(
+      const GaiaAuthConsumer::ReAuthProofTokenStatus error) override;
+
+  void SendResult(ParentPermissionDialog::Result result);
+
   // Sets the |extension| to be optionally displayed in the dialog.  This
   // causes the view to show several extension properties including the
   // permissions, the icon and the extension name.
@@ -127,17 +151,52 @@ class ParentPermissionDialogView : public views::BubbleDialogDelegateView {
   // if an extension has been set.
   extensions::InstallPromptPermissions prompt_permissions_;
 
+  // The email address of the parents to display in the dialog.
+  std::vector<base::string16> parent_permission_email_addresses_;
+
+  bool reprompt_after_incorrect_credential_ = true;
+
+  // Contains the parent-permission related views widgets.
   std::unique_ptr<ParentPermissionSection> parent_permission_section_;
 
+  views::Label* invalid_credential_label_ = nullptr;
+
+  bool invalid_credential_received_ = false;
+
+  // The currently selected parent email.
   base::string16 selected_parent_permission_email_;
+
+  // The currently entered parent credential.
   base::string16 parent_permission_credential_;
 
-  // Configuration parameters for the prompt.
+  // Parameters for the dialog.
   std::unique_ptr<Params> params_;
 
-  DoneCallback done_callback_;
+  // Used to ensure we don't try to show same dialog twice.
+  bool is_showing_ = false;
+
+  // Used to fetch the Reauth token.
+  std::unique_ptr<GaiaAuthFetcher> reauth_token_fetcher_;
+
+  // Used to fetch OAuth2 access tokens.
+  signin::IdentityManager* identity_manager_ = nullptr;
+  std::unique_ptr<signin::AccessTokenFetcher> oauth2_access_token_fetcher_;
+
+  Observer* observer_;
 
   base::WeakPtrFactory<ParentPermissionDialogView> weak_factory_{this};
 };
 
+// Allows tests to observe the create of the testing instance of
+// ParentPermissionDialogView
+class TestParentPermissionDialogViewObserver {
+ public:
+  // Implementers should pass "this" as constructor argument.
+  TestParentPermissionDialogViewObserver(
+      TestParentPermissionDialogViewObserver* observer);
+  ~TestParentPermissionDialogViewObserver();
+  virtual void OnTestParentPermissionDialogViewCreated(
+      ParentPermissionDialogView* view) = 0;
+};
+
 #endif  // CHROME_BROWSER_UI_VIEWS_PARENT_PERMISSION_DIALOG_VIEW_H_

chrome/browser/ui/views/parent_permission_dialog_view_browsertest.cc

-// Copyright 2020 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/ui/views/parent_permission_dialog_view.h"
-
-#include <memory>
-#include <vector>
-
-#include "chrome/browser/extensions/extension_util.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_window.h"
-#include "chrome/browser/ui/supervised_user/parent_permission_dialog.h"
-#include "chrome/browser/ui/test/test_browser_dialog.h"
-#include "chrome/test/base/chrome_test_utils.h"
-#include "components/signin/public/identity_manager/identity_test_environment.h"
-
-// A simple DialogBrowserTest for the ParentPermissionDialogView that just
-// shows the dialog.
-class ParentPermissionDialogViewBrowserTest : public DialogBrowserTest {
- public:
-  ParentPermissionDialogViewBrowserTest() {}
-
-  ParentPermissionDialogViewBrowserTest(
-      const ParentPermissionDialogViewBrowserTest&) = delete;
-  ParentPermissionDialogViewBrowserTest& operator=(
-      const ParentPermissionDialogViewBrowserTest&) = delete;
-
-  void OnParentPermissionPromptDone(
-      internal::ParentPermissionDialogViewResult result) {}
-
-  void ShowUi(const std::string& name) override {
-    std::vector<base::string16> parent_emails =
-        std::vector<base::string16>{base::UTF8ToUTF16("parent1@google.com"),
-                                    base::UTF8ToUTF16("parent2@google.com")};
-
-    gfx::ImageSkia icon = extensions::util::GetDefaultExtensionIcon();
-
-    ShowParentPermissionDialog(
-        browser()->profile(), browser()->window()->GetNativeWindow(),
-        parent_emails, false, icon, base::UTF8ToUTF16("Test Message"), nullptr,
-        base::BindOnce(&ParentPermissionDialogViewBrowserTest::
-                           OnParentPermissionPromptDone,
-                       base::Unretained(this)));
-  }
-};
-
-IN_PROC_BROWSER_TEST_F(ParentPermissionDialogViewBrowserTest,
-                       InvokeUi_default) {
-  ShowAndVerifyUi();
-}

chrome/test/BUILD.gn

@@ -2396,7 +2396,6 @@ if (!is_android) {
         "../browser/ui/ash/tablet_mode_page_behavior_browsertest.cc",
         "../browser/ui/ash/volume_controller_browsertest.cc",
         "../browser/ui/browser_finder_chromeos_browsertest.cc",
-        "../browser/ui/supervised_user/parent_permission_dialog_browsertest.cc",
         "../browser/ui/views/apps/app_dialog/app_dialog_view_browsertest.cc",
         "../browser/ui/views/apps/app_dialog/app_uninstall_dialog_view_browsertest.cc",
         "../browser/ui/views/apps/chrome_native_app_window_views_aura_ash_browsertest.cc",
@@ -2416,7 +2415,7 @@ if (!is_android) {
         "../browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc",
         "../browser/ui/views/frame/system_menu_model_builder_browsertest_chromeos.cc",
         "../browser/ui/views/frame/top_controls_slide_controller_chromeos_browsertest.cc",
-        "../browser/ui/views/parent_permission_dialog_view_browsertest.cc",
+        "../browser/ui/views/parent_permission_dialog_browsertest.cc",
         "../browser/ui/views/plugin_vm/plugin_vm_installer_view_browsertest.cc",
         "../browser/ui/views/web_apps/web_app_ash_interactive_ui_test.cc",
         "../browser/ui/web_applications/web_app_guest_session_browsertest_chromeos.cc",