|
|
Created:
6 years, 3 months ago by boliu Modified:
6 years, 3 months ago CC:
chromium-reviews, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionFix threading issue in StreamTextureProxyImpl
BindToLoop need to set |loop_| synchronously to prevent delete from
destroying the object on the wrong thread.
BUG=415306, 412578
Committed: https://crrev.com/c8072c10dd733f38c7f429333b85dc63383597ef
Cr-Commit-Position: refs/heads/master@{#295543}
Patch Set 1 #
Total comments: 3
Patch Set 2 : no lock #
Total comments: 9
Patch Set 3 : review, comment only changes #
Messages
Total messages: 17 (2 generated)
boliu@chromium.org changed reviewers: + qinmin@chromium.org, sievers@chromium.org
PTAL asap please. This context lost fix has dragged on for 2+weeks.
https://codereview.chromium.org/575293003/diff/1/content/renderer/media/andro... File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/575293003/diff/1/content/renderer/media/andro... content/renderer/media/android/stream_texture_factory_impl.cc:52: scoped_refptr<base::MessageLoopProxy> loop; why do we need this, cannot we use loop_? if bindToloop() is called on the impl thread, it will block the main thread and Release() cannot be called at the same time, am I missing something here? https://codereview.chromium.org/575293003/diff/1/content/renderer/media/andro... File content/renderer/media/android/stream_texture_factory_synchronous_impl.cc (right): https://codereview.chromium.org/575293003/diff/1/content/renderer/media/andro... content/renderer/media/android/stream_texture_factory_synchronous_impl.cc:70: scoped_refptr<base::MessageLoopProxy> loop; ditto
https://codereview.chromium.org/575293003/diff/1/content/renderer/media/andro... File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/575293003/diff/1/content/renderer/media/andro... content/renderer/media/android/stream_texture_factory_impl.cc:52: scoped_refptr<base::MessageLoopProxy> loop; On 2014/09/18 02:01:30, qinmin wrote: > why do we need this, cannot we use loop_? > > if bindToloop() is called on the impl thread, it will block the main thread and > Release() cannot be called at the same time, am I missing something here? I guess you can argue it both ways. One side is "all access to client_ and loop_ must be protected". Another side is "this is release, so the last thing to be called on this thread, so no need to require any locks". I chose the first one. It doesn't matter in practice I guess, but it's hard to understand the second argument.
Ehh, whatever, removed that lock on loop
lgtm
On 2014/09/18 02:22:07, qinmin wrote: > lgtm I'll wait for Daniel too
https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... content/renderer/media/android/stream_texture_factory_impl.cc:68: DCHECK(loop); loop is not always non-null. For Layout tests, RenderThreadImpl::current()->compositor_message_loop_proxy() actually returns NULL
bug?
On 2014/09/18 16:46:28, boliu wrote: > bug? I mean is there a bug number?
On 2014/09/18 16:46:28, boliu wrote: > bug? Probably layout test is not running threaded compositing // Will be NULL if threaded compositing has not been enabled. scoped_refptr<base::MessageLoopProxy> compositor_message_loop_proxy() const { return compositor_message_loop_proxy_; }
lgtm https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... content/renderer/media/android/stream_texture_factory_impl.cc:39: base::Lock lock_; nit: put a comment 'protects access to client_ and loop_. https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... content/renderer/media/android/stream_texture_factory_impl.cc:52: { nit: // we cannot call into client anymore (from any thread) after returning from here https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... content/renderer/media/android/stream_texture_factory_impl.cc:57: // lock. nit: maybe rephrase this comment to that outside calls into this object are not allowed/expected anymore from either thread when Release() is called. Assuming that we don't allow binding to a yet completely different thread (DCHECK in line 72 below), you could simply handle it here though by just putting loop_ into a local variable while holding the lock above. It might not make too much sense to handle this case though, since it gets a bit silly. https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... content/renderer/media/android/stream_texture_factory_impl.cc:68: DCHECK(loop); On 2014/09/18 16:45:11, qinmin wrote: > loop is not always non-null. > For Layout tests, RenderThreadImpl::current()->compositor_message_loop_proxy() > actually returns NULL should we be passing the main thread's loop then instead for those cases?
https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... content/renderer/media/android/stream_texture_factory_impl.cc:39: base::Lock lock_; On 2014/09/18 18:46:53, sievers wrote: > nit: put a comment 'protects access to client_ and loop_. Done. https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... content/renderer/media/android/stream_texture_factory_impl.cc:52: { On 2014/09/18 18:46:53, sievers wrote: > nit: // we cannot call into client anymore (from any thread) after returning > from here Done. https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... content/renderer/media/android/stream_texture_factory_impl.cc:57: // lock. On 2014/09/18 18:46:52, sievers wrote: > nit: maybe rephrase this comment to that outside calls into this object are not > allowed/expected anymore from either thread when Release() is called. Done. > > Assuming that we don't allow binding to a yet completely different thread > (DCHECK in line 72 below), you could simply handle it here though by just > putting loop_ into a local variable while holding the lock above. It might not > make too much sense to handle this case though, since it gets a bit silly. I had that in PS1, then Min said it's not needed, which is true. https://codereview.chromium.org/575293003/diff/20001/content/renderer/media/a... content/renderer/media/android/stream_texture_factory_impl.cc:68: DCHECK(loop); On 2014/09/18 18:46:53, sievers wrote: > On 2014/09/18 16:45:11, qinmin wrote: > > loop is not always non-null. > > For Layout tests, RenderThreadImpl::current()->compositor_message_loop_proxy() > > actually returns NULL > > should we be passing the main thread's loop then instead for those cases? In another CL after I reproduce the problem.
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/575293003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as b4fd97206c57cd2347d6e2997b5f0d9db6fefda1
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c8072c10dd733f38c7f429333b85dc63383597ef Cr-Commit-Position: refs/heads/master@{#295543} |