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..1cde39f1767fba9ea8a8f5d40c85c98d69146e8b 100644
--- a/chrome/browser/image_decoder_browsertest.cc
+++ b/chrome/browser/image_decoder_browsertest.cc
@@ -9,20 +9,35 @@
 #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 +47,11 @@ class TestImageRequest : public ImageDecoder::ImageRequest {
     }
   }
 
+  const bool decode_succeeded() const { return decode_succeeded_; }
+
  private:
   void OnImageDecoded(const SkBitmap& decoded_image) override {
+    decode_succeeded_ = true;
     Quit();
   }
 
@@ -47,6 +65,8 @@ class TestImageRequest : public ImageDecoder::ImageRequest {
     quit_closure_.Run();
   }
 
+  bool decode_succeeded_;
+
   base::Closure quit_closure_;
   bool quit_called_;
 
@@ -55,7 +75,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 +86,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,19 +106,30 @@ 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));

[Lei Zhang, 2015/05/07 00:24:13]

0 is "exit normally" on Windows -> OnProcessCrashed() does not get called.

+    // Use a non-zero exit code so it counts as a crash.
+    EXPECT_TRUE(base::Process(handle).Terminate(1, true));
+    did_kill_ = true;
   }
+
+  bool did_kill_;
+  const base::string16 utility_process_name_;
+
+  DISALLOW_COPY_AND_ASSIGN(KillProcessObserver);
 };
 
 #if defined(OS_POSIX)
 class FreezeProcessObserver : public content::BrowserChildProcessObserver {
  public:
-  FreezeProcessObserver() {
+  FreezeProcessObserver()
+      : did_freeze_(false),
+        utility_process_name_(
+            l10n_util::GetStringUTF16(IDS_UTILITY_PROCESS_IMAGE_DECODER_NAME)) {
     Add(this);
   }
 
@@ -97,12 +137,36 @@ class FreezeProcessObserver : public content::BrowserChildProcessObserver {
     Remove(this);
   }
 
+  bool did_freeze() const { return did_freeze_; }
+
  private:
+  static void ResumeFrozenProcess(const base::ProcessHandle& handle) {
+    DCHECK_NE(handle, base::kNullProcessHandle);
+    EXPECT_EQ(0, kill(handle, SIGCONT));

[Anand Mistry (off Chromium), 2015/05/07 05:35:22]

I'm not 100% sure, but I suspect there's a race here similar to the
StartAndKillProcess case. Consider:
1. Start decoding an image, which starts a utility process and sends the
request.
2. Process creation posts the observer callback to the UI thread.
3. Decoding finishes and result is received.
4. UI thread runs observer.
5. ImageDecoder timeout fires and kills the utility process since there are no
outstanding requests.
6. This function is run and kill(SIGCONT) returns -1 with errno = ESRCH because
the process doesn't exist.

[Lei Zhang, 2015/05/07 22:31:05]

How do you feel if I just removed this test? It no longer tests what we
originally wanted it to test, and trying to fix the raciness is more work than
what I'd like to do here.

[Anand Mistry (off Chromium), 2015/05/08 00:02:10]

That sounds fine.

+  }
+
   void BrowserChildProcessHostConnected(
       const content::ChildProcessData& data) override {
-    if (data.handle != base::kNullProcessHandle)
-      EXPECT_EQ(0, kill(data.handle, SIGSTOP));
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    if (data.handle == base::kNullProcessHandle ||
+        data.name != utility_process_name_) {
+      return;
+    }
+
+    ASSERT_FALSE(did_freeze_);
+    did_freeze_ = true;
+    EXPECT_EQ(0, kill(data.handle, SIGSTOP));
+    BrowserThread::PostDelayedTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&FreezeProcessObserver::ResumeFrozenProcess, data.handle),
+        base::TimeDelta::FromSeconds(7));
   }
+
+  bool did_freeze_;
+  const base::string16 utility_process_name_;
+
+  DISALLOW_COPY_AND_ASSIGN(FreezeProcessObserver);
 };
 #endif  // defined(OS_POSIX)
 
@@ -117,6 +181,16 @@ 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, BasicDecode) {
+  scoped_refptr<content::MessageLoopRunner> runner =
+      new content::MessageLoopRunner;
+  TestImageRequest test_request(runner->QuitClosure());
+  ImageDecoder::Start(&test_request, GetValidPngString());
+  runner->Run();
+  EXPECT_TRUE(test_request.decode_succeeded());
 }
 
 IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, StartAndDestroy) {
@@ -129,24 +203,36 @@ IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, StartAndDestroy) {
   runner->Run();
 }
 
+// Killing the utility process counts as a crash. Thus the request fails.
+// 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.

[Anand Mistry (off Chromium), 2015/05/07 05:35:22]

I should have commented this when I wrote this test, but the goal was to test
that requests aren't left dangling when the utility process crashes (and hence
would time out the test). Not to test that the request fails, which as you
mention, is racy. I think it should be added to this comment so that "future
folk" don't get confused.

[Lei Zhang, 2015/05/07 22:36:22]

Done.

 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.
 }
 
 #if defined(OS_POSIX)
+// Freezing the utility process just long enough that it would try to end batch
+// mode. However, the outstanding request prevents that. The request should
+// eventually finish.
 IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, StartAndFreezeProcess) {
   FreezeProcessObserver 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();
+  EXPECT_TRUE(observer.did_freeze());
+  EXPECT_TRUE(test_request.decode_succeeded());
 }
 #endif  // defined(OS_POSIX)