Fix ImageDecoderBrowserTest.StartAndKillProcess flakyness and re-enable.

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"
 
 #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"
@@ skipping 90 matching lines @@
     base::ProcessHandle out_handle;
     if (!::DuplicateHandle(GetCurrentProcess(), handle,
                            GetCurrentProcess(), &out_handle,
                            0, FALSE, DUPLICATE_SAME_ACCESS)) {
       return;
     }
     handle = out_handle;
 #endif
 
     // Use a non-zero exit code so it counts as a crash.
-    EXPECT_TRUE(base::Process(handle).Terminate(1, true));
+    // Don't wait for the process after sending the kill signal. According to

[Lei Zhang, 2015/05/11 23:09:19]

By 'kill signal', did you mean SIGTERM?

[Anand Mistry (off Chromium), 2015/05/12 00:05:31]

Yes.

+    // POSIX, doing so causes the resulting zombie to be removed from the
+    // process table. However, Chromium treats an error on |waitpid| (in this
+    // case, ECHILD) as a "normal" termination and doesn't invoke the process
+    // host detegate's |OnProcessCrashed|.

[Lei Zhang, 2015/05/11 23:09:19]

nit: typo

[Anand Mistry (off Chromium), 2015/05/12 00:05:31]

Done.

+    EXPECT_TRUE(base::Process(handle).Terminate(1, false));

[Lei Zhang, 2015/05/11 23:09:19]

Maybe we should post this to the IO thread, so the kill() calls don't race
against each other.

[Anand Mistry (off Chromium), 2015/05/12 00:05:31]

I don't think it's possible for the two to race. If this functions runs, it
means the UI message loop is still looping, which means the request's response
functions haven't been called (and quit the loop), which means there are still
requests in the image decoder's request map, which means the image decoder won't
try to kill the decoder process. Phew, that was a mouthful.

[Lei Zhang, 2015/05/12 02:23:24]

I was thinking if we had left this as true, then the code runs waitpid() on the
UI thread, but at the same time, the IO thread is calling
GetChildTerminationStatus() which may also involve calling kill() and waitpid().
But ya, I guess we don't really need to wait, so n/m.

     did_kill_ = true;
   }
 
   bool did_kill_;
   const base::string16 utility_process_name_;
 
   DISALLOW_COPY_AND_ASSIGN(KillProcessObserver);
 };
 
 }  // namespace
@@ skipping 28 matching lines @@
   test_request.reset();
   runner->Run();
 }
 
 // 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.
-// BUG(486194): Disabled due to flakyness.
-IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, DISABLED_StartAndKillProcess) {
+IN_PROC_BROWSER_TEST_F(ImageDecoderBrowserTest, StartAndKillProcess) {
   KillProcessObserver observer;
   scoped_refptr<content::MessageLoopRunner> runner =
       new content::MessageLoopRunner;
   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.
 }