media: Simplify MediaPermissionDispatcher.

content/renderer/media/media_permission_dispatcher.cc

Index: content/renderer/media/media_permission_dispatcher.cc
diff --git a/content/renderer/media/media_permission_dispatcher.cc b/content/renderer/media/media_permission_dispatcher.cc
index 666d194e76b901963bddba78ea951754384d2a6f..14d29285495d71abea57f2f443fad4a9015ffa4f 100644
--- a/content/renderer/media/media_permission_dispatcher.cc
+++ b/content/renderer/media/media_permission_dispatcher.cc
@@ -4,14 +4,46 @@
 
 #include "content/renderer/media/media_permission_dispatcher.h"
 
+#include "base/bind.h"
 #include "base/logging.h"
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#include "content/public/common/service_registry.h"
+#include "content/public/renderer/render_frame.h"
+#include "media/base/bind_to_current_loop.h"
+#include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
+#include "url/gurl.h"
+
+namespace {
+
+using Type = media::MediaPermission::Type;
+
+content::PermissionName MediaPermissionTypeToPermissionName(Type type) {
+  switch (type) {
+    case Type::PROTECTED_MEDIA_IDENTIFIER:
+      return content::PERMISSION_NAME_PROTECTED_MEDIA_IDENTIFIER;
+    case Type::AUDIO_CAPTURE:
+      return content::PERMISSION_NAME_AUDIO_CAPTURE;
+    case Type::VIDEO_CAPTURE:
+      return content::PERMISSION_NAME_VIDEO_CAPTURE;
+  }
+  NOTREACHED();
+  return content::PERMISSION_NAME_PROTECTED_MEDIA_IDENTIFIER;
+}
+
+}  // namespace
 
 namespace content {
 
-MediaPermissionDispatcher::MediaPermissionDispatcher() : next_request_id_(0) {}
+MediaPermissionDispatcher::MediaPermissionDispatcher(RenderFrame* render_frame)
+    : RenderFrameObserver(render_frame),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      next_request_id_(0),
+      weak_factory_(this) {}
 
 MediaPermissionDispatcher::~MediaPermissionDispatcher() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
   // Fire all pending callbacks with |false|.
   for (auto& request : requests_)
     request.second.Run(false);
@@ -19,19 +51,80 @@ MediaPermissionDispatcher::~MediaPermissionDispatcher() {
   requests_.clear();
 }
 
+void MediaPermissionDispatcher::HasPermission(

[Sergey Ulanov, 2015/11/24 21:08:42]

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?

[xhwang, 2015/11/24 22:54:59]

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.

[guoweis_left_chromium, 2015/11/24 23:15:07]

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.

+    Type type,
+    const GURL& security_origin,
+    const PermissionStatusCB& permission_status_cb) {
+  if (!task_runner_->RunsTasksOnCurrentThread()) {
+    // Use BindToCurrentLoop to bind the callback to the current thread.
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&MediaPermissionDispatcher::HasPermission,
+                              weak_factory_.GetWeakPtr(), type, security_origin,

[Sergey Ulanov, 2015/11/24 21:08:42]

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.

[xhwang, 2015/11/24 22:54:59]

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.

[Wez, 2015/11/25 00:44:48]

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.
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!

[Sergey Ulanov, 2015/11/25 01:01:36]

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?
And that's the problem in this code: GetWeakPtr() is called on arbitrary
threads, potentially in parallel.

[Wez, 2015/11/25 01:09:00]

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.
Exactly.

[xhwang, 2015/11/25 01:18:02]

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?

[Wez, 2015/11/25 02:09:54]

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..?
Yeah, I'll add something.

[xhwang, 2015/11/25 07:06:03]

(omitting old comments)
Here's an example where we use two weak ptr factories:

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[Wez, 2015/11/25 20:01:40]

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)

+                              media::BindToCurrentLoop(permission_status_cb)));
+    return;
+  }
+
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  if (!permission_service_.get()) {
+    render_frame()->GetServiceRegistry()->ConnectToRemoteService(
+        mojo::GetProxy(&permission_service_));
+  }
+
+  int request_id = RegisterCallback(permission_status_cb);
+  DVLOG(2) << __FUNCTION__ << ": request ID " << request_id;
+
+  permission_service_->HasPermission(
+      MediaPermissionTypeToPermissionName(type), security_origin.spec(),
+      base::Bind(&MediaPermissionDispatcher::OnPermissionStatus,
+                 base::Unretained(this), request_id));
+}
+
+void MediaPermissionDispatcher::RequestPermission(
+    Type type,
+    const GURL& security_origin,
+    const PermissionStatusCB& permission_status_cb) {
+  if (!task_runner_->RunsTasksOnCurrentThread()) {
+    // Use BindToCurrentLoop to bind the callback to the current thread.
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&MediaPermissionDispatcher::RequestPermission,
+                              weak_factory_.GetWeakPtr(), type, security_origin,
+                              media::BindToCurrentLoop(permission_status_cb)));
+    return;
+  }
+
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  if (!permission_service_.get()) {
+    render_frame()->GetServiceRegistry()->ConnectToRemoteService(
+        mojo::GetProxy(&permission_service_));
+  }
+
+  int request_id = RegisterCallback(permission_status_cb);
+  DVLOG(2) << __FUNCTION__ << ": request ID " << request_id;
+
+  permission_service_->RequestPermission(
+      MediaPermissionTypeToPermissionName(type), security_origin.spec(),
+      blink::WebUserGestureIndicator::isProcessingUserGesture(),
+      base::Bind(&MediaPermissionDispatcher::OnPermissionStatus,
+                 base::Unretained(this), request_id));
+}
+
 uint32_t MediaPermissionDispatcher::RegisterCallback(
     const PermissionStatusCB& permission_status_cb) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
   uint32_t request_id = next_request_id_++;
   DCHECK(!requests_.count(request_id));
   requests_[request_id] = permission_status_cb;
+
   return request_id;
 }
 
-void MediaPermissionDispatcher::DeliverResult(uint32_t request_id,
-                                              bool granted) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DVLOG(2) << __FUNCTION__ << ": (" << request_id << ", " << granted << ")";
+void MediaPermissionDispatcher::OnPermissionStatus(uint32_t request_id,
+                                                   PermissionStatus status) {
+  DVLOG(2) << __FUNCTION__ << ": (" << request_id << ", " << status << ")";
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
 
   RequestMap::iterator iter = requests_.find(request_id);
   if (iter == requests_.end()) {
@@ -42,7 +135,7 @@ void MediaPermissionDispatcher::DeliverResult(uint32_t request_id,
   PermissionStatusCB permission_status_cb = iter->second;
   requests_.erase(iter);
 
-  permission_status_cb.Run(granted);
+  permission_status_cb.Run(status == PERMISSION_STATUS_GRANTED);
 }
 
 }  // namespace content