[Autofill] Componentize AutofillCCInfoBarDelegate.

components/autofill/core/browser/autofill_cc_infobar_delegate.h

 // Copyright (c) 2011 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_AUTOFILL_AUTOFILL_CC_INFOBAR_DELEGATE_H_
-#define CHROME_BROWSER_AUTOFILL_AUTOFILL_CC_INFOBAR_DELEGATE_H_
+#ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_AUTOFILL_CC_INFOBAR_DELEGATE_H_
+#define COMPONENTS_AUTOFILL_CORE_BROWSER_AUTOFILL_CC_INFOBAR_DELEGATE_H_
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "components/autofill/core/browser/autofill_metrics.h"
 #include "components/infobars/core/confirm_infobar_delegate.h"
 #include "ui/base/window_open_disposition.h"
 
-class CreditCard;
-class PersonalDataManager;
-class InfoBarService;
+namespace infobars {
+class InfoBarManager;
+}
 
 namespace autofill {
 
+class AutofillDriver;
+
 // An InfoBar delegate that enables the user to allow or deny storing credit
 // card information gathered from a form submission.
 class AutofillCCInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:
-  // Creates an autofill credit card infobar and delegate and adds the infobar
-  // to |infobar_service|.
-  static void Create(InfoBarService* infobar_service,
+  // Creates an autofill credit card infobar and delegate and add the infobar

[Peter Kasting, 2014/12/29 22:24:30]

The old comment was correct (except for using infobar_service where we now want
infobar_manager).  You don't add an autofill driver to the infobar manager, and
your change of "adds" to "add" is a grammar error.

[Pritam Nikam, 2014/12/30 13:22:14]

Done.

+  // and associated |autofill_driver| to the |infobar_manager|.
+  static void Create(infobars::InfoBarManager* infobar_manager,
+                     AutofillDriver* autofill_driver,
                      const base::Closure& save_card_callback);
 
 #if defined(UNIT_TEST)
   static scoped_ptr<ConfirmInfoBarDelegate> Create(
+      AutofillDriver* autofill_driver,
       const base::Closure& save_card_callback) {
     return scoped_ptr<ConfirmInfoBarDelegate>(
-        new AutofillCCInfoBarDelegate(save_card_callback));
+        new AutofillCCInfoBarDelegate(autofill_driver, save_card_callback));
   }
 #endif
 
  private:
-  explicit AutofillCCInfoBarDelegate(const base::Closure& save_card_callback);
+  AutofillCCInfoBarDelegate(AutofillDriver* autofill_driver,
+                            const base::Closure& save_card_callback);
   ~AutofillCCInfoBarDelegate() override;
 
   void LogUserAction(AutofillMetrics::InfoBarMetric user_action);
 
   // ConfirmInfoBarDelegate:
   void InfoBarDismissed() override;
   int GetIconID() const override;
   Type GetInfoBarType() const override;
   bool ShouldExpireInternal(const NavigationDetails& details) const override;
   base::string16 GetMessageText() const override;
   base::string16 GetButtonLabel(InfoBarButton button) const override;
   bool Accept() override;
   bool Cancel() override;
   base::string16 GetLinkText() const override;
   bool LinkClicked(WindowOpenDisposition disposition) override;
 
+  // Performs navigation to handle any link click. |autofill_driver_| is not
+  // owned and does not garantee to outlive the lifetime of |this| object. So
+  // care must be taken to avoid any unsafe access. Hence, instaed of using raw
+  // |autofill_driver_| pointers, it's recommended here to content within
+  // base::WeakPtr<>. E.g. AutofillDriver object associated to a form inside an
+  // iframe may get destroy long before the main frame (its tab). Apparantly,
+  // caching raw |autofill_driver_| pointer instead would be unsafe, performing
+  // LinkClinked() from infobar would be catastrophic.

[Peter Kasting, 2014/12/29 22:24:30]

This comment is confusing, in part because of its grammar/mechanics errors, but
also because I don't know if I understand what you're saying.  Are you saying
that it's possible to have an autofill infobar related to iframe content, and
then have the iframe be destroyed, but the infobar not be removed?  If so, that
seems like a bug: why isn't the infobar being closed when the content it's
related to is destroyed?  Or is that not the scenario you're describing?

And why wasn't this a problem in the old code, which unconditionally derefed
WebContentsFromInfoBar()?  It seems like that always assumed that there was a
matching WebContents for the infobar, but now you're saying there isn't.  Was
the code before broken, or are you speaking in hypotheticals instead of about
something you've actually reproduced?

[Pritam Nikam, 2014/12/30 13:22:14]

Sorry for confusion! maybe I didn't phrase it correctly.

I'd share my understanding about life expectancies of different objects in below
excerpt:

A. WebContents life expectancy -> as long as tab is open (with switch
"--process-per-tab")
B. Infobar's life expectancy -> WebContents
C. AutofillClient's life expectancy -> WebContents (One AutofillClient per
WebContents)
D. AutofillDriverFactory's life expectancy -> WebContents (One factory per
WebContents)
E. WebContents can have multiple RenderFrame (i.e. iframes).
AutofillDriverFactory takes care of creating one AutofillDriver per RenderFrame
within that WebContents.
F. i.e. AutofillDriver's (& AutofillManager's) life expectancy -> RenderFrame
(One AutofillDriver per RenderFrame).

Going by above understanding, Infobar lives longer compare to AutofillDriver
(associated to RenderFrames). In other words; cases where RenderFrames
associated to the page contents get destroy;  autofill driver *does not* outlive
the infobar.

If we cache raw AutofillDriver pointers (associated to RenderFrames) in Infobar,
there's a chance that link clicked on infobar would be unsafe.


Please correct me if I'm missing something.

[Peter Kasting, 2014/12/30 18:57:27]

You keep speaking in abstract/hypothetical terms.  I'm trying to stop talking
about hypothetical possibilities, and start verifying whether we can really
cause problems in actual usage.

In other words, you need to verify, concretely, that you can actually have a
problem here.  If you can, the fix is probably not to use WeakPtrs, but either:
(1) to make sure the infobar is removed if its associated page content is
destroyed, or
(2) to make AutofillDriver scoped to the WebContents, not the RenderFrame

For example, crate a page with an inner frame that triggers the infobar,
followed by the containing frame destroying the inner frame.  Something like
that.

+  base::WeakPtr<AutofillDriver> autofill_driver_;
+
   // The callback to save credit card if the user accepts the infobar.
   base::Closure save_card_callback_;
 
   // Did the user ever explicitly accept or dismiss this infobar?
   bool had_user_interaction_;
 
   FRIEND_TEST_ALL_PREFIXES(AutofillMetricsTest, CreditCardInfoBar);
 
   DISALLOW_COPY_AND_ASSIGN(AutofillCCInfoBarDelegate);
 };
 
 }  // namespace autofill
 
-#endif  // CHROME_BROWSER_AUTOFILL_AUTOFILL_CC_INFOBAR_DELEGATE_H_
+#endif  // COMPONENTS_AUTOFILL_CORE_BROWSER_AUTOFILL_CC_INFOBAR_DELEGATE_H_