[DesktopCaptureDevice] Remove the if no client_ check

[DesktopCaptureDevice] Remove the if no client_ check

DesktopCaptureDevice is not designed to work with asynchronous implementation
of DesktopCapturer. So the if (!client_) check in OnCaptureResult() is not
reasonable and confusing.

BUG=681145
Review-Url: https://codereview.chromium.org/2625083006
Cr-Commit-Position: refs/heads/master@{#444541}
Committed: https://chromium.googlesource.com/chromium/src/+/c4f77c059dcf999c6411ce18d0312dd8f46774bf

[Hzj_jie, 2017-01-13 02:30:43]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 02:31:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-01-13 02:40:19]

Description was changed from

==========
[DesktopCaptureDevice] Check client_ before using it

This code defect does not really like to be the root cause of the bug, but it's
worth to fix it.
And this fix is also not a safe solution, using variables of instance after it
has been deleted is not always safe. IMO, DesktopCaptureDevice::Core should
inherit base::RefCountedThreadSafe.

BUG=680027
==========

to

==========
[DesktopCaptureDevice] Check client_ before using it

This fix is also not a safe solution, using variables of instance after it has
been deleted is not always safe. IMO, DesktopCaptureDevice::Core should
inherit base::RefCountedThreadSafe.

BUG=680027
==========

[Hzj_jie, 2017-01-13 02:40:20]

zijiehe@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Hzj_jie, 2017-01-13 02:48:30]

zijiehe@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[commit-bot: I haz the power, 2017-01-13 03:59:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 03:59:02]

Dry run: This issue passed the CQ dry run.

[miu, 2017-01-13 06:52:31]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2017-01-13 06:52:31]

lgtm, but note: Ref-counting is *not* a default solution to problems like these.
The Chromium project specifically recommends ref-counting be avoided, and
discusses why here: https://www.chromium.org/developers/smart-pointer-guidelines

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

I'm not sure how this change is related to the linked bug. The crash in that bug
is in Magnification.dll.

https://codereview.chromium.org/2625083006/diff/1/content/browser/media/captu...
File content/browser/media/capture/desktop_capture_device.cc (right):

https://codereview.chromium.org/2625083006/diff/1/content/browser/media/captu...
content/browser/media/capture/desktop_capture_device.cc:220: if (!client_)
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.

[Sergey Ulanov, 2017-01-13 07:52:39]

On 2017/01/13 06:52:31, miu wrote:
> lgtm, but note: Ref-counting is *not* a default solution to problems like
these.
> The Chromium project specifically recommends ref-counting be avoided, and
> discusses why here:
https://www.chromium.org/developers/smart-pointer-guidelines

DesktopCaptureDevice::Core is actually single-threaded (except for creation)
with very simple ownership semantics, so ref-counting would not be useful here
at all.

[Hzj_jie, 2017-01-13 20:17:05]

Description was changed from

==========
[DesktopCaptureDevice] Check client_ before using it

This fix is also not a safe solution, using variables of instance after it has
been deleted is not always safe. IMO, DesktopCaptureDevice::Core should
inherit base::RefCountedThreadSafe.

BUG=680027
==========

to

==========
[DesktopCaptureDevice] Check client_ before using it

This fix is also not a safe solution, using variables of instance after it has
been deleted is not always safe. IMO, DesktopCaptureDevice::Core should be
protected by a WeakPtr.

BUG=680027
==========

[Hzj_jie, 2017-01-13 20:17:24]

On 2017/01/13 07:52:39, Sergey Ulanov wrote:
> On 2017/01/13 06:52:31, miu wrote:
> > lgtm, but note: Ref-counting is *not* a default solution to problems like
> these.
> > The Chromium project specifically recommends ref-counting be avoided, and
> > discusses why here:
> https://www.chromium.org/developers/smart-pointer-guidelines
> 
> DesktopCaptureDevice::Core is actually single-threaded (except for creation)
> with very simple ownership semantics, so ref-counting would not be useful here
> at all.

Sorry, I mean WeakPtr instead of RefCounted.

[Hzj_jie, 2017-01-13 20:26:53]

On 2017/01/13 20:17:24, Hzj_jie wrote:
> On 2017/01/13 07:52:39, Sergey Ulanov wrote:
> > On 2017/01/13 06:52:31, miu wrote:
> > > lgtm, but note: Ref-counting is *not* a default solution to problems like
> > these.
> > > The Chromium project specifically recommends ref-counting be avoided, and
> > > discusses why here:
> > https://www.chromium.org/developers/smart-pointer-guidelines
> > 
> > DesktopCaptureDevice::Core is actually single-threaded (except for creation)
> > with very simple ownership semantics, so ref-counting would not be useful
here
> > at all.
> 
> Sorry, I mean WeakPtr instead of RefCounted.

No, the bug does not relate to the change, I was so lazy to create a new bug.

[Hzj_jie, 2017-01-13 20:30:28]

Description was changed from

==========
[DesktopCaptureDevice] Check client_ before using it

This fix is also not a safe solution, using variables of instance after it has
been deleted is not always safe. IMO, DesktopCaptureDevice::Core should be
protected by a WeakPtr.

BUG=680027
==========

to

==========
[DesktopCaptureDevice] Check client_ before using it

This fix is also not a safe solution, using variables of instance after it has
been deleted is not always safe. IMO, DesktopCaptureDevice::Core should be
protected by a WeakPtr.

BUG=681145
==========

[Hzj_jie, 2017-01-13 20:38:09]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 20:38:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-01-13 20:43:03]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 20:43:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-01-13 20:43:46]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-13 21:01:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 21:01:25]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hzj_jie, 2017-01-13 21:31:09]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2017-01-13 21:31:18]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-01-13 21:31:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2625083006/diff/1/content/browser/media/captu...
File content/browser/media/capture/desktop_capture_device.cc (right):

https://codereview.chromium.org/2625083006/diff/1/content/browser/media/captu...
content/browser/media/capture/desktop_capture_device.cc:220: if (!client_)
On 2017/01/13 07:49:03, Sergey Ulanov wrote:
> 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.

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.

[commit-bot: I haz the power, 2017-01-13 22:37:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 22:37:23]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-01-17 22:53:44]

On 2017/01/13 22:10:23, Hzj_jie wrote:
> 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.

Even if capturers call OnCaptureResult() asynchronously they still must
guarantee that the callback is not called after the capturer is destroyed. If
OnCaptureResult() is ever called after the destructor then it's a bug in the
capturer and should be fixed there. Otherwise the current DesktopCapturer would
not be safe as it uses raw pointer for callback_.

BTW, note that DesktopCaptureDevice currently doesn't support asynchronous
capturers at all, there are DCHECKS for that.

[Hzj_jie, 2017-01-17 23:24:22]

On 2017/01/17 22:53:44, Sergey Ulanov wrote:
> On 2017/01/13 22:10:23, Hzj_jie wrote:
> > 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.
> 
> Even if capturers call OnCaptureResult() asynchronously they still must
> guarantee that the callback is not called after the capturer is destroyed. If
> OnCaptureResult() is ever called after the destructor then it's a bug in the
> capturer and should be fixed there. Otherwise the current DesktopCapturer
would
> not be safe as it uses raw pointer for callback_.
> 
> BTW, note that DesktopCaptureDevice currently doesn't support asynchronous
> capturers at all, there are DCHECKS for that.

I have not noticed the DCHECK. Thank you.

[Hzj_jie, 2017-01-17 23:24:34]

zijiehe@chromium.org changed reviewers:
- jamiewalch@chromium.org, miu@chromium.org, sergeyu@chromium.org

[Hzj_jie, 2017-01-17 23:28:43]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2017-01-17 23:28:56]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-01-17 23:29:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-01-17 23:42:22]

Description was changed from

==========
[DesktopCaptureDevice] Check client_ before using it

This fix is also not a safe solution, using variables of instance after it has
been deleted is not always safe. IMO, DesktopCaptureDevice::Core should be
protected by a WeakPtr.

BUG=681145
==========

to

==========
[DesktopCaptureDevice] Remove the if no client_ check

DesktopCaptureDevice is not designed to work with asynchronized implementation
of DesktopCapturer. So the if (!client_) check in OnCaptureResult() is not
reasonable and confusing.

BUG=681145
==========

[Hzj_jie, 2017-01-17 23:42:22]

zijiehe@chromium.org changed reviewers:
+ miu@chromium.org, sergeyu@chromium.org

[Hzj_jie, 2017-01-17 23:43:20]

On 2017/01/17 23:24:22, Hzj_jie wrote:
> On 2017/01/17 22:53:44, Sergey Ulanov wrote:
> > On 2017/01/13 22:10:23, Hzj_jie wrote:
> > > 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.
> > 
> > Even if capturers call OnCaptureResult() asynchronously they still must
> > guarantee that the callback is not called after the capturer is destroyed.
If
> > OnCaptureResult() is ever called after the destructor then it's a bug in the
> > capturer and should be fixed there. Otherwise the current DesktopCapturer
> would
> > not be safe as it uses raw pointer for callback_.
> > 
> > BTW, note that DesktopCaptureDevice currently doesn't support asynchronous
> > capturers at all, there are DCHECKS for that.
> 
> I have not noticed the DCHECK. Thank you.

It looks like we should replace the if-check with a DCHECK, which is confusing.

[Hzj_jie, 2017-01-17 23:43:49]

Description was changed from

==========
[DesktopCaptureDevice] Remove the if no client_ check

DesktopCaptureDevice is not designed to work with asynchronized implementation
of DesktopCapturer. So the if (!client_) check in OnCaptureResult() is not
reasonable and confusing.

BUG=681145
==========

to

==========
[DesktopCaptureDevice] Remove the if no client_ check

DesktopCaptureDevice is not designed to work with asynchronous implementation
of DesktopCapturer. So the if (!client_) check in OnCaptureResult() is not
reasonable and confusing.

BUG=681145
==========

[commit-bot: I haz the power, 2017-01-18 00:34:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 00:34:41]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-01-18 23:14:34]

lgtm

[Sergey Ulanov, 2017-01-18 23:14:39]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2017-01-18 23:14:39]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2625083006/#ps100001
(title: "Resolve review comments")

[commit-bot: I haz the power, 2017-01-18 23:15:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 23:24:37]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484781279575440,
"parent_rev": "f9744eae0a8431c08c291a3936d85532931d7a4f", "commit_rev":
"c4f77c059dcf999c6411ce18d0312dd8f46774bf"}

[commit-bot: I haz the power, 2017-01-18 23:25:11]

Description was changed from

==========
[DesktopCaptureDevice] Remove the if no client_ check

DesktopCaptureDevice is not designed to work with asynchronous implementation
of DesktopCapturer. So the if (!client_) check in OnCaptureResult() is not
reasonable and confusing.

BUG=681145
==========

to

==========
[DesktopCaptureDevice] Remove the if no client_ check

DesktopCaptureDevice is not designed to work with asynchronous implementation
of DesktopCapturer. So the if (!client_) check in OnCaptureResult() is not
reasonable and confusing.

BUG=681145

Review-Url: https://codereview.chromium.org/2625083006
Cr-Commit-Position: refs/heads/master@{#444541}
Committed:
https://chromium.googlesource.com/chromium/src/+/c4f77c059dcf999c6411ce18d031...
==========

[commit-bot: I haz the power, 2017-01-18 23:25:12]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c4f77c059dcf999c6411ce18d031...