Made it possible to tell whether an extension is being installed or updated.

chrome/browser/policy/policy_browsertest.cc

Index: chrome/browser/policy/policy_browsertest.cc
diff --git a/chrome/browser/policy/policy_browsertest.cc b/chrome/browser/policy/policy_browsertest.cc
index 1c0dddfa1673098ee5d118ef3e3a7224ce985900..123c2c0a3c0fe9d10ddcd44c75e7872753d79731 100644
--- a/chrome/browser/policy/policy_browsertest.cc
+++ b/chrome/browser/policy/policy_browsertest.cc
@@ -225,6 +225,72 @@ class MakeRequestFail {
   const std::string host_;
 };
 
+// Based on content::WindowedNotificationObserver, but specially written to
+// watch for an extensions::InstalledExtensionInfo and store its fields.
+// This is necessary because a normal WindowedNotificationObserver will store
+// the pointer to the InstalledExtensionInfo, which will immediately become
+// invalid after the call to Observe().

[Matt Giuca, 2013/05/02 09:51:44]

This class is pretty complicated and duplicates a lot of code from
content::WindowedNotificationObserver, but it seems as though it will be
impossible to make this not crash using WindowedNotificationObserver.

The problem is that:
a) ExtensionService::FinishInstallation allocates memory for an
InstalledExtensionInfo, passes it to Notify, then frees the memory immediately.
The observer is not expected to hold on to the pointer. (In preparing this
approach, I looked at other Notify calls and they follow this same pattern.)
b) WindowedNotificationObserver::Observe stores the NotificationDetails object
(in this case, a pointer to an InstalledExtensionInfo) in a field, and then
returns. Therefore, it is storing a dangling pointer.

There is no way to make WindowedNotificationObserver work correctly for this
case and still be fully general. And the NotificationDetails has to be a plain
pointer (not a shared pointer or something similar). So the only way I could
think of of fixing this was to copy the WindowedNotificationObserver code, and
then change it so that its Observe method reads out the fields of the
InstalledExtensionInfo into its fields rather than storing the pointer.

If you have any other suggestions that would be simpler, I'm all ears.

[Mattias Nissler (ping if slow), 2013/05/02 11:00:33]

Given that we're mostly only using the notification to achieve synchronization,
I guess it'd be fine to just continue using WindowedNotificationObserver and
just check via the ExtensionService that our extension has been installed. So
we'd no longer check that the notification contains the correct extension ID,
but that's not really of concern in these tests anyhow.

Adding Joao as well to see what he thinks.

[Joao da Silva, 2013/05/02 11:36:29]

I'd guess that other notifications have the same issue so this behavior is
surprising.

As Mattias mentioned, these notifications are used mostly for synchronization in
these tests and checking that the extension is installed at the ExtensionService
achieves the same.

I don't have a strong preference here. I suggest subclassing
WindowedNotificationObserver and just overriding Observe() to have less code
here, and then just extract the Extension* from the details:

  virtual void Observer(args...) OVERRIDE {
    WindowedNotificationObserver::Observe(args...);
    extension_ = details()->extension;
  }

[Joao da Silva, 2013/05/02 11:42:03]

Discussed a bit more offline with Mattias. We agreed that just waiting for the
notification and ignoring the details is good enough for these tests, and this
class can be avoided altogether.

[Matt Giuca, 2013/05/03 02:02:09]

OK thanks for discussing and coming to a consensus, guys. I have removed the
class and also removed the checks that were crashing (with a comment to explain
why we don't check).

+class InstalledExtensionInfoObserver : public content::NotificationObserver {
+  public:

[Mattias Nissler (ping if slow), 2013/05/02 11:00:33]

indentation

[Matt Giuca, 2013/05/03 02:02:09]

Done. (Not relevant any more.)

+  // Register to listen for notifications of the given type from either a
+  // specific source, or from all sources if |source| is
+  // NotificationService::AllSources().
+  InstalledExtensionInfoObserver(int notification_type,
+                                 const content::NotificationSource& source)
+      : seen_(false),
+        running_(false) {
+    registrar_.Add(this, notification_type, source);
+  }
+
+  // Wait until the specified notification occurs.  If the notification was
+  // emitted between the construction of this object and this call then it
+  // returns immediately.
+  void Wait() {
+    if (seen_)
+      return;
+
+    running_ = true;
+    message_loop_runner_ = new content::MessageLoopRunner;
+    message_loop_runner_->Run();
+    EXPECT_TRUE(seen_);
+  }
+
+  const extensions::Extension* extension() const {
+    return extension_;
+  }
+
+  bool is_update() const {
+    return is_update_;
+  }
+
+  // NotificationObserver:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE {
+    content::Details<const extensions::InstalledExtensionInfo> installed_info(
+        details);
+    extension_ = installed_info->extension;
+    is_update_ = installed_info->is_update;
+    seen_ = true;
+    if (!running_)
+      return;
+
+    message_loop_runner_->Quit();
+    running_ = false;
+  }
+
+ private:
+  bool seen_;
+  bool running_;
+  content::NotificationRegistrar registrar_;
+
+  const extensions::Extension* extension_;

[Joao da Silva, 2013/05/02 11:36:29]

Doesn't this pointer have the same lifetime issue? Shouldn't this be a
scoped_refptr?

[Matt Giuca, 2013/05/03 02:02:09]

No, because the extension itself is longer lived (it gets destroyed by the test
case, after the call to observer.Wait()). The lifetime issue is just that the
caller of Notify / Observe is going to immediately free the
InstalledExtensionInfo object.

It can't be a scoped_refptr without completely refactoring all uses of
InstalledExtensionInfo to contain the extension as a scoped_refptr.

Anyway, we seem to have consensus on deleting this class.

+  bool is_update_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(InstalledExtensionInfoObserver);
+};
+
 // Verifies that the given url |spec| can be opened. This assumes that |spec|
 // points at empty.html in the test data dir.
 void CheckCanOpenURL(Browser* browser, const char* spec) {
@@ -1298,14 +1364,14 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, ExtensionInstallForcelist) {
   PolicyMap policies;
   policies.Set(key::kExtensionInstallForcelist, POLICY_LEVEL_MANDATORY,
                POLICY_SCOPE_USER, forcelist.DeepCopy());
-  content::WindowedNotificationObserver observer(
+  InstalledExtensionInfoObserver observer(
       chrome::NOTIFICATION_EXTENSION_INSTALLED,
       content::NotificationService::AllSources());
   UpdateProviderPolicy(policies);
   observer.Wait();
-  content::Details<const extensions::Extension> details = observer.details();
-  EXPECT_EQ(kGoodCrxId, details->id());
-  EXPECT_EQ(details.ptr(), service->GetExtensionById(kGoodCrxId, true));
+  const extensions::Extension* extension = observer.extension();
+  EXPECT_EQ(kGoodCrxId, extension->id());
+  EXPECT_EQ(extension, service->GetExtensionById(kGoodCrxId, true));
   // The user is not allowed to uninstall force-installed extensions.
   UninstallExtension(kGoodCrxId, false);
 }
@@ -1370,14 +1436,13 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, ExtensionInstallSources) {
                POLICY_SCOPE_USER, install_sources.DeepCopy());
   UpdateProviderPolicy(policies);
 
-  content::WindowedNotificationObserver observer(
+  InstalledExtensionInfoObserver observer(
       chrome::NOTIFICATION_EXTENSION_INSTALLED,
       content::NotificationService::AllSources());
   PerformClick(1, 0);
   observer.Wait();
 
-  content::Details<const extensions::Extension> details = observer.details();
-  EXPECT_EQ(kAdBlockCrxId, details->id());
+  EXPECT_EQ(kAdBlockCrxId, observer.extension()->id());
 
   // The first extension shouldn't be present, the second should be there.
   EXPECT_FALSE(extension_service()->GetExtensionById(kGoodCrxId, true));