Fix crash when terminating a task in the task manager with a PID of 0

Simple guard to prevent tasks with pid 0 from being killable.
If a user tries to kill a task with pid 0 there is a crash.
By doing a simple check to see if the pid is 0 we prevent this. A pid
should also only be 0 for a short period of time in certain race
conditions.

Bug: 749459
Change-Id: Iba63b63d6606dbf403cbae3bf2fef4c882f395c5
Reviewed-on: https://chromium-review.googlesource.com/594308Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Commit-Queue: Charles Burnell <cburn@google.com>
Cr-Commit-Position: refs/heads/master@{#491424}

chrome/browser/task_manager/providers/task.cc

@@ -69,10 +69,17 @@ void Task::Activate() {
 }
 
 bool Task::IsKillable() {
+  // Protects from trying to kill a task that doesn't have an accurate process
+  // Id yet. This can result in calling "kill 0" which kills all processes in
+  // the process group.
+  if (process_id() == base::kNullProcessId)
+    return false;
   return true;
 }
 
 void Task::Kill() {
+  if (!IsKillable())
+    return;
   DCHECK_NE(process_id(), base::GetCurrentProcId());
   base::Process process = base::Process::Open(process_id());
   process.Terminate(content::RESULT_CODE_KILLED, false);

chrome/browser/task_manager/sampling/task_group_unittest.cc

@@ -342,7 +342,7 @@ TEST_F(TaskGroupTest, NetworkBytesReadAsGroup) {
 }
 
 // Tests that the network usage rate does not get affected until a refresh is
-// called and that the cumulative is as up to date as possible
+// called and that the cumulative is as up to date as possible.
 TEST_F(TaskGroupTest, NetworkBytesTransferredRefreshOutOfOrder) {
   const int read_bytes = 1024;
   const int sent_bytes = 1;
@@ -429,7 +429,7 @@ TEST_F(TaskGroupTest, NetworkBytesTransferredAsGroup) {
 
 // Tests that after two tasks in a task group read bytes that a refresh will
 // zero out network usage rate while maintaining the correct cumulative network
-// usage
+// usage.
 TEST_F(TaskGroupTest, NetworkBytesReadAsGroupThenNone) {
   const int read_bytes1 = 1013;
   const int read_bytes2 = 679;
@@ -459,7 +459,7 @@ TEST_F(TaskGroupTest, NetworkBytesReadAsGroupThenNone) {
 
 // Tests that after two tasks in a task group send bytes that a refresh will
 // zero out network usage rate while maintaining the correct cumulative network
-// usage
+// usage.
 TEST_F(TaskGroupTest, NetworkBytesSentAsGroupThenNone) {
   const int sent_bytes1 = 1023;
   const int sent_bytes2 = 678;
@@ -489,7 +489,7 @@ TEST_F(TaskGroupTest, NetworkBytesSentAsGroupThenNone) {
 
 // Tests that after two tasks in a task group transferred bytes that a refresh
 // will zero out network usage rate while maintaining the correct cumulative
-// network usage
+// network usage.
 TEST_F(TaskGroupTest, NetworkBytesTransferredAsGroupThenNone) {
   const int read_bytes = 321;
   const int sent_bytes = 987;
@@ -517,4 +517,10 @@ TEST_F(TaskGroupTest, NetworkBytesTransferredAsGroupThenNone) {
             task_group_.cumulative_per_process_network_usage());
 }
 
+// Test the task can't be killed with a PID of base::kNullProcessId.
+TEST_F(TaskGroupTest, TaskWithPidZero) {
+  FakeTask fake_task(base::kNullProcessId, Task::RENDERER);
+  EXPECT_FALSE(fake_task.IsKillable());
+}
+
 }  // namespace task_manager