Wait for URLBlacklist update tasks on automation tests.

chrome/browser/automation/testing_automation_provider.cc

Index: chrome/browser/automation/testing_automation_provider.cc
diff --git a/chrome/browser/automation/testing_automation_provider.cc b/chrome/browser/automation/testing_automation_provider.cc
index 51ae96eddac979f4cdc9ded67d1c8753dfb86d98..836fe06342d1fc80b98e73c2f7fa47b9e1351b21 100644
--- a/chrome/browser/automation/testing_automation_provider.cc
+++ b/chrome/browser/automation/testing_automation_provider.cc
@@ -176,6 +176,12 @@ class AutomationInterstitialPage : public InterstitialPage {
   DISALLOW_COPY_AND_ASSIGN(AutomationInterstitialPage);
 };
 
+// Helper function for nested Binds that resolves the overloading of
+// BrowserThread::PostTask, and ignores the return value.

[Joao da Silva, 2011/10/21 12:59:58]

I tried to resolve the overloading with

bool (*post)(BrowserThread::ID, const Location&, const Closure&) =
&BrowserThread::PostTask;

And passing |post| to bind instead, but this had issues because the bool return
value wasn't ignored. Adding base::IgnoreResult() then confused the nested
Binds. At this point my humble c++ skills were defeated, and I opted for this. I
also find it more readable in the end.

+void PostTask(BrowserThread::ID id, const base::Closure& task) {
+  CHECK(BrowserThread::PostTask(id, FROM_HERE, task));

[Paweł Hajdan Jr., 2011/10/21 15:13:17]

Please make it return the value it gets from the real PostTask, and let's see
below if we still need to CHECK. We shouldn't.

+}
+
 }  // namespace
 
 TestingAutomationProvider::TestingAutomationProvider(Profile* profile)
@@ -2730,41 +2736,26 @@ void TestingAutomationProvider::PerformActionOnInfobar(
 
 namespace {
 
-// Task to get info about BrowserChildProcessHost. Must run on IO thread to
+// Gets info about BrowserChildProcessHost. Must run on IO thread to
 // honor the semantics of BrowserChildProcessHost.
 // Used by AutomationProvider::GetBrowserInfo().
-class GetChildProcessHostInfoTask : public Task {
- public:
-  GetChildProcessHostInfoTask(base::WaitableEvent* event,
-                              ListValue* child_processes)
-    : event_(event),
-      child_processes_(child_processes) {}
-
-  virtual void Run() {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    for (BrowserChildProcessHost::Iterator iter; !iter.Done(); ++iter) {
-      // Only add processes which are already started,
-      // since we need their handle.
-      if ((*iter)->handle() == base::kNullProcessHandle) {
-        continue;
-      }
-      ChildProcessInfo* info = *iter;
-      DictionaryValue* item = new DictionaryValue;
-      item->SetString("name", info->name());
-      item->SetString("type",
-                      ChildProcessInfo::GetTypeNameInEnglish(info->type()));
-      item->SetInteger("pid", base::GetProcId(info->handle()));
-      child_processes_->Append(item);
+void GetChildProcessHostInfo(ListValue* child_processes) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  for (BrowserChildProcessHost::Iterator iter; !iter.Done(); ++iter) {
+    // Only add processes which are already started,
+    // since we need their handle.
+    if ((*iter)->handle() == base::kNullProcessHandle) {

[Paweł Hajdan Jr., 2011/10/21 15:13:17]

nit: No need for braces {}.

[Joao da Silva, 2011/10/22 11:26:50]

Done.

+      continue;
     }
-    event_->Signal();
+    ChildProcessInfo* info = *iter;
+    DictionaryValue* item = new DictionaryValue;
+    item->SetString("name", info->name());
+    item->SetString("type",
+                    ChildProcessInfo::GetTypeNameInEnglish(info->type()));
+    item->SetInteger("pid", base::GetProcId(info->handle()));
+    child_processes->Append(item);
   }
-
- private:
-  base::WaitableEvent* const event_;  // weak
-  ListValue* child_processes_;
-
-  DISALLOW_COPY_AND_ASSIGN(GetChildProcessHostInfoTask);
-};
+}
 
 }  // namespace
 
@@ -2876,20 +2867,6 @@ void TestingAutomationProvider::GetBrowserInfo(
   int flags = ChildProcessHost::CHILD_NORMAL;
 #endif
 
-  return_value->SetString("child_process_path",
-                          ChildProcessHost::GetChildPath(flags).value());
-  // Child processes are the processes for plugins and other workers.
-  // Add all child processes in a list of dictionaries, one dictionary item
-  // per child process.
-  ListValue* child_processes = new ListValue;
-  base::WaitableEvent event(true   /* manual reset */,
-                            false  /* not initially signaled */);
-  CHECK(BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      new GetChildProcessHostInfoTask(&event, child_processes)));
-  event.Wait();
-  return_value->Set("child_processes", child_processes);
-
   // Add all extension processes in a list of dictionaries, one dictionary
   // item per extension process.
   ListValue* extension_views = new ListValue;
@@ -2945,7 +2922,20 @@ void TestingAutomationProvider::GetBrowserInfo(
     }
   }
   return_value->Set("extension_views", extension_views);
-  AutomationJSONReply(this, reply_message).SendSuccess(return_value.get());
+
+  return_value->SetString("child_process_path",
+                          ChildProcessHost::GetChildPath(flags).value());
+  // Child processes are the processes for plugins and other workers.
+  // Add all child processes in a list of dictionaries, one dictionary item
+  // per child process.
+  ListValue* child_processes = new ListValue;
+  return_value->Set("child_processes", child_processes);
+  CHECK(BrowserThread::PostTaskAndReply(

[Paweł Hajdan Jr., 2011/10/21 15:13:17]

Why don't you just SendError if this fails?

+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&GetChildProcessHostInfo, child_processes),
+      base::Bind(&AutomationJSONReply::SendSuccess,
+                 base::Owned(new AutomationJSONReply(this, reply_message)),
+                 base::Owned(return_value.release()))));
 }
 
 // Sample json input: { "command": "GetProcessInfo" }
@@ -5871,10 +5861,11 @@ void TestingAutomationProvider::WaitForAllTabsToStopLoading(
 void TestingAutomationProvider::SetPolicies(
     DictionaryValue* args,
     IPC::Message* reply_message) {
-  AutomationJSONReply reply(this, reply_message);
+  scoped_ptr<AutomationJSONReply> reply(
+      new AutomationJSONReply(this, reply_message));
 
 #if !defined(ENABLE_CONFIGURATION_POLICY) || defined(OFFICIAL_BUILD)
-  reply.SendError("Configuration Policy disabled");
+  reply->SendError("Configuration Policy disabled");
 #else
   const policy::PolicyDefinitionList* list =
       policy::GetChromePolicyDefinitionList();
@@ -5895,7 +5886,7 @@ void TestingAutomationProvider::SetPolicies(
     if (args->GetDictionary(providers[i].name, &policies) &&
         policies &&
         !providers[i].provider) {
-      reply.SendError("Provider not available: " + providers[i].name);
+      reply->SendError("Provider not available: " + providers[i].name);
       return;
     }
   }
@@ -5908,7 +5899,17 @@ void TestingAutomationProvider::SetPolicies(
     }
   }
 
-  reply.SendSuccess(NULL);
+  // OverridePolicies() triggers preference updates, which triggers preference
+  // listeners. Some policies (e.g. URLBlacklist) post tasks to other message
+  // loops before they start being enforced; make sure those tasks have
+  // finished.
+  PostTask(BrowserThread::IO,

[Paweł Hajdan Jr., 2011/10/21 15:13:17]

Please add some base::Bind gurus for possible advice about this. I'm sort of
fine, but maybe there's a better way and we can all learn.

[Joao da Silva, 2011/10/21 16:25:53]

Good idea, adding @willchan as the Bind guru :-)

[willchan no longer on Chromium, 2011/10/21 20:50:29]

This code gave me a good chuckle today. Thanks for sending it my way! I think
what you've done is fine.

That said, do you need to do UI=>IO=>FILE=>IO=>UI? That's not obvious from your
comments. They make it sound like you just need to make sure you've done
UI=>IO=>UI and UI=>FILE=>UI (which could be done in parallel). I can imagine
this might be a more common task (indeed, we do something like this for shutdown
to make sure we've flushed files to disk), and I can imagine creating a
BarrierClosure or something to block until all the other closures have
completed. I don't think the full sequence you've specified is common nor done
in a cleaner way than what you already have.

One thing to note though is that this approach is somewhat fragile. If someone
updates URLBlacklistManager to have a different threading model, then won't
obviously break, but may subtly break. If it becomes more common to do stuff
like this, it may be advisable to create an explicit observer interface to watch
for completion.

[Joao da Silva, 2011/10/22 11:26:50]

Thanks for taking a look. I've added an explicit comment about the update
sequence of the blacklist.
This code could be moved to URLBlacklistManager::Update, with the last task
replaced with a NotificationService::Notify(NOTIFICATION_URL_BLACKLIST_UPDATED)
or similar. This place would then wait for that notification before replying,
and changes to the URLBlacklistManager threading model would also change when
the notification is fired.

Since this is the only place needing such a notification, I'd leave as is for
now. As an owner of policy/ and the blacklist in particular, I'll watch out for
subtle changes :-)

+      base::Bind(&PostTask, BrowserThread::FILE,
+          base::Bind(&PostTask, BrowserThread::IO,
+              base::Bind(&PostTask, BrowserThread::UI,

[Paweł Hajdan Jr., 2011/10/21 15:13:17]

So it seems you've done the CHECK in PostTask because they're nested, but I
think you can still pass the reply message as an argument and instead of
CHECKing send an error message. Are there problems with that approach that I've
missed?

[Joao da Silva, 2011/10/21 16:25:53]

I think replies have to be sent from UI; please correct me if I'm wrong about
this.

If so, then an inner PostTask failure would have to post SendError to UI, which
can in turn fail again. I don't see what could be done in that case, and
handling all that would complicate this a lot.

+                  base::Bind(&AutomationJSONReply::SendSuccess,
+                             base::Owned(reply.release()),
+                             static_cast<const Value*>(NULL))))));
 #endif  // defined(OFFICIAL_BUILD)
 }