Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1464183002:
media: Simplify MediaPermissionDispatcher. (Closed)
|
Created:
5 years, 1 month ago by xhwang Modified:
4 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri (slow - plz ping), nasko+codewatch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Simplify MediaPermissionDispatcher.
This partially reverts commit f4282b031b403e1358125e0db371bda17cac604a and
supports calling {Has|Request}Permission on any thread with a simpler way.
This would also make it easier to use MediaPermissionDispatcher in other places
(e.g. in the GPU process).
BUG=520101, 559839
Committed: https://crrev.com/5206d4e628900aad06a755f81c41a95b667a247d
Cr-Commit-Position: refs/heads/master@{#373717}
Patch Set 1 : revert only #Patch Set 2 : simplification #Patch Set 3 : update callers (p2p code) #Patch Set 4 : fix test #Patch Set 5 : fix comment #
Total comments: 14
Patch Set 6 : comments addressed #
Total comments: 5
Patch Set 7 : #
Total comments: 7
Patch Set 8 : rebase only #Patch Set 9 : comments addressed #
Total comments: 6
Patch Set 10 : comments addressed #
Total comments: 7
Patch Set 11 : rebase only #Patch Set 12 : comments addressed #Patch Set 13 : fix rebase errors #
Total comments: 12
Patch Set 14 : comments addressed #Patch Set 15 : comments addressed #Patch Set 16 : rebase only #Messages
Total messages: 77 (14 generated)
xhwang@chromium.org changed reviewers: + guoweis@chromium.org
guoweis: I need to use MediaPermissionDispatcher out of the render process so I am refactoring it to make it more generic. As the first step, I am simplifying the code a bit to make it easier to use. It seems what we need from MediaPermissionDispatcherProxy is to support multiple threads. Could you please help check whether this code would work for you? I am not sure how to test the RTC/P2P code.
On 2015/11/22 18:04:34, xhwang wrote: > guoweis: I need to use MediaPermissionDispatcher out of the render process so I > am refactoring it to make it more generic. As the first step, I am simplifying > the code a bit to make it easier to use. It seems what we need from > MediaPermissionDispatcherProxy is to support multiple threads. Could you please > help check whether this code would work for you? I am not sure how to test the > RTC/P2P code. For easier review: PS1 is the revert of f4282b031b403e1358125e0db371bda17cac604a. PS2 implements the multithread support in a (hopefully) simpler way. PS2 fixes the callers (P2P code) where instead of passing in a scoped_ptr, we simply use a raw pointer. PS4 fixes the test.
On 2015/11/22 18:08:10, xhwang wrote: > On 2015/11/22 18:04:34, xhwang wrote: > > guoweis: I need to use MediaPermissionDispatcher out of the render process so > I > > am refactoring it to make it more generic. As the first step, I am simplifying > > the code a bit to make it easier to use. It seems what we need from > > MediaPermissionDispatcherProxy is to support multiple threads. Could you > please > > help check whether this code would work for you? I am not sure how to test the > > RTC/P2P code. > > For easier review: > PS1 is the revert of f4282b031b403e1358125e0db371bda17cac604a. > PS2 implements the multithread support in a (hopefully) simpler way. > PS2 fixes the callers (P2P code) where instead of passing in a scoped_ptr, we > simply use a raw pointer. > PS4 fixes the test. lgtm. Nice to remove all these.
On 2015/11/23 18:42:58, guoweis_chromium wrote: > On 2015/11/22 18:08:10, xhwang wrote: > > On 2015/11/22 18:04:34, xhwang wrote: > > > guoweis: I need to use MediaPermissionDispatcher out of the render process > so > > I > > > am refactoring it to make it more generic. As the first step, I am > simplifying > > > the code a bit to make it easier to use. It seems what we need from > > > MediaPermissionDispatcherProxy is to support multiple threads. Could you > > please > > > help check whether this code would work for you? I am not sure how to test > the > > > RTC/P2P code. > > > > For easier review: > > PS1 is the revert of f4282b031b403e1358125e0db371bda17cac604a. > > PS2 implements the multithread support in a (hopefully) simpler way. > > PS2 fixes the callers (P2P code) where instead of passing in a scoped_ptr, we > > simply use a raw pointer. > > PS4 fixes the test. > > lgtm. Nice to remove all these. Thanks! Did you test P2P code and make sure it's still working?
On 2015/11/23 18:43:52, xhwang wrote: > On 2015/11/23 18:42:58, guoweis_chromium wrote: > > On 2015/11/22 18:08:10, xhwang wrote: > > > On 2015/11/22 18:04:34, xhwang wrote: > > > > guoweis: I need to use MediaPermissionDispatcher out of the render process > > so > > > I > > > > am refactoring it to make it more generic. As the first step, I am > > simplifying > > > > the code a bit to make it easier to use. It seems what we need from > > > > MediaPermissionDispatcherProxy is to support multiple threads. Could you > > > please > > > > help check whether this code would work for you? I am not sure how to test > > the > > > > RTC/P2P code. > > > > > > For easier review: > > > PS1 is the revert of f4282b031b403e1358125e0db371bda17cac604a. > > > PS2 implements the multithread support in a (hopefully) simpler way. > > > PS2 fixes the callers (P2P code) where instead of passing in a scoped_ptr, > we > > > simply use a raw pointer. > > > PS4 fixes the test. > > > > lgtm. Nice to remove all these. > > Thanks! Did you test P2P code and make sure it's still working? Doing that now, will get back once I'm done. You might want to ping sergeyu to get owner lgtm in the meantime.
xhwang@chromium.org changed reviewers: + sergeyu@chromium.org
sergeyu@chromium.org: Please OWNERS review!
On 2015/11/23 18:48:54, xhwang wrote: > mailto:sergeyu@chromium.org: Please OWNERS review! I confirm that this still works fine as intended.
On 2015/11/23 20:31:26, guoweis_chromium wrote: > On 2015/11/23 18:48:54, xhwang wrote: > > mailto:sergeyu@chromium.org: Please OWNERS review! > > I confirm that this still works fine as intended. Great. Thanks for the confirmation!
xhwang@chromium.org changed reviewers: + nasko@chromium.org
nasko@chromium.org: Please review changes in content/renderer/render_frame_impl.*. I am simplifying these files :) sergeyu: kindly ping?
https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... File content/renderer/media/media_permission_dispatcher.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... content/renderer/media/media_permission_dispatcher.cc:54: void MediaPermissionDispatcher::HasPermission( So this method can be called from any thread and you are passing around raw MediaPermission pointer. Is there anything to ensure that it's not deleted while another thread tries to call this method? https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... content/renderer/media/media_permission_dispatcher.cc:62: weak_factory_.GetWeakPtr(), type, security_origin, You are posting a task to a different thread using weak pointer here. In general weak pointers are not thread-safe. See the comments base/memory/weak_ptr.h . It's likely that some DCHECKS in weak_ptr will fail when you try to run this in debug builds. You can still use weak_ptr for cross-thread PostTask(), but you need to make sure that: 1. The first time weak_factory_.GetWeakPtr() is called on target thread. E.g. you can call it in the constructor, save the result and use it here instead of calling GetWeakPtr() here. 2. weak_factory_ is deleted on the thread referenced by task_runner_ - there is already DCHECK for that in the destructor, which is good.
comments only https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... File content/renderer/media/media_permission_dispatcher.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... content/renderer/media/media_permission_dispatcher.cc:54: void MediaPermissionDispatcher::HasPermission( On 2015/11/24 21:08:42, Sergey Ulanov wrote: > So this method can be called from any thread and you are passing around raw > MediaPermission pointer. Is there anything to ensure that it's not deleted while > another thread tries to call this method? Good point! The MediaPermission has the same lifetime as RenderFrameImpl. I don't know how P2P code works. For media code, the media player will be destroyed (including the part running on the "media" thread) before RenderFrameImpl goes away. Can we make similar assumption here for P2P code? If not, we probably should let P2P code hold a weak ptr to MediaPermission and always check it before calling any method. When the MediaPermission is destructed, the weak ptr will be invalidated. https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... content/renderer/media/media_permission_dispatcher.cc:62: weak_factory_.GetWeakPtr(), type, security_origin, On 2015/11/24 21:08:42, Sergey Ulanov wrote: > You are posting a task to a different thread using weak pointer here. In general > weak pointers are not thread-safe. See the comments base/memory/weak_ptr.h . > It's likely that some DCHECKS in weak_ptr will fail when you try to run this in > debug builds. > You can still use weak_ptr for cross-thread PostTask(), but you need to make > sure that: > 1. The first time weak_factory_.GetWeakPtr() is called on target thread. E.g. > you can call it in the constructor, save the result and use it here instead of > calling GetWeakPtr() here. > 2. weak_factory_ is deleted on the thread referenced by task_runner_ - there > is already DCHECK for that in the destructor, which is good. From weak_ptr.h: // To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory // is dereferenced, the factory and its WeakPtrs become bound to the calling // thread..., and cannot be dereferenced or invalidated on any other task runner As you can see, the WeakPtr is always passed to the |task_runner_| immediately after it's created. So it'll be bound to the |task_runner| and will always be dereferenced and/or invalidated there. I don't think we always need call weak_factory_.GetWeakPtr() on the target thread. It seems GetWeakPtr is implemented by WeakReferenceOwner which uses a WeakReference::Flag that is thread safe. But I am not super sure about the implementation though.
xhwang@chromium.org changed reviewers: + wez@chromium.org
Adding wez@ since he contributed many thread safety notes on WeakPtr. wez@: Please see the concern sergeyu@ raised and my response. I think there are some confusion around this (mostly from me).
https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... File content/renderer/media/media_permission_dispatcher.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... content/renderer/media/media_permission_dispatcher.cc:54: void MediaPermissionDispatcher::HasPermission( On 2015/11/24 22:54:59, xhwang wrote: > On 2015/11/24 21:08:42, Sergey Ulanov wrote: > > So this method can be called from any thread and you are passing around raw > > MediaPermission pointer. Is there anything to ensure that it's not deleted > while > > another thread tries to call this method? > > Good point! The MediaPermission has the same lifetime as RenderFrameImpl. I > don't know how P2P code works. For media code, the media player will be > destroyed (including the part running on the "media" thread) before > RenderFrameImpl goes away. Can we make similar assumption here for P2P code? > > If not, we probably should let P2P code hold a weak ptr to MediaPermission and > always check it before calling any method. When the MediaPermission is > destructed, the weak ptr will be invalidated. > PeerConnectionDependencyFactory is owned by RenderThreadImpl which, from my look at the code, seems to be able to live without RenderFrameImpl. You probably want a weakptr here to be safe. Also, need to deny the request if the mediapermission is invalid at that point, just as what proxy did in the past.
On 2015/11/24 23:15:07, guoweis_chromium wrote: > https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... > File content/renderer/media/media_permission_dispatcher.cc (right): > > https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... > content/renderer/media/media_permission_dispatcher.cc:54: void > MediaPermissionDispatcher::HasPermission( > On 2015/11/24 22:54:59, xhwang wrote: > > On 2015/11/24 21:08:42, Sergey Ulanov wrote: > > > So this method can be called from any thread and you are passing around raw > > > MediaPermission pointer. Is there anything to ensure that it's not deleted > > while > > > another thread tries to call this method? > > > > Good point! The MediaPermission has the same lifetime as RenderFrameImpl. I > > don't know how P2P code works. For media code, the media player will be > > destroyed (including the part running on the "media" thread) before > > RenderFrameImpl goes away. Can we make similar assumption here for P2P code? > > > > If not, we probably should let P2P code hold a weak ptr to MediaPermission and > > always check it before calling any method. When the MediaPermission is > > destructed, the weak ptr will be invalidated. > > > > PeerConnectionDependencyFactory is owned by RenderThreadImpl which, from my look > at the code, seems to be able to live without RenderFrameImpl. You probably > want a weakptr here to be safe. Also, need to deny the request if the > mediapermission is invalid at that point, just as what proxy did in the past. I would be surprised if an object created by RenderFrameImpl is passed to an object that can outlive RenderFrameImpl. Actually, looking at the code, PeerConnectionDependencyFactory doesn't keep the media_permission. It pass it to P2PPortAllocatorFactory immediately after it get media_permission [1], which seems to be using the permission immediately during initialization [2]. Are there actual cases where we'll keep and use media_permission after RenderFrameImpl is destroyed? If you do need a MediaPermission that has the life time of the RenderProcess/RenderThread, we probably should create a MediaPermissionDispatcher that is based on the ServiceRegistry in RenderThreadImpl (instead of the one in RenderFrameImpl). I am pretty sure we have Permission service hooked up there as well [4]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... [3] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... [4] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... content/renderer/media/media_permission_dispatcher.cc:62: weak_factory_.GetWeakPtr(), type, security_origin, On 2015/11/24 at 22:54:59, xhwang wrote: > On 2015/11/24 21:08:42, Sergey Ulanov wrote: > > You are posting a task to a different thread using weak pointer here. In general > > weak pointers are not thread-safe. See the comments base/memory/weak_ptr.h . > > It's likely that some DCHECKS in weak_ptr will fail when you try to run this in > > debug builds. > > You can still use weak_ptr for cross-thread PostTask(), but you need to make > > sure that: > > 1. The first time weak_factory_.GetWeakPtr() is called on target thread. E.g. > > you can call it in the constructor, save the result and use it here instead of > > calling GetWeakPtr() here. That's not true; it's only required that the WeakPtr is only de-referenced, or checked, on the same thread on which the WeakPtrFactory will invalidate it - you can safely create an object on thread A, take a WeakPtr to it, and then post tasks to it on thread B through that pointer, provided that the pointer is also invalidated on thread B. The caveat to that would be if you had multiple threads calling in to the WeakPtrFactory::GetWeakPtr() call - that call is not designed to be thread-safe. > > 2. weak_factory_ is deleted on the thread referenced by task_runner_ - there > > is already DCHECK for that in the destructor, which is good. > > From weak_ptr.h: > > // To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory > // is dereferenced, the factory and its WeakPtrs become bound to the calling > // thread..., and cannot be dereferenced or invalidated on any other task runner > > As you can see, the WeakPtr is always passed to the |task_runner_| immediately after > it's created. So it'll be bound to the |task_runner| and will always be > dereferenced and/or invalidated there. > > I don't think we always need call weak_factory_.GetWeakPtr() on the target thread. > It seems GetWeakPtr is implemented by WeakReferenceOwner which uses a > WeakReference::Flag that is thread safe. But I am not super sure about the > implementation though. My recommendation, if you need WeakPtrs, would be to create a WeakPtr from the factory during construction, and Bind() that in to PostTask calls. However, please see my earlier comment on the thread constraints of this class! https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... File content/renderer/media/media_permission_dispatcher.h (right): https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... content/renderer/media/media_permission_dispatcher.h:32: // be fired on the thread where these methods are called. How is it safe to call "on any thread"? The dispatcher is an owned object, not ref-counted, so how can callers on threads A & B, for example, make sure not to try to call it while it's being deleted on thread C? Is there some fundamental assumption re the calling code that you need to document here?
https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... content/renderer/media/media_permission_dispatcher.cc:62: weak_factory_.GetWeakPtr(), type, security_origin, On 2015/11/25 00:44:48, Wez wrote: > On 2015/11/24 at 22:54:59, xhwang wrote: > > On 2015/11/24 21:08:42, Sergey Ulanov wrote: > > > You are posting a task to a different thread using weak pointer here. In > general > > > weak pointers are not thread-safe. See the comments base/memory/weak_ptr.h . > > > It's likely that some DCHECKS in weak_ptr will fail when you try to run this > in > > > debug builds. > > > You can still use weak_ptr for cross-thread PostTask(), but you need to make > > > sure that: > > > 1. The first time weak_factory_.GetWeakPtr() is called on target thread. > E.g. > > > you can call it in the constructor, save the result and use it here instead > of > > > calling GetWeakPtr() here. > > That's not true; it's only required that the WeakPtr is only de-referenced, or > checked, on the same thread on which the WeakPtrFactory will invalidate it - you > can safely create an object on thread A, take a WeakPtr to it, and then post > tasks to it on thread B through that pointer, provided that the pointer is also > invalidated on thread B. What happens here is that the object is created and destroyed on the same thread and all weak pointers are dereferenced on the same thread. But GetWeakPtr() is called on other threads. Is that safe? > > The caveat to that would be if you had multiple threads calling in to the > WeakPtrFactory::GetWeakPtr() call - that call is not designed to be thread-safe. And that's the problem in this code: GetWeakPtr() is called on arbitrary threads, potentially in parallel.
https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... content/renderer/media/media_permission_dispatcher.cc:62: weak_factory_.GetWeakPtr(), type, security_origin, On 2015/11/25 at 01:01:36, Sergey Ulanov wrote: > On 2015/11/25 00:44:48, Wez wrote: > > On 2015/11/24 at 22:54:59, xhwang wrote: > > > On 2015/11/24 21:08:42, Sergey Ulanov wrote: > > > > You are posting a task to a different thread using weak pointer here. In > > general > > > > weak pointers are not thread-safe. See the comments base/memory/weak_ptr.h . > > > > It's likely that some DCHECKS in weak_ptr will fail when you try to run this > > in > > > > debug builds. > > > > You can still use weak_ptr for cross-thread PostTask(), but you need to make > > > > sure that: > > > > 1. The first time weak_factory_.GetWeakPtr() is called on target thread. > > E.g. > > > > you can call it in the constructor, save the result and use it here instead > > of > > > > calling GetWeakPtr() here. > > > > That's not true; it's only required that the WeakPtr is only de-referenced, or > > checked, on the same thread on which the WeakPtrFactory will invalidate it - you > > can safely create an object on thread A, take a WeakPtr to it, and then post > > tasks to it on thread B through that pointer, provided that the pointer is also > > invalidated on thread B. > > What happens here is that the object is created and destroyed on the same thread and all weak pointers are dereferenced on the same thread. But GetWeakPtr() is called on other threads. Is that safe? OK, gotcha! It _is_ safe to pass such a WeakPtr off to code on other threads, but it _is not_ safe to call WeakPtrFactory::GetWeakPtr() from multiple threads, since there is no locking around the assignment of the Flag object into the factory. So in this case you should definitely take the WeakPtr at construction time and use that to Bind() tasks - then you're just inc/decrementing a reference to a stable Flag. > > > > > The caveat to that would be if you had multiple threads calling in to the > > WeakPtrFactory::GetWeakPtr() call - that call is not designed to be thread-safe. > > And that's the problem in this code: GetWeakPtr() is called on arbitrary threads, potentially in parallel. Exactly.
https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... content/renderer/media/media_permission_dispatcher.cc:62: weak_factory_.GetWeakPtr(), type, security_origin, On 2015/11/25 01:09:00, Wez wrote: > On 2015/11/25 at 01:01:36, Sergey Ulanov wrote: > > On 2015/11/25 00:44:48, Wez wrote: > > > On 2015/11/24 at 22:54:59, xhwang wrote: > > > > On 2015/11/24 21:08:42, Sergey Ulanov wrote: > > > > > You are posting a task to a different thread using weak pointer here. In > > > general > > > > > weak pointers are not thread-safe. See the comments > base/memory/weak_ptr.h . > > > > > It's likely that some DCHECKS in weak_ptr will fail when you try to run > this > > > in > > > > > debug builds. > > > > > You can still use weak_ptr for cross-thread PostTask(), but you need to > make > > > > > sure that: > > > > > 1. The first time weak_factory_.GetWeakPtr() is called on target > thread. > > > E.g. > > > > > you can call it in the constructor, save the result and use it here > instead > > > of > > > > > calling GetWeakPtr() here. > > > > > > That's not true; it's only required that the WeakPtr is only de-referenced, > or > > > checked, on the same thread on which the WeakPtrFactory will invalidate it - > you > > > can safely create an object on thread A, take a WeakPtr to it, and then post > > > tasks to it on thread B through that pointer, provided that the pointer is > also > > > invalidated on thread B. > > > > What happens here is that the object is created and destroyed on the same > thread and all weak pointers are dereferenced on the same thread. But > GetWeakPtr() is called on other threads. Is that safe? > > OK, gotcha! It _is_ safe to pass such a WeakPtr off to code on other threads, > but it _is not_ safe to call WeakPtrFactory::GetWeakPtr() from multiple threads, > since there is no locking around the assignment of the Flag object into the > factory. So in this case you should definitely take the WeakPtr at construction > time and use that to Bind() tasks - then you're just inc/decrementing a > reference to a stable Flag. Will do. Actually I saw cases where we have two WeakPtrFactory in one class. That must be because of this. But it's never this clear. Sounds like it's worth a comment in weak_ptr.h? > > > The caveat to that would be if you had multiple threads calling in to the > > > WeakPtrFactory::GetWeakPtr() call - that call is not designed to be > thread-safe. > > > > And that's the problem in this code: GetWeakPtr() is called on arbitrary > threads, potentially in parallel. > > Exactly. > https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... File content/renderer/media/media_permission_dispatcher.h (right): https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... content/renderer/media/media_permission_dispatcher.h:32: // be fired on the thread where these methods are called. On 2015/11/25 00:44:48, Wez wrote: > How is it safe to call "on any thread"? The dispatcher is an owned object, not > ref-counted, so how can callers on threads A & B, for example, make sure not to > try to call it while it's being deleted on thread C? > > Is there some fundamental assumption re the calling code that you need to > document here? |this| shouldn't worry about how it's owned, right? I think it's the caller's responsibility to make sure that it never call into a deleted object. In our case, media players are always destroyed before RenderFrameImpl is destructed so this is okay. Actually the reason I am making this change is that I need to use MediaPermissionDispatcher in other places as well, e.g. in the GPU process. In that case we'll have a different ownership model. It should always be the responsibility of the caller (in collaboration with the owner) to make sure these calls are safe. Does that make sense?
comments addressed
I updated this CL to use |weak_this_|. PTAL again! guoweis: Please help double check the life time of P2P code.
https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... content/renderer/media/media_permission_dispatcher.cc:62: weak_factory_.GetWeakPtr(), type, security_origin, On 2015/11/25 at 01:18:02, xhwang wrote: > On 2015/11/25 01:09:00, Wez wrote: > > On 2015/11/25 at 01:01:36, Sergey Ulanov wrote: > > > On 2015/11/25 00:44:48, Wez wrote: > > > > On 2015/11/24 at 22:54:59, xhwang wrote: > > > > > On 2015/11/24 21:08:42, Sergey Ulanov wrote: > > > > > > You are posting a task to a different thread using weak pointer here. In > > > > general > > > > > > weak pointers are not thread-safe. See the comments > > base/memory/weak_ptr.h . > > > > > > It's likely that some DCHECKS in weak_ptr will fail when you try to run > > this > > > > in > > > > > > debug builds. > > > > > > You can still use weak_ptr for cross-thread PostTask(), but you need to > > make > > > > > > sure that: > > > > > > 1. The first time weak_factory_.GetWeakPtr() is called on target > > thread. > > > > E.g. > > > > > > you can call it in the constructor, save the result and use it here > > instead > > > > of > > > > > > calling GetWeakPtr() here. > > > > > > > > That's not true; it's only required that the WeakPtr is only de-referenced, > > or > > > > checked, on the same thread on which the WeakPtrFactory will invalidate it - > > you > > > > can safely create an object on thread A, take a WeakPtr to it, and then post > > > > tasks to it on thread B through that pointer, provided that the pointer is > > also > > > > invalidated on thread B. > > > > > > What happens here is that the object is created and destroyed on the same > > thread and all weak pointers are dereferenced on the same thread. But > > GetWeakPtr() is called on other threads. Is that safe? > > > > OK, gotcha! It _is_ safe to pass such a WeakPtr off to code on other threads, > > but it _is not_ safe to call WeakPtrFactory::GetWeakPtr() from multiple threads, > > since there is no locking around the assignment of the Flag object into the > > factory. So in this case you should definitely take the WeakPtr at construction > > time and use that to Bind() tasks - then you're just inc/decrementing a > > reference to a stable Flag. > > Will do. > > Actually I saw cases where we have two WeakPtrFactory in one class. That must be because of this. But it's never this clear. Do you mean that you saw a WeakPtrFactory, and a WeakPtr in one class? The only reason you should ever see multiple WeakPtrFactory instances in a class is if that class needs to dispense or Bind() WeakPtrs for multiple distinct objects. Do you have an example in mind..? > Sounds like it's worth a comment in weak_ptr.h? Yeah, I'll add something. > > > > The caveat to that would be if you had multiple threads calling in to the > > > > WeakPtrFactory::GetWeakPtr() call - that call is not designed to be > > thread-safe. > > > > > > And that's the problem in this code: GetWeakPtr() is called on arbitrary > > threads, potentially in parallel. > > > > Exactly. > >
https://codereview.chromium.org/1464183002/diff/100001/content/renderer/media... File content/renderer/media/media_permission_dispatcher.h (right): https://codereview.chromium.org/1464183002/diff/100001/content/renderer/media... content/renderer/media/media_permission_dispatcher.h:57: // Bound to the |task_runner_|. This doesn't know anything about "the |task_runner_|", so this is misleading. If you want a comment to remind you that it can only be de-referenced on that task runner's thread then I'd suggest something more explanatory like "Used to safely post MediaPermission calls for execution on |task_runner_|." However, as previously noted, the "Can be called on any thread" comment earlier on is highly suspect - there's nothing in this implementation that hints as to why code on other threads can safely assume that this object is still valid when call it in the first place. https://codereview.chromium.org/1464183002/diff/100001/content/renderer/media... content/renderer/media/media_permission_dispatcher.h:58: base::WeakPtr<MediaPermissionDispatcher> weak_this_; nit: Convention elsewhere is to call these |weak_ptr_|.
https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... File content/renderer/media/media_permission_dispatcher.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/80001/content/renderer... content/renderer/media/media_permission_dispatcher.cc:62: weak_factory_.GetWeakPtr(), type, security_origin, (omitting old comments) > > Actually I saw cases where we have two WeakPtrFactory in one class. That must > be because of this. But it's never this clear. > > Do you mean that you saw a WeakPtrFactory, and a WeakPtr in one class? The only > reason you should ever see multiple WeakPtrFactory instances in a class is if > that class needs to dispense or Bind() WeakPtrs for multiple distinct objects. > Do you have an example in mind..? Here's an example where we use two weak ptr factories: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://chromiumcodereview.appspot.com/1464183002/diff/100001/content/rendere... File content/renderer/media/media_permission_dispatcher.h (right): https://chromiumcodereview.appspot.com/1464183002/diff/100001/content/rendere... content/renderer/media/media_permission_dispatcher.h:57: // Bound to the |task_runner_|. On 2015/11/25 02:16:20, Wez wrote: > This doesn't know anything about "the |task_runner_|", so this is misleading. > > If you want a comment to remind you that it can only be de-referenced on that > task runner's thread then I'd suggest something more explanatory like "Used to > safely post MediaPermission calls for execution on |task_runner_|." Done. > However, as previously noted, the "Can be called on any thread" comment earlier > on is highly suspect - there's nothing in this implementation that hints as to > why code on other threads can safely assume that this object is still valid when > call it in the first place. Updated the comment. Again ISTM the caller need to make sure |this| is valid when the call is made. guoweis / sergeyu: It seems the permission is passed in PeerConnectionDependencyFactory::CreatePeerConnection() which is RenderFrame specific. Does that mean the peer connection created is owned by RenderFrameImpl? What does the lifetime look like here? How does things work when the P2P code needs to check permission only to find that the permission service is already dead? https://chromiumcodereview.appspot.com/1464183002/diff/100001/content/rendere... content/renderer/media/media_permission_dispatcher.h:58: base::WeakPtr<MediaPermissionDispatcher> weak_this_; On 2015/11/25 02:16:19, Wez wrote: > nit: Convention elsewhere is to call these |weak_ptr_|. Done.
https://codereview.chromium.org/1464183002/diff/120001/content/renderer/media... File content/renderer/media/media_permission_dispatcher.h (right): https://codereview.chromium.org/1464183002/diff/120001/content/renderer/media... content/renderer/media/media_permission_dispatcher.h:25: public RenderFrameObserver { How is this related to https://codereview.chromium.org/1470233002/?
guoweis@chromium.org changed reviewers: + tommi@chromium.org
+tommi. Tommi, there is a question about the life management of PC w.r.t. the RenderFrame. Could you shed some light on this? https://codereview.chromium.org/1464183002/diff/100001/content/renderer/media... File content/renderer/media/media_permission_dispatcher.h (right): https://codereview.chromium.org/1464183002/diff/100001/content/renderer/media... content/renderer/media/media_permission_dispatcher.h:57: // Bound to the |task_runner_|. On 2015/11/25 07:06:03, xhwang wrote: > On 2015/11/25 02:16:20, Wez wrote: > > This doesn't know anything about "the |task_runner_|", so this is misleading. > > > > If you want a comment to remind you that it can only be de-referenced on that > > task runner's thread then I'd suggest something more explanatory like "Used to > > safely post MediaPermission calls for execution on |task_runner_|." > > Done. > > > However, as previously noted, the "Can be called on any thread" comment > earlier > > on is highly suspect - there's nothing in this implementation that hints as to > > why code on other threads can safely assume that this object is still valid > when > > call it in the first place. > > Updated the comment. Again ISTM the caller need to make sure |this| is valid > when > the call is made. > > guoweis / sergeyu: It seems the permission is passed in > PeerConnectionDependencyFactory::CreatePeerConnection() which is RenderFrame > specific. Does that mean the peer connection created is owned by > RenderFrameImpl? What does the lifetime look like here? How does things work > when the P2P code needs to check permission only to find that the permission > service is already dead? > +tommi to the review. RTCPeerConnection is eventually owned by the javascript world although the exact relationship b/w it and RenderFrame is not clear to me. Tommi, do you know if there is any situation that PC could outlive the associated RenderFrame? We currently get a MediaPermission for mic/camera permission check which is owned by RenderFrame. Can it still hold a pointer to an invalid MediaPermission if the RenderFrame is deleted?
tommi@chromium.org changed reviewers: + perkj@chromium.org
Per - can you answer the question above on PC lifetime?
On 2015/11/25 16:26:08, tommi wrote: > Per - can you answer the question above on PC lifetime? FWIW, it'll be totally fine if PC can outlive RenderFrames. But in that case, PC shouldn't use a service provided by a RenderFrame. I can actually create a MediaPermissionDispatcher in RenderThreadImpl so that you have a permission service in the render process (instead of render frame). But before we do that, I'd really like to understand how the lifetime model works today.
https://chromiumcodereview.appspot.com/1464183002/diff/120001/content/rendere... File content/renderer/media/media_permission_dispatcher.h (right): https://chromiumcodereview.appspot.com/1464183002/diff/120001/content/rendere... content/renderer/media/media_permission_dispatcher.h:25: public RenderFrameObserver { On 2015/11/25 15:08:30, nasko wrote: > How is this related to https://codereview.chromium.org/1470233002/ In this CL I am simplifying MediaPermissionDispatcher. In https://codereview.chromium.org/1470233002/ I am generalizing it so that it doesn't depend on RenderFrameObserver so it can be used in other places (e.g. Gpu process). After all, this is just a class that implements MediaPermission using mojo PermissionService.
https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... content/renderer/media/media_permission_dispatcher.cc:62: weak_factory_.GetWeakPtr(), type, security_origin, On 2015/11/25 at 07:06:03, xhwang wrote: > (omitting old comments) > > > > Actually I saw cases where we have two WeakPtrFactory in one class. That must > > be because of this. But it's never this clear. > > > > Do you mean that you saw a WeakPtrFactory, and a WeakPtr in one class? The only > > reason you should ever see multiple WeakPtrFactory instances in a class is if > > that class needs to dispense or Bind() WeakPtrs for multiple distinct objects. > > Do you have an example in mind..? > > Here's an example where we use two weak ptr factories: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... tl;dr: That code is overcomplicated and should be cleaned up, not used as an example of how to use WeakPtrFactory! Fundamentally it has no comments to explain the curious use of WeakPtrFactory. What it's doing, though, is working around the fact that MediaSourceDelegate effectively conflates two objects that run on different threads into a single object, for some reason. It would be both easier to understand, and more robust against breakage by future changes, if it were properly split into a main-thread-only MediaSourceDelegate and a media-thread-only sub-component object. Incidentally, looking at the use of WeakPtrs to the media-thread side of the MSD, it's not clear that they are actually required, since they seem only to be bound into callbacks, not into tasks, and the objects those callbacks are passed to are all destroyed during the StopDemuxer() stage - ironically the only PostTasks to the media thread don't use WeakPtrs. :P There are some similarly curious uses of base::Lock that would need to be resolved; it's not obvious whether they're really required, though, and in at least one case it looks like there's a bug with the |demuxer_client_| being called out to while the lock is held? (Disclaimer: This is all based on a quick glance at this code)
On 2015/11/25 20:01:40, Wez wrote: > https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... > File content/renderer/media/media_permission_dispatcher.cc (right): > > https://codereview.chromium.org/1464183002/diff/80001/content/renderer/media/... > content/renderer/media/media_permission_dispatcher.cc:62: > weak_factory_.GetWeakPtr(), type, security_origin, > On 2015/11/25 at 07:06:03, xhwang wrote: > > (omitting old comments) > > > > > > Actually I saw cases where we have two WeakPtrFactory in one class. That > must > > > be because of this. But it's never this clear. > > > > > > Do you mean that you saw a WeakPtrFactory, and a WeakPtr in one class? The > only > > > reason you should ever see multiple WeakPtrFactory instances in a class is > if > > > that class needs to dispense or Bind() WeakPtrs for multiple distinct > objects. > > > Do you have an example in mind..? > > > > Here's an example where we use two weak ptr factories: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > tl;dr: That code is overcomplicated and should be cleaned up, not used as an > example of how to use WeakPtrFactory! > > Fundamentally it has no comments to explain the curious use of WeakPtrFactory. > > What it's doing, though, is working around the fact that MediaSourceDelegate > effectively conflates two objects that run on different threads into a single > object, for some reason. It would be both easier to understand, and more robust > against breakage by future changes, if it were properly split into a > main-thread-only MediaSourceDelegate and a media-thread-only sub-component > object. Incidentally, looking at the use of WeakPtrs to the media-thread side > of the MSD, it's not clear that they are actually required, since they seem only > to be bound into callbacks, not into tasks, and the objects those callbacks are > passed to are all destroyed during the StopDemuxer() stage - ironically the only > PostTasks to the media thread don't use WeakPtrs. :P There are some similarly > curious uses of base::Lock that would need to be resolved; it's not obvious > whether they're really required, though, and in at least one case it looks like > there's a bug with the |demuxer_client_| being called out to while the lock is > held? > > (Disclaimer: This is all based on a quick glance at this code) RTCPeerConnection is an ActiveDomObject and as far as I can understand, it might be alive in the sense it has not yet been garbage collected after the RenderFrame is destroyed. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... During shutdown, when the fast shutdown path is not used, blink does not gc all its objects so we destroy the RTCPeerConnections when the RenderThread is destroyed.
xhwang@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for insights
Description was changed from
==========
media: Simplify MediaPermissionDispatcher.
This partially reverts commit f4282b031b403e1358125e0db371bda17cac604a and
supports calling {Has|Request}Permission on any thread with a simpler way.
This would also make it easier to use MediaPermissionDispatcher in other places
(e.g. in the GPU process).
BUG=520101,559839
==========
to
==========
media: Simplify MediaPermissionDispatcher.
This partially reverts commit f4282b031b403e1358125e0db371bda17cac604a and
supports calling {Has|Request}Permission on any thread with a simpler way.
This would also make it easier to use MediaPermissionDispatcher in other places
(e.g. in the GPU process).
BUG=520101,559839
==========
tommi@chromium.org changed reviewers: - tommi@chromium.org
On 2016/01/06 at 22:53:24, xhwang wrote: > +dcheng for insights Taking a look at this, I don't think passing |media_permission| in PeerConnectionDependencyFactory::CreatePeerConnection() is a problem. - The object returned from CreatePeerConnection() is saved here in RTCPeerConnectionHandler::initialize() [1]. - RTCPeerConnectionHandler is the implementation of the Blink platform class WebRTCPeerConnectionHandler [2]. - This is owned by RTCPeerConnection::m_peerHandler in Blink [3]. - But in RTCPeerConnection::stop(), m_peerHandler is cleared(), which will delete the RTCPeerConnectionHandler [4]. ActiveDOMObject::stop() is guaranteed to be called before a frame is detached, so it's impossible for RTCPeerConnectionHandler to outlive the frame. If it could, there would be a lot of other potential problems. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [3] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [4] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1464183002/diff/120001/content/renderer/media... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/120001/content/renderer/media... content/renderer/media/media_permission_dispatcher.cc:43: weak_ptr_ = weak_factory_.GetWeakPtr(); This needs a comment: WeakPtrFactory and WeakPtr thread-safety semantics are complicated. "Initialize the WeakPtrFactory since GetWeakPtr() is not thread-safe"? I don't know, it seems like WeakPtrFactory should have some of this comment too, but I'm not sure what the best split is. https://codereview.chromium.org/1464183002/diff/120001/content/renderer/media... content/renderer/media/media_permission_dispatcher.cc:71: if (!permission_service_.get()) { No .get() here and elsewhere.
perkj / sergeyu: Any thoughts on dcheng's comment about the lifetime discussion?
My concern w/ Daniel's comments would be the fact that it was necessary to defer to him for his insights in the first place - if it requires this much investigation to work out what's safe then at the very least the assumptions & constraints need documenting in comments in this code, to defend against future changes that may change them. https://chromiumcodereview.appspot.com/1464183002/diff/120001/content/rendere... File content/renderer/media/media_permission_dispatcher.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/120001/content/rendere... content/renderer/media/media_permission_dispatcher.cc:43: weak_ptr_ = weak_factory_.GetWeakPtr(); On 2016/01/12 at 21:55:53, dcheng wrote: > This needs a comment: WeakPtrFactory and WeakPtr thread-safety semantics are complicated. > > "Initialize the WeakPtrFactory since GetWeakPtr() is not thread-safe"? > > I don't know, it seems like WeakPtrFactory should have some of this comment too, but I'm not sure what the best split is. Yeah, there's a tension between allowing GetWeakPtr() to be called from whatever thread you like, regardless of where the factory or pointer will be used, versus checking for _concurrent_ calls to GetWeakPtr() from different threads, which is what's not safe. I'll send you a CL to add a comment explaining the concurrency issue at least, dcheng@. :) I don't think you need a comment here, though; taking a WeakPtr from a WeakPtrFactory you just initialized is fine - at this point there can't be any code running on other threads touching this object.
rebase only
On 2016/01/15 18:59:30, Wez wrote: > My concern w/ Daniel's comments would be the fact that it was necessary to defer > to him for his insights in the first place - if it requires this much > investigation to work out what's safe then at the very least the assumptions & > constraints need documenting in comments in this code, to defend against future > changes that may change them. Comments added.
I rebased my CL and added comments as suggested. PTAL again! https://chromiumcodereview.appspot.com/1464183002/diff/120001/content/rendere... File content/renderer/media/media_permission_dispatcher.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/120001/content/rendere... content/renderer/media/media_permission_dispatcher.cc:43: weak_ptr_ = weak_factory_.GetWeakPtr(); On 2016/01/15 18:59:30, Wez wrote: > On 2016/01/12 at 21:55:53, dcheng wrote: > > This needs a comment: WeakPtrFactory and WeakPtr thread-safety semantics are > complicated. > > > > "Initialize the WeakPtrFactory since GetWeakPtr() is not thread-safe"? > > > > I don't know, it seems like WeakPtrFactory should have some of this comment > too, but I'm not sure what the best split is. > > Yeah, there's a tension between allowing GetWeakPtr() to be called from whatever > thread you like, regardless of where the factory or pointer will be used, versus > checking for _concurrent_ calls to GetWeakPtr() from different threads, which is > what's not safe. I'll send you a CL to add a comment explaining the concurrency > issue at least, dcheng@. :) > > I don't think you need a comment here, though; taking a WeakPtr from a > WeakPtrFactory you just initialized is fine - at this point there can't be any > code running on other threads touching this object. Acknowledged. https://chromiumcodereview.appspot.com/1464183002/diff/120001/content/rendere... content/renderer/media/media_permission_dispatcher.cc:71: if (!permission_service_.get()) { On 2016/01/12 21:55:53, dcheng wrote: > No .get() here and elsewhere. Done.
perkj / sergeyu: kindly ping!
Sorry for not having reviewed this before. I am not familiar with the media_permission_dispatcher but it looks pretty straight forward. I can only find nits but if possible, I would prefer you get approval from someone who knows the media_persmission_dispatcher better. https://codereview.chromium.org/1464183002/diff/160001/content/renderer/media... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/160001/content/renderer/media... content/renderer/media/media_permission_dispatcher.cc:79: permission_service_->HasPermission( can permission_service_ still be nullptr? https://codereview.chromium.org/1464183002/diff/160001/content/renderer/media... content/renderer/media/media_permission_dispatcher.cc:108: permission_service_->RequestPermission( dito? https://codereview.chromium.org/1464183002/diff/160001/content/renderer/media... File content/renderer/media/media_permission_dispatcher.h (right): https://codereview.chromium.org/1464183002/diff/160001/content/renderer/media... content/renderer/media/media_permission_dispatcher.h:34: // Note: Can be called on any thread but the caller needs to make sure |this| |this| must be valid? Yes - it is a public method. I am afraid I dont understand what you mean.
media_permission_dispatcher https://chromiumcodereview.appspot.com/1464183002/diff/160001/content/rendere... File content/renderer/media/media_permission_dispatcher.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/160001/content/rendere... content/renderer/media/media_permission_dispatcher.cc:79: permission_service_->HasPermission( On 2016/01/25 21:19:56, perkj_chrome wrote: > can permission_service_ still be nullptr? |permission_service_| is a PermissionServicePtr, not a raw pointer. If it's not connected, the call should be just be a no-op. It should never crash. https://chromiumcodereview.appspot.com/1464183002/diff/160001/content/rendere... content/renderer/media/media_permission_dispatcher.cc:108: permission_service_->RequestPermission( On 2016/01/25 21:19:56, perkj_chrome wrote: > dito? ditto https://chromiumcodereview.appspot.com/1464183002/diff/160001/content/rendere... File content/renderer/media/media_permission_dispatcher.h (right): https://chromiumcodereview.appspot.com/1464183002/diff/160001/content/rendere... content/renderer/media/media_permission_dispatcher.h:34: // Note: Can be called on any thread but the caller needs to make sure |this| On 2016/01/25 21:19:56, perkj_chrome wrote: > |this| must be valid? Yes - it is a public method. I am afraid I dont > understand what you mean. Good point :) Since we are passing a raw pointer to MediaPermissionDispatcher (hence |this|) around, the caller needs to make sure the raw pointer is still valid at the time of the call. Otherwise it'll crash. But I agree with you this seems dumb and confusing. Updated.
perkj: guoweis@ is familiar with media_permission_dispatcher. I still need your OWNERS approval on content/renderer/media/webrtc/* guoweis: Does this CL still look good to you? sergeyu: Please OWNERS review content/renderer/p2p/* nasko: Please OWNERS review content/renderer/render_frame_impl.* Thanks! https://chromiumcodereview.appspot.com/1464183002/diff/180001/content/rendere... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/180001/content/rendere... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:408: // frame. Therefore using a raw pointer of |media_permission| is safe here. guoweis: Could you please test this tear down logic to make sure the theory is correct? Thanks!
On 2016/01/29 01:08:06, xhwang wrote: > perkj: guoweis@ is familiar with media_permission_dispatcher. I still need your > OWNERS approval on content/renderer/media/webrtc/* > > guoweis: Does this CL still look good to you? > > sergeyu: Please OWNERS review content/renderer/p2p/* > > nasko: Please OWNERS review content/renderer/render_frame_impl.* > > Thanks! > > https://chromiumcodereview.appspot.com/1464183002/diff/180001/content/rendere... > File content/renderer/media/webrtc/peer_connection_dependency_factory.cc > (right): > > https://chromiumcodereview.appspot.com/1464183002/diff/180001/content/rendere... > content/renderer/media/webrtc/peer_connection_dependency_factory.cc:408: // > frame. Therefore using a raw pointer of |media_permission| is safe here. > guoweis: Could you please test this tear down logic to make sure the theory is > correct? Thanks! content/renderer/media/webrtc/* lgtm
https://codereview.chromium.org/1464183002/diff/180001/content/renderer/media... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/180001/content/renderer/media... content/renderer/media/media_permission_dispatcher.cc:61: // Use BindToCurrentLoop to bind the callback to the current thread. nit: This comment seems redundant. https://codereview.chromium.org/1464183002/diff/180001/content/renderer/media... content/renderer/media/media_permission_dispatcher.cc:133: DVLOG(2) << "Request not found."; Surely this can never happen, since requests are only removed when satisfied, or during deletion, and after deletion the WeakPtr to |this| will be invalid, so OnPermissionStatus will be dropped silently?
The patch itself looks fine in content/renderer/render_frame_impl*. I do have one overall question. https://codereview.chromium.org/1464183002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1464183002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:5659: media_permission_dispatcher_ = new MediaPermissionDispatcher(this); I don't see us cleaning it up in RenderFrameImpl itself. Are we intentionally leaking this object or is some other object responsible for cleaning it up?
rebase only
comments addressed
xhwang@chromium.org changed reviewers: - guoweis@chromium.org
Removing guoweis from the reviewer list since he has left Chromium. Otherwise, PTAL! https://codereview.chromium.org/1464183002/diff/180001/content/renderer/media... File content/renderer/media/media_permission_dispatcher.cc (right): https://codereview.chromium.org/1464183002/diff/180001/content/renderer/media... content/renderer/media/media_permission_dispatcher.cc:61: // Use BindToCurrentLoop to bind the callback to the current thread. On 2016/01/29 22:59:15, Wez wrote: > nit: This comment seems redundant. Done. https://codereview.chromium.org/1464183002/diff/180001/content/renderer/media... content/renderer/media/media_permission_dispatcher.cc:133: DVLOG(2) << "Request not found."; On 2016/01/29 22:59:15, Wez wrote: > Surely this can never happen, since requests are only removed when satisfied, or > during deletion, and after deletion the WeakPtr to |this| will be invalid, so > OnPermissionStatus will be dropped silently? Good catch. Use DCHECK since this should never happen. https://codereview.chromium.org/1464183002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1464183002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:5659: media_permission_dispatcher_ = new MediaPermissionDispatcher(this); On 2016/02/01 19:34:13, nasko wrote: > I don't see us cleaning it up in RenderFrameImpl itself. Are we intentionally > leaking this object or is some other object responsible for cleaning it up? MediaPermissionDispatcher is a RenderFrameObserver, which will be released when the frame is destroyed. We have many similar members in RenderFrameImpl, e.g. MidiDispatcher. Does this address your concern or do I miss anything? In a later CL [1] I'll update MediaPermissionDispatcher so that it will NOT be a RenderFrameObserver, and RenderFrameImpl will just hold a scoped_ptr of MediaPermissionDispatcher. [1] https://chromiumcodereview.appspot.com/1470233002/
On 2016/02/02 00:40:34, xhwang wrote: > Removing guoweis from the reviewer list since he has left Chromium. > > Otherwise, PTAL! > > https://codereview.chromium.org/1464183002/diff/180001/content/renderer/media... > File content/renderer/media/media_permission_dispatcher.cc (right): > > https://codereview.chromium.org/1464183002/diff/180001/content/renderer/media... > content/renderer/media/media_permission_dispatcher.cc:61: // Use > BindToCurrentLoop to bind the callback to the current thread. > On 2016/01/29 22:59:15, Wez wrote: > > nit: This comment seems redundant. > > Done. > > https://codereview.chromium.org/1464183002/diff/180001/content/renderer/media... > content/renderer/media/media_permission_dispatcher.cc:133: DVLOG(2) << "Request > not found."; > On 2016/01/29 22:59:15, Wez wrote: > > Surely this can never happen, since requests are only removed when satisfied, > or > > during deletion, and after deletion the WeakPtr to |this| will be invalid, so > > OnPermissionStatus will be dropped silently? > > Good catch. Use DCHECK since this should never happen. > > https://codereview.chromium.org/1464183002/diff/180001/content/renderer/rende... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/1464183002/diff/180001/content/renderer/rende... > content/renderer/render_frame_impl.cc:5659: media_permission_dispatcher_ = new > MediaPermissionDispatcher(this); > On 2016/02/01 19:34:13, nasko wrote: > > I don't see us cleaning it up in RenderFrameImpl itself. Are we intentionally > > leaking this object or is some other object responsible for cleaning it up? > > MediaPermissionDispatcher is a RenderFrameObserver, which will be released when > the frame is destroyed. We have many similar members in RenderFrameImpl, e.g. > MidiDispatcher. > > Does this address your concern or do I miss anything? Yes, it does. Thanks! > In a later CL [1] I'll update MediaPermissionDispatcher so that it will NOT be a > RenderFrameObserver, and RenderFrameImpl will just hold a scoped_ptr of > MediaPermissionDispatcher. > > [1] https://chromiumcodereview.appspot.com/1470233002/
nasko: Would you like to OWNERS approve the changes to render_frame_impl.*? sergeyu: Please OWNERS review content/renderer/p2p changes or recommend other owners to review. Thanks!
https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/content... File content/content_renderer.gypi (left): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/content... content/content_renderer.gypi:303: 'renderer/media/media_permission_dispatcher_impl.cc', Did you also want to remove these files? I think you did have them removed in previous patchsets, but it was lost somehow. https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/media/media_permission_dispatcher.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/media/media_permission_dispatcher.cc:53: requests_.clear(); nit: don't need this. https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/render_frame_impl.cc:5952: media_permission_dispatcher_ = new MediaPermissionDispatcher(this); This is not thread-safe. What thread is it supposed to be called on? https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/render_frame_impl.h (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/render_frame_impl.h:1093: MediaPermissionDispatcher* media_permission_dispatcher_; Should this be a scoped_ptr<>? As far as I can tell it's leaked when RenderFrame is destroyed.
comments addressed
sergeyu: Thanks for the review! Please take a look again. https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/content... File content/content_renderer.gypi (left): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/content... content/content_renderer.gypi:303: 'renderer/media/media_permission_dispatcher_impl.cc', On 2016/02/02 22:11:02, Sergey Ulanov wrote: > Did you also want to remove these files? I think you did have them removed in > previous patchsets, but it was lost somehow. Thanks for catching that. It seems this is lost during rebase. Done. https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/media/media_permission_dispatcher.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/media/media_permission_dispatcher.cc:53: requests_.clear(); On 2016/02/02 22:11:02, Sergey Ulanov wrote: > nit: don't need this. Done. https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/render_frame_impl.cc:5952: media_permission_dispatcher_ = new MediaPermissionDispatcher(this); On 2016/02/02 22:11:02, Sergey Ulanov wrote: > This is not thread-safe. What thread is it supposed to be called on? RenderFrameImpl lives exclusively on the main thread. So all RenderFrameImpl methods should only be called on the main thread, which I *think* is the case for PeerConnectionDependencyFactory::CreatePeerConnection(). Otherwise even the call to RenderFrameImpl::FromWebFrame() may not be safe. https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/render_frame_impl.h (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/render_frame_impl.h:1093: MediaPermissionDispatcher* media_permission_dispatcher_; On 2016/02/02 22:11:02, Sergey Ulanov wrote: > Should this be a scoped_ptr<>? As far as I can tell it's leaked when RenderFrame > is destroyed. (reposting a previous reply with edits) MediaPermissionDispatcher is a RenderFrameObserver, which will be released when the frame is destroyed [1]. We have many similar members in RenderFrameImpl, e.g. MidiDispatcher. I agree that this is a bit confusing. It's this way for historical reasons. Actually in a later CL [2] I'll update MediaPermissionDispatcher so that it will NOT be a RenderFrameObserver, and RenderFrameImpl will just hold a scoped_ptr of MediaPermissionDispatcher. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren... [2] https://chromiumcodereview.appspot.com/1470233002/
content/renderer/render_frame_impl.* LGTM
sergeyu: kindly ping!
lgtm https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/render_frame_impl.cc:5952: media_permission_dispatcher_ = new MediaPermissionDispatcher(this); On 2016/02/03 00:17:20, xhwang wrote: > On 2016/02/02 22:11:02, Sergey Ulanov wrote: > > This is not thread-safe. What thread is it supposed to be called on? > > RenderFrameImpl lives exclusively on the main thread. So all RenderFrameImpl > methods should only be called on the main thread, which I *think* is the case > for PeerConnectionDependencyFactory::CreatePeerConnection(). Otherwise even the > call to RenderFrameImpl::FromWebFrame() may not be safe. Add a DCHECK? https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/render_frame_impl.h (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/render_frame_impl.h:1093: MediaPermissionDispatcher* media_permission_dispatcher_; On 2016/02/03 00:17:20, xhwang wrote: > On 2016/02/02 22:11:02, Sergey Ulanov wrote: > > Should this be a scoped_ptr<>? As far as I can tell it's leaked when > RenderFrame > > is destroyed. > > (reposting a previous reply with edits) > > MediaPermissionDispatcher is a RenderFrameObserver, which will be released when > the frame is destroyed [1]. We have many similar members in RenderFrameImpl, > e.g. > MidiDispatcher. > > I agree that this is a bit confusing. It's this way for historical reasons. > > Actually in a later CL [2] I'll update MediaPermissionDispatcher so that it will > NOT be a > RenderFrameObserver, and RenderFrameImpl will just hold a scoped_ptr of > MediaPermissionDispatcher. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren... > > [2] https://chromiumcodereview.appspot.com/1470233002/ I see. Add a comment here? similar to web_user_media_client_
comments addressed
https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/render_frame_impl.cc:5952: media_permission_dispatcher_ = new MediaPermissionDispatcher(this); On 2016/02/04 20:50:23, Sergey Ulanov wrote: > On 2016/02/03 00:17:20, xhwang wrote: > > On 2016/02/02 22:11:02, Sergey Ulanov wrote: > > > This is not thread-safe. What thread is it supposed to be called on? > > > > RenderFrameImpl lives exclusively on the main thread. So all RenderFrameImpl > > methods should only be called on the main thread, which I *think* is the case > > for PeerConnectionDependencyFactory::CreatePeerConnection(). Otherwise even > the > > call to RenderFrameImpl::FromWebFrame() may not be safe. > > Add a DCHECK? In RenderFrameImpl we don't have this kind of DCHECK anywhere. It's the callers responsibility to make sure RenderFrameImpl methods are called on the correct thread. Maybe we can add a DCHECK in PeerConnectionDependencyFactory code? https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... File content/renderer/render_frame_impl.h (right): https://chromiumcodereview.appspot.com/1464183002/diff/240001/content/rendere... content/renderer/render_frame_impl.h:1093: MediaPermissionDispatcher* media_permission_dispatcher_; On 2016/02/04 20:50:23, Sergey Ulanov wrote: > On 2016/02/03 00:17:20, xhwang wrote: > > On 2016/02/02 22:11:02, Sergey Ulanov wrote: > > > Should this be a scoped_ptr<>? As far as I can tell it's leaked when > > RenderFrame > > > is destroyed. > > > > (reposting a previous reply with edits) > > > > MediaPermissionDispatcher is a RenderFrameObserver, which will be released > when > > the frame is destroyed [1]. We have many similar members in RenderFrameImpl, > > e.g. > > MidiDispatcher. > > > > I agree that this is a bit confusing. It's this way for historical reasons. > > > > Actually in a later CL [2] I'll update MediaPermissionDispatcher so that it > will > > NOT be a > > RenderFrameObserver, and RenderFrameImpl will just hold a scoped_ptr of > > MediaPermissionDispatcher. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren... > > > > [2] https://chromiumcodereview.appspot.com/1470233002/ > > I see. Add a comment here? similar to web_user_media_client_ Done.
rebase only
If there are no objections I'am gonna land this CL later this afternoon. Thanks for all the valuable comments. I did learn a lot!
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guoweis@chromium.org, perkj@chromium.org, sergeyu@chromium.org, nasko@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1464183002/#ps300001 (title: "rebase only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464183002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464183002/300001
Message was sent while issue was closed.
Description was changed from
==========
media: Simplify MediaPermissionDispatcher.
This partially reverts commit f4282b031b403e1358125e0db371bda17cac604a and
supports calling {Has|Request}Permission on any thread with a simpler way.
This would also make it easier to use MediaPermissionDispatcher in other places
(e.g. in the GPU process).
BUG=520101,559839
==========
to
==========
media: Simplify MediaPermissionDispatcher.
This partially reverts commit f4282b031b403e1358125e0db371bda17cac604a and
supports calling {Has|Request}Permission on any thread with a simpler way.
This would also make it easier to use MediaPermissionDispatcher in other places
(e.g. in the GPU process).
BUG=520101,559839
==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from
==========
media: Simplify MediaPermissionDispatcher.
This partially reverts commit f4282b031b403e1358125e0db371bda17cac604a and
supports calling {Has|Request}Permission on any thread with a simpler way.
This would also make it easier to use MediaPermissionDispatcher in other places
(e.g. in the GPU process).
BUG=520101,559839
==========
to
==========
media: Simplify MediaPermissionDispatcher.
This partially reverts commit f4282b031b403e1358125e0db371bda17cac604a and
supports calling {Has|Request}Permission on any thread with a simpler way.
This would also make it easier to use MediaPermissionDispatcher in other places
(e.g. in the GPU process).
BUG=520101,559839
Committed: https://crrev.com/5206d4e628900aad06a755f81c41a95b667a247d
Cr-Commit-Position: refs/heads/master@{#373717}
==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/5206d4e628900aad06a755f81c41a95b667a247d Cr-Commit-Position: refs/heads/master@{#373717} |
