Wait for pending requests to finish in ImageDecoder.

chrome/browser/image_decoder_browsertest.cc

 // Copyright 2015 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/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) {
   }
 
   ~TestImageRequest() override {
     if (!quit_called_) {
       quit_closure_.Run();
     }
   }
 
+  const bool decode_succeeded() const { return decode_succeeded_; }
+
  private:
   void OnImageDecoded(const SkBitmap& decoded_image) override {
+    decode_succeeded_ = true;
     Quit();
   }
 
   void OnDecodeImageFailed() override {
     Quit();
   }
 
   void Quit() {
     EXPECT_FALSE(quit_called_);
     quit_called_ = true;
     quit_closure_.Run();
   }
 
+  bool decode_succeeded_;
+
   base::Closure quit_closure_;
   bool quit_called_;
 
   DISALLOW_COPY_AND_ASSIGN(TestImageRequest);
 };
 
 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);
   }
 
   ~KillProcessObserver() override {
     Remove(this);
   }
 
+  bool did_kill() const { return did_kill_; }
+
  private:
   void BrowserChildProcessHostConnected(
       const content::ChildProcessData& data) override {
+    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 (handle == base::kNullProcessHandle)
-      return;
-
 #if defined(OS_WIN)
     // On windows, duplicate the process handle since base::Process closes it on
     // destruction.
     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);
   }
 
   ~FreezeProcessObserver() override {
     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)
 
 }  // namespace
 
 class ImageDecoderBrowserTest : public InProcessBrowserTest {
 };
 
 IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, Basic) {
   scoped_refptr<content::MessageLoopRunner> runner =
       new content::MessageLoopRunner;
   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) {
   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();
 }
 
+// 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)