[DesktopCaptureDevice] Remove the if no client_ check

content/browser/media/capture/desktop_capture_device.cc

 // Copyright (c) 2013 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 "content/browser/media/capture/desktop_capture_device.h"
 
 #include <stddef.h>
 #include <stdint.h>
 #include <string.h>
 #include <utility>
@@ skipping 199 matching lines @@
     first_capture_returned_ = true;
     if (capturer_type_ == DesktopMediaID::TYPE_SCREEN) {
       IncrementDesktopCaptureCounter(success ? FIRST_SCREEN_CAPTURE_SUCCEEDED
                                              : FIRST_SCREEN_CAPTURE_FAILED);
     } else {
       IncrementDesktopCaptureCounter(success ? FIRST_WINDOW_CAPTURE_SUCCEEDED
                                              : FIRST_WINDOW_CAPTURE_FAILED);
     }
   }
 
+  if (!client_)

[Sergey Ulanov, 2017/01/13 07:49:03]

Why do we need this check? As far as I can tell client_ is reset only in the
destructor. Is OnCaptureResult() being called after Core is destroyed? If so,
then this code is still unsafe. The capturer must ensure that it doesn't call
OnCaptureResult after it's destroyed.

[Hzj_jie, 2017/01/13 22:10:23]

No, as I mentioned in the code review, this change is still not safe. But it
resolves part of the issue. My original thought is to use WeakPtr (Sorry for the
typo in the description). But a backup solution without WeakPtr is to separate
destructor of Core with the deletion. i.e. In
DesktopCaptureDevice::StopAndDeAllocate() {
  if (core_) {
    thread_.task_runner()->PostTask(FROM_HERE, base::Bind(&Core::Stop,
core_.release());
  }
}

In Core::Stop() {
  client_.reset();
  output_frame_.reset();
  previous_frame_size_.set(0, 0);
  desktop_capturer_.reset();
  task_runner_->DeleteSoon(FROM_HERE, this);
}

The issue here is, before desktop_capture_.reset() has been called,
DesktopCapturer is always able to push a callback of OnCaptureResult() in the
thread. So in the thread there may be,
CaptureFrame() -> Destructor(pushed by StopAndDeAllocate()) ->
OnCaptureResult(pushed by CaptureFrame()).
We do not have this scenario for now, as all the DesktopCapturers synchronously
call OnCaptureResult() in CaptureFrame() function. But as far as we design the
interface as this, the consumer should prepare for it.

+    return;
+
   if (!success) {
     if (result == webrtc::DesktopCapturer::Result::ERROR_PERMANENT)
       client_->OnError(FROM_HERE, "The desktop capturer has failed.");
     return;
   }
   DCHECK(frame);
 
-  if (!client_)
-    return;
-
   base::TimeDelta capture_time(
       base::TimeDelta::FromMilliseconds(frame->capture_time_ms()));
 
   // The two UMA_ blocks must be put in its own scope since it creates a static
   // variable which expected constant histogram name.
   if (capturer_type_ == DesktopMediaID::TYPE_SCREEN) {
     UMA_HISTOGRAM_TIMES(kUmaScreenCaptureTime, capture_time);
   } else {
     UMA_HISTOGRAM_TIMES(kUmaWindowCaptureTime, capture_time);
   }
@@ skipping 222 matching lines @@
 #else
   base::MessageLoop::Type thread_type = base::MessageLoop::TYPE_DEFAULT;
 #endif
 
   thread_.StartWithOptions(base::Thread::Options(thread_type, 0));
 
   core_.reset(new Core(thread_.task_runner(), std::move(capturer), type));
 }
 
 }  // namespace content