Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
130253002:
Attempt to fixed crash on stop mirroring by stopping render thread on a worker thread. (Closed)
|
Created:
6 years, 11 months ago by imcheng Modified:
6 years, 11 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAttempt to fixed crash on stop mirroring by stopping render thread on a worker thread.
BUG=324454
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244876
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed Alpha's comments #Patch Set 3 : added sequence token for executing render thread join using blocking pool #Patch Set 4 : Fixed compile for desktop_capture_device_aura.cc. Removed the use of StopOnWorkerThread and use Del… #Patch Set 5 : Removed sequence token usage since thread is always detached when being stopped #Patch Set 6 : Fix compile desktop_capture_device_aura.cc #
Total comments: 2
Patch Set 7 : Create render_thread_ during Start() instead of ctor / Stop() #
Total comments: 4
Patch Set 8 : Rebase #Patch Set 9 : Addressed miu's comments #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_device_impl.cc (right): https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_device_impl.cc:275: base::Passed(&capture_machine_)))); Not sure if this is problematic here - will VideoCaptureMachine::Stop be holding a valid reference to the capture machine? https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:608: render_thread_->StopSoon(); Not sure if needed? Seems the general advice is to avoid using StopSoon() as it was created as a very specific workaround for a Windows issue.
Wanted to send this out first as I was already working on this.
https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_device_impl.cc (right): https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_device_impl.cc:275: base::Passed(&capture_machine_)))); On 2014/01/09 01:15:17, imcheng1 wrote: > Not sure if this is problematic here - will VideoCaptureMachine::Stop be holding > a valid reference to the capture machine? when you call base::Bind(&DeleteCaptureMachineOnUIThread, base::Passed(&capture_machine_)) then the ownership of CaptureMachine is held by the callback created. The problem here is the mixed use of capture_machine_.get() and base::Passed(&capture_machine_). The latter clears the capture_machine_. The consequence is you'll get undefined behavior. What you can do is like this: VideoCaptureMachine* capture_machine_ptr = capture_machine_.get(); and then base::Unretained(capture_machine_ptr) and base::Passed(&capture_machine_) https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:113: if (render_thread) { DCHECK that render_thread is not NULL instead of doing if a if(){}. https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:122: if (render_thread) { Don't need this. Just call reset. https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:304: scoped_ptr<base::Thread> render_thread_; There's no need to change this. https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:574: BrowserThread::GetBlockingPool()->PostTask(FROM_HERE, This shouldn't be necessary. The thread has already been stopped. If not that's a problem we would like to know. https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:608: render_thread_->StopSoon(); On 2014/01/09 01:15:17, imcheng1 wrote: > Not sure if needed? Seems the general advice is to avoid using StopSoon() as it > was created as a very specific workaround for a Windows issue. That's fine if we don't call this. https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:613: base::Bind(&StopOnWorkerThread, render_thread_.get(), callback)); Just use &render_thread_ instead of render_thread_.get().
https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/video_capture_device_impl.cc (right): https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/video_capture_device_impl.cc:275: base::Passed(&capture_machine_)))); Thanks for the explanation. I changed the code to get the raw pointer first. On 2014/01/09 01:37:07, Alpha wrote: > On 2014/01/09 01:15:17, imcheng1 wrote: > > Not sure if this is problematic here - will VideoCaptureMachine::Stop be > holding > > a valid reference to the capture machine? > > when you call base::Bind(&DeleteCaptureMachineOnUIThread, > base::Passed(&capture_machine_)) then the ownership of CaptureMachine is held by > the callback created. > > The problem here is the mixed use of capture_machine_.get() and > base::Passed(&capture_machine_). The latter clears the capture_machine_. The > consequence is you'll get undefined behavior. > > What you can do is like this: > VideoCaptureMachine* capture_machine_ptr = capture_machine_.get(); > > and then > base::Unretained(capture_machine_ptr) and > base::Passed(&capture_machine_) https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:113: if (render_thread) { On 2014/01/09 01:37:07, Alpha wrote: > DCHECK that render_thread is not NULL instead of doing if a if(){}. Done. https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:122: if (render_thread) { On 2014/01/09 01:37:07, Alpha wrote: > Don't need this. Just call reset. Done. https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:304: scoped_ptr<base::Thread> render_thread_; On 2014/01/09 01:37:07, Alpha wrote: > There's no need to change this. See comment below https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:574: BrowserThread::GetBlockingPool()->PostTask(FROM_HERE, On 2014/01/09 01:37:07, Alpha wrote: > This shouldn't be necessary. The thread has already been stopped. If not that's > a problem we would like to know. See comment below https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:608: render_thread_->StopSoon(); On 2014/01/09 01:37:07, Alpha wrote: > On 2014/01/09 01:15:17, imcheng1 wrote: > > Not sure if needed? Seems the general advice is to avoid using StopSoon() as > it > > was created as a very specific workaround for a Windows issue. > > That's fine if we don't call this. ACK, removed. https://codereview.chromium.org/130253002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/web_contents_video_capture_device.cc:613: base::Bind(&StopOnWorkerThread, render_thread_.get(), callback)); On 2014/01/09 01:37:07, Alpha wrote: > Just use &render_thread_ instead of render_thread_.get(). I am a bit concerned about render_thread_ being invalid by the time the task is being executed if: - we don't call post task to call DeleteOnWorkerThread during ~WebContentsCaptureMachine() - render_thread_ is simply a base::Thread as opposed to a scoped_ptr that can be passed If the sequence of events go like the following: WebContentsCaptureMachine::Stop() - this posts a task to the blocking pool to call StopOnWorkerThread ~WebContentsCaptureMachine() - if task hasn't executed, this will try to stop render_thread_ on UI thread which is invalid. Even if it worked, the task would be holding on to an invalid pointer. So it seems like a race condition here.
I added a sequence token to be passed to the blocking pool for doing render thread joining. The reason is that both VideoCaptureDeviceImpl::StopAndDeallocate() and ~VideoCaptureDeviceImpl() post a task to the blocking pool to stop/destruct the render thread respectively. Both end up calling PlatformThread::Join(). If the tasks are not posted with a sequence token they might end up executing in parallel. If one task is already waiting on PlatformThread::Join() the other task would fail (error code 22, which seems to be EINVAL) when it tries to do the same thing (more specifically, pthread_join). So the solution I have is have the blocking pool wait for the first task to finish joining first, and only then start the second join task (which should be no-op since thread is already stopped)
LGTM.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/130253002/140001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/130253002/140001
FYI, I made some changes to this CL. Previously calling WebContentsCaptureMachine::Stop() will post a task to stop render_thread_ but not detach it from WebContentsCaptureMachine. This creates a race when caller tries to restart the machine -- if render_thread_ has not been joined yet then calling render_thread_.Start() will blow up. To get around this I made WebContentsCaptureMachine::Stop() deal with render_thread_ the same way as it does in the dtor, i.e. detach render_thread_ and destruct it in UI thread, and then reset render_thread_ with a new thread. With that I made another change to WebContentsCaptureMachine::DidCopyFromWorkingStore() - since this can be called after render_thread_ has been reset, I added an NULL check against render_thread_->message_loop_proxy(). If it is NULL then the machine has already stopped and the frame will be skipped.
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
+sergeyu for content/browser/renderer_host/media/desktop_capture_device_aura.cc
https://codereview.chromium.org/130253002/diff/470002/content/browser/rendere... File content/browser/renderer_host/media/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/130253002/diff/470002/content/browser/rendere... content/browser/renderer_host/media/web_contents_video_capture_device.cc:609: render_thread_.reset(new base::Thread(kRenderThreadName)); Seems strange to create a new thread as part of the ...Machine::Stop() operation. Can we just eliminate creating the render_thread_ (from everywhere), and then only create it in ...Machine::Start()?
https://codereview.chromium.org/130253002/diff/470002/content/browser/rendere... File content/browser/renderer_host/media/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/130253002/diff/470002/content/browser/rendere... content/browser/renderer_host/media/web_contents_video_capture_device.cc:609: render_thread_.reset(new base::Thread(kRenderThreadName)); On 2014/01/11 03:48:36, miu wrote: > Seems strange to create a new thread as part of the ...Machine::Stop() > operation. Can we just eliminate creating the render_thread_ (from everywhere), > and then only create it in ...Machine::Start()? Done.
lgtm https://codereview.chromium.org/130253002/diff/850001/content/browser/rendere... File content/browser/renderer_host/media/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/130253002/diff/850001/content/browser/rendere... content/browser/renderer_host/media/web_contents_video_capture_device.cc:729: if (success && render_thread_ && render_thread_->message_loop_proxy()) { nit: This should be a DCHECK(render_thread_.get()) before the if-statement instead, since CopyFromBackingStore() shouldn't be invoked after subscription_.reset() is called in the WCCaptureMachine::Stop() method.
lgtm Sorry...one more comment. https://codereview.chromium.org/130253002/diff/850001/content/browser/rendere... File content/browser/renderer_host/media/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/130253002/diff/850001/content/browser/rendere... content/browser/renderer_host/media/web_contents_video_capture_device.cc:582: return false; Add a render_thread_.reset() here before the return statement.
https://codereview.chromium.org/130253002/diff/850001/content/browser/rendere... File content/browser/renderer_host/media/web_contents_video_capture_device.cc (right): https://codereview.chromium.org/130253002/diff/850001/content/browser/rendere... content/browser/renderer_host/media/web_contents_video_capture_device.cc:582: return false; On 2014/01/13 23:25:10, miu wrote: > Add a render_thread_.reset() here before the return statement. Done. It seems it's also needed below in "if (!StartObservingWebContents()) { ... }" so I added it there as well. Let me know if it isn't the case. https://codereview.chromium.org/130253002/diff/850001/content/browser/rendere... content/browser/renderer_host/media/web_contents_video_capture_device.cc:729: if (success && render_thread_ && render_thread_->message_loop_proxy()) { On 2014/01/13 23:22:14, miu wrote: > nit: This should be a DCHECK(render_thread_.get()) before the if-statement > instead, since CopyFromBackingStore() shouldn't be invoked after > subscription_.reset() is called in the WCCaptureMachine::Stop() method. Done.
Friendly ping for Sergey.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/130253002/1010001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/130253002/1010001
Message was sent while issue was closed.
Change committed as 244876 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
