Wait for pending requests to finish in ImageDecoder.

chrome/browser/image_decoder_browsertest.cc

Index: chrome/browser/image_decoder_browsertest.cc
diff --git a/chrome/browser/image_decoder_browsertest.cc b/chrome/browser/image_decoder_browsertest.cc
index f5c81eeb1bbae5eeabdd4a4138ab50d9d5e3a173..7314976722d61edbd5dab82d122d1e005180d7a8 100644
--- a/chrome/browser/image_decoder_browsertest.cc
+++ b/chrome/browser/image_decoder_browsertest.cc
@@ -4,25 +4,35 @@
 
 #include "chrome/browser/image_decoder.h"
 
-#if defined(OS_POSIX)
-#include <sys/types.h>
-#include <signal.h>
-#endif
-
+#include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/browser/browser_child_process_observer.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/child_process_data.h"
 #include "content/public/test/test_utils.h"
+#include "ui/base/l10n/l10n_util.h"
 
 using content::BrowserThread;
 
 namespace {
 
+std::string GetValidPngString() {
+  // 1x1 PNG. Does not get much smaller than this.
+  static const char kPngData[] =
+      "\x89\x50\x4e\x47\x0d\x0a\x1a\x0a\x00\x00\x00\x0d\x49\x48\x44\x52"
+      "\x00\x00\x00\x01\x00\x00\x00\x01\x08\x02\x00\x00\x00\x90\x77\x53"
+      "\xde\x00\x00\x00\x0c\x49\x44\x41\x54\x08\xd7\x63\xf8\xff\xff\x3f"
+      "\x00\x05\xfe\x02\xfe\xdc\xcc\x59\xe7\x00\x00\x00\x00\x49\x45\x4e"
+      "\x44\xae\x42\x60\x82";
+  // Need to specify the buffer size because it contains NULs.
+  return std::string(kPngData, sizeof(kPngData) - 1);
+}
+
 class TestImageRequest : public ImageDecoder::ImageRequest {
  public:
   explicit TestImageRequest(const base::Closure& quit_closure)
-      : quit_closure_(quit_closure),
+      : decode_succeeded_(false),
+        quit_closure_(quit_closure),
         quit_called_(false) {
   }
 
@@ -32,8 +42,11 @@ class TestImageRequest : public ImageDecoder::ImageRequest {
     }
   }
 
+  bool decode_succeeded() const { return decode_succeeded_; }
+
  private:
   void OnImageDecoded(const SkBitmap& decoded_image) override {
+    decode_succeeded_ = true;
     Quit();
   }
 
@@ -47,6 +60,8 @@ class TestImageRequest : public ImageDecoder::ImageRequest {
     quit_closure_.Run();
   }
 
+  bool decode_succeeded_;
+
   base::Closure quit_closure_;
   bool quit_called_;
 
@@ -55,7 +70,10 @@ class TestImageRequest : public ImageDecoder::ImageRequest {
 
 class KillProcessObserver : public content::BrowserChildProcessObserver {
  public:
-  KillProcessObserver() {
+  KillProcessObserver()
+      : did_kill_(false),
+        utility_process_name_(
+            l10n_util::GetStringUTF16(IDS_UTILITY_PROCESS_IMAGE_DECODER_NAME)) {
     Add(this);
   }
 
@@ -63,13 +81,19 @@ class KillProcessObserver : public content::BrowserChildProcessObserver {
     Remove(this);
   }
 
+  bool did_kill() const { return did_kill_; }
+
  private:
   void BrowserChildProcessHostConnected(
       const content::ChildProcessData& data) override {
-    base::ProcessHandle handle = data.handle;
-
-    if (handle == base::kNullProcessHandle)
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    if (data.handle == base::kNullProcessHandle ||
+        data.name != utility_process_name_) {
       return;
+    }
+
+    ASSERT_FALSE(did_kill_);
+    base::ProcessHandle handle = data.handle;
 
 #if defined(OS_WIN)
     // On windows, duplicate the process handle since base::Process closes it on
@@ -77,34 +101,22 @@ class KillProcessObserver : public content::BrowserChildProcessObserver {
     base::ProcessHandle out_handle;
     if (!::DuplicateHandle(GetCurrentProcess(), handle,
                            GetCurrentProcess(), &out_handle,
-                           0, FALSE, DUPLICATE_SAME_ACCESS))
+                           0, FALSE, DUPLICATE_SAME_ACCESS)) {
       return;
+    }
     handle = out_handle;
 #endif
 
-    EXPECT_TRUE(base::Process(handle).Terminate(0, true));
-  }
-};
-
-#if defined(OS_POSIX)
-class FreezeProcessObserver : public content::BrowserChildProcessObserver {
- public:
-  FreezeProcessObserver() {
-    Add(this);
+    // Use a non-zero exit code so it counts as a crash.
+    EXPECT_TRUE(base::Process(handle).Terminate(1, true));
+    did_kill_ = true;
   }
 
-  ~FreezeProcessObserver() override {
-    Remove(this);
-  }
+  bool did_kill_;
+  const base::string16 utility_process_name_;
 
- private:
-  void BrowserChildProcessHostConnected(
-      const content::ChildProcessData& data) override {
-    if (data.handle != base::kNullProcessHandle)
-      EXPECT_EQ(0, kill(data.handle, SIGSTOP));
-  }
+  DISALLOW_COPY_AND_ASSIGN(KillProcessObserver);
 };
-#endif  // defined(OS_POSIX)
 
 }  // namespace
 
@@ -117,36 +129,44 @@ IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, Basic) {
   TestImageRequest test_request(runner->QuitClosure());
   ImageDecoder::Start(&test_request, std::string());
   runner->Run();
+  EXPECT_FALSE(test_request.decode_succeeded());
 }
 
-IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, StartAndDestroy) {
+IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, BasicDecode) {
   scoped_refptr<content::MessageLoopRunner> runner =
       new content::MessageLoopRunner;
-  scoped_ptr<TestImageRequest> test_request(
-      new TestImageRequest(runner->QuitClosure()));
-  ImageDecoder::Start(test_request.get(), std::string());
-  test_request.reset();
+  TestImageRequest test_request(runner->QuitClosure());
+  ImageDecoder::Start(&test_request, GetValidPngString());
   runner->Run();
+  EXPECT_TRUE(test_request.decode_succeeded());
 }
 
-IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, StartAndKillProcess) {
-  KillProcessObserver observer;
+IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, StartAndDestroy) {
   scoped_refptr<content::MessageLoopRunner> runner =
       new content::MessageLoopRunner;
   scoped_ptr<TestImageRequest> test_request(
       new TestImageRequest(runner->QuitClosure()));
   ImageDecoder::Start(test_request.get(), std::string());
+  test_request.reset();
   runner->Run();
 }
 
-#if defined(OS_POSIX)
-IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, StartAndFreezeProcess) {
-  FreezeProcessObserver observer;
+// Killing the utility process counts as a crash. Thus the request fails.
+// If ImageDecoder did not handle the crash properly, the request never finishes
+// and this test would hang.
+// Note: This test is inherently racy because KillProcessObserver lives on the
+// UI thread but ImageDecoder does its work mainly on the IO thread. So the test
+// checks for both possible valid outcomes.
+IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, StartAndKillProcess) {
+  KillProcessObserver observer;
   scoped_refptr<content::MessageLoopRunner> runner =
       new content::MessageLoopRunner;
-  scoped_ptr<TestImageRequest> test_request(
-      new TestImageRequest(runner->QuitClosure()));
-  ImageDecoder::Start(test_request.get(), std::string());
+  TestImageRequest test_request(runner->QuitClosure());
+  ImageDecoder::Start(&test_request, GetValidPngString());
   runner->Run();
+  if (!test_request.decode_succeeded()) {
+    // The UI thread won the race. Make sure the utility process did get killed.
+    EXPECT_TRUE(observer.did_kill());
+  }
+  // Else the IO thread won the race and the image got decoded. Oh well.
 }
-#endif  // defined(OS_POSIX)