Fix race when setting Desktop window id.

This fix a potential race when setting the window id.

It also add a check to verify that a media::VideoCaptureDevice is not deleted on the wrong thread.

BUG=450382
Committed: https://crrev.com/0c9bb6edbc97d5ff103f544b2bc02ab4c569c5a2
Cr-Commit-Position: refs/heads/master@{#315381}

[perkj_chrome, 2015-02-05 17:36:16]

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

[perkj_chrome, 2015-02-05 17:36:17]

Hej
I have not been able to repro a crash with this CHECK but I think its worth
investigating if this is the root cause of the bug. 

WDYT? If you agree, please click the CQ.

[Sergey Ulanov, 2015-02-05 18:52:06]

https://codereview.chromium.org/899943004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/899943004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:285:
CHECK(entry->video_capture_device() == NULL);
I think it's better to add a CHECK in ~DesktopCaptureDevice(), in case there are
other code paths that can delete it on a wrong thread.

https://codereview.chromium.org/899943004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:656:
existing_device->video_capture_device(),
can this be NULL?

[perkj_chrome, 2015-02-06 13:04:26]

Hi, got spotting of the race when setting the window id. How about this?

https://codereview.chromium.org/899943004/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/899943004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:285:
CHECK(entry->video_capture_device() == NULL);
On 2015/02/05 18:52:06, Sergey Ulanov wrote:
> I think it's better to add a CHECK in ~DesktopCaptureDevice(), in case there
are
> other code paths that can delete it on a wrong thread.

How about in ~DeviceEntry to catch all types of capturers?

https://codereview.chromium.org/899943004/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/video_capture_manager.cc:656:
existing_device->video_capture_device(),
On 2015/02/05 18:52:06, Sergey Ulanov wrote:
> can this be NULL?

Hmm, maybe that is the issue. 
video_capture_device is set on the device thread when the capturer is started.
But if this is called just after StartCaptureForClient, video_capturer_device()
will return null and this would fail.

It worked better before since DeviceEntry* was then posted to the device thread
and thus that would never happen.

[perkj_chrome, 2015-02-06 13:30:42]

On 2015/02/06 13:04:26, perkj wrote:
> Hi, got spotting of the race when setting the window id. How about this?
> 
>
https://codereview.chromium.org/899943004/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/media/video_capture_manager.cc (right):
> 
>
https://codereview.chromium.org/899943004/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/media/video_capture_manager.cc:285:
> CHECK(entry->video_capture_device() == NULL);
> On 2015/02/05 18:52:06, Sergey Ulanov wrote:
> > I think it's better to add a CHECK in ~DesktopCaptureDevice(), in case there
> are
> > other code paths that can delete it on a wrong thread.
> 
> How about in ~DeviceEntry to catch all types of capturers?
> 
>
https://codereview.chromium.org/899943004/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/media/video_capture_manager.cc:656:
> existing_device->video_capture_device(),
> On 2015/02/05 18:52:06, Sergey Ulanov wrote:
> > can this be NULL?
> 
> Hmm, maybe that is the issue. 
> video_capture_device is set on the device thread when the capturer is started.
> But if this is called just after StartCaptureForClient,
video_capturer_device()
> will return null and this would fail.
> 
> It worked better before since DeviceEntry* was then posted to the device
thread
> and thus that would never happen.

And I can't spell. I meant, good spotting of the potential race...

[miu, 2015-02-06 21:49:21]

Be careful if you plan on committing this change with CHECKs in the destructors.
 A few months back, I was looking into shutdown issues in the
content/browser/media/capture/* code, and there I found it is possible for some
of the destructors to be run on the main thread AFTER the BrowserThreads are
destroyed.  See comments/code here for more details:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

[miu, 2015-02-06 21:51:47]

Oh, maybe never mind my last comment.  I see now nothing here is running on the
main/UI thread.

https://codereview.chromium.org/899943004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/899943004/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:116: // CHECK that
the media::VideoCaptureDevice is not
s/media::VideoCaptureDevice/media::VideoCaptureDevice::DeviceEntry/

[Sergey Ulanov, 2015-02-06 22:20:03]

https://codereview.chromium.org/899943004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/899943004/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:118:
CHECK(video_capture_device_ == NULL);
s/NULL/nullptr/, or CHECK(!video_capture_device_)

https://codereview.chromium.org/899943004/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:666:
notification_window_ids_[session_id]));
I think you also want to remove the value from notification_window_ids_.
Otherwise it will grow indefinitely.

[perkj_chrome, 2015-02-09 08:27:41]

PTAL

https://codereview.chromium.org/899943004/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/899943004/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:116: // CHECK that
the media::VideoCaptureDevice is not
On 2015/02/06 21:51:47, miu wrote:
> s/media::VideoCaptureDevice/media::VideoCaptureDevice::DeviceEntry/

I want to check that the entry does not own a media::VideoCaptureDevice when its
deleted. Refrased

https://codereview.chromium.org/899943004/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:118:
CHECK(video_capture_device_ == NULL);
On 2015/02/06 22:20:03, Sergey Ulanov wrote:
> s/NULL/nullptr/, or CHECK(!video_capture_device_)

ok. I am pretty sure the race is the problem that causes the crash. So I change
this to a DCHECK as well.

https://codereview.chromium.org/899943004/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:666:
notification_window_ids_[session_id]));
On 2015/02/06 22:20:03, Sergey Ulanov wrote:
> I think you also want to remove the value from notification_window_ids_.
> Otherwise it will grow indefinitely.

ok, so this will fix an old bug as well? It looks like that has been the case
for some time.

[Sergey Ulanov, 2015-02-09 19:20:08]

lgtm

[perkj_chrome, 2015-02-09 19:36:39]

The CQ bit was checked by perkj@chromium.org

[commit-bot: I haz the power, 2015-02-09 19:37:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/899943004/40001

[commit-bot: I haz the power, 2015-02-09 20:44:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-09 20:45:06]

Patchset 3 (id:??) landed as
https://crrev.com/0c9bb6edbc97d5ff103f544b2bc02ab4c569c5a2
Cr-Commit-Position: refs/heads/master@{#315381}