Fix for flakiness in pyauto automation hook WaitForDownloadsToComplete.

chrome/browser/automation/testing_automation_provider.cc

 // 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.
 
 #include "chrome/browser/automation/testing_automation_provider.h"
 
 #include <map>
 #include <set>
 #include <string>
 #include <vector>
@@ skipping 2924 matching lines @@
   if (downloads.empty()) {
     AutomationJSONReply(this, reply_message).SendSuccess(NULL);
     return;
   }
   // The observer owns itself.  When the last observed item pings, it
   // deletes itself.
   AutomationProviderDownloadItemObserver* item_observer =
       new AutomationProviderDownloadItemObserver(
           this, reply_message, downloads.size());
   for (std::vector<DownloadItem*>::iterator i = downloads.begin();
-       i != downloads.end();
-       i++) {
+       i != downloads.end(); ++i) {
     (*i)->AddObserver(item_observer);
   }
+  // It's possible that pending downloads may have completed before the observer

[Paweł Hajdan Jr., 2011/08/01 22:42:10]

Seems hacky. How about starting the new download only after the observer is
ready? Also, isn't the new version still racy?

[Nirnimesh, 2011/08/01 22:49:24]

In a perfect world, yes. But the flakes in the download system aren't fully
solved, and I see crashes every few days. What happens when and how isn't quite
clear. This CL patches up this automation to make it at least more robust.

[dennis_jeffrey, 2011/08/02 00:04:53]

The suggested approach works well in the case where we simply want to trigger a
download in a particular way and wait for it to finish, as these actions can be
encapsulated in a single automation hook.  However, this use case doesn't
satisfy all existing pyauto needs: e.g., see testMultipleDownloadsInfobar in
infobars.py, which separates the action of triggering a zip download from the
action of waiting for downloads to finish.  A complete solution for fixing
download flakiness in the automation is not clear to me, but the current change
at least significantly improves robustness of this hook and should improve
robustness of several existing pyauto tests.  I needed this change to unblock me
in other work I'm doing to investigate performance of downloads in
chrome/chromeos, and I don't have the bandwidth to tackle a complete solution
anytime soon.  Given this, how should we proceed with this CL?

[Paweł Hajdan Jr., 2011/08/02 16:53:23]

Please show me the bugs and I can fix them. Let's at least make sure they're on
file and not mythical.

Also note that doing *this* in a hacky way contributes to overall flakiness and
hackiness.
Agreed, we have crashes, and I think pyauto is very helpful in detecting them (I
remember it was pyauto that detected most of my download code regressions in
2010). However, I think this is not relevant to doing this in a hack or
non-hacky way.
Agreed that for short term it improves things, but the fix seems fragile and can
break with the next change in the downloads system. Let's make sure we
understand the root cause of the problem and fix it once and for good.

[Paweł Hajdan Jr., 2011/08/02 16:53:23]

This is a known problem with pyauto. I see the following ways to handle it:

1) Declare it's out of scope for pyauto and test it with a browser test instead.
You probably don't want that, but I included it for completeness.

2) Have a windowed notification observer, i.e. set up the observers first with
one automation message, then (possibly indirectly) trigger the download, and
finally wait for that observer to observe the wanted event. This is what we do
in browser_tests and it *works*.
"I have no time to do it properly" is no argument. Sorry, but I have no time to
fix all the bad hacks after you guys, and this is why I'm doing those drive-by
reviews. The hacks cause us a lot of problems long term, and we should really
avoid doing them in the first place.

Please let me know though if I can help you with writing a more correct
solution. My only goal here is to help you make this change in a solid way, so
that it wouldn't need frequent fixes in the future.

+  // above has been completely set up.  Handle this case by checking again and
+  // returning if there are no pending downloads.
+  browser->profile()->GetDownloadManager()->
+      GetCurrentDownloads(FilePath(), &downloads);
+  if (downloads.empty()) {
+    delete item_observer;
+    AutomationJSONReply(this, reply_message).SendSuccess(NULL);
+  }
 }
 
 namespace {
 
 DownloadItem* GetDownloadItemFromId(int id, DownloadManager* download_manager) {
   std::vector<DownloadItem*> downloads;
   download_manager->GetAllDownloads(FilePath(), &downloads);
   DownloadItem* selected_item = NULL;
 
   for (std::vector<DownloadItem*>::iterator it = downloads.begin();
@@ skipping 3181 matching lines @@
   IPC::ParamTraits<std::vector<GURL> >::Write(reply_message_, redirects_gurl);
 
   Send(reply_message_);
   redirect_query_ = 0;
   reply_message_ = NULL;
 }
 
 void TestingAutomationProvider::OnRemoveProvider() {
   AutomationProviderList::GetInstance()->RemoveProvider(this);
 }