[DesktopCaptureDevice] Remove the if no client_ check

content/browser/media/capture/desktop_capture_device.cc

Index: content/browser/media/capture/desktop_capture_device.cc
diff --git a/content/browser/media/capture/desktop_capture_device.cc b/content/browser/media/capture/desktop_capture_device.cc
index 18a0934000258a1fe3b0743fe737ffb5846e8378..2c5407f87796338d625c91893142d76cad6119fd 100644
--- a/content/browser/media/capture/desktop_capture_device.cc
+++ b/content/browser/media/capture/desktop_capture_device.cc
@@ -217,6 +217,9 @@ void DesktopCaptureDevice::Core::OnCaptureResult(
     }
   }
 
+  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.");
@@ -224,9 +227,6 @@ void DesktopCaptureDevice::Core::OnCaptureResult(
   }
   DCHECK(frame);
 
-  if (!client_)
-    return;
-
   base::TimeDelta capture_time(
       base::TimeDelta::FromMilliseconds(frame->capture_time_ms()));