Fix DCHECK getting hit on destruction of GpuJpegDecodeAcceleratorHost

media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.h"
 
 #include <stddef.h>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 13 matching lines @@
 // Class to receive AcceleratedJpegDecoderHostMsg_DecodeAck IPC message on IO
 // thread. This does very similar what MessageFilter usually does. It is not
 // MessageFilter because GpuChannelHost doesn't support AddFilter.
 class GpuJpegDecodeAcceleratorHost::Receiver : public IPC::Listener,
                                                public base::NonThreadSafe {
  public:
   Receiver(Client* client,
            const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner)
       : client_(client),
         io_task_runner_(io_task_runner),
-        weak_factory_for_io_(this) {
+        weak_factory_for_io_(
+            base::MakeUnique<base::WeakPtrFactory<Receiver>>(this)) {

[sandersd (OOO until July 31), 2017/03/30 20:58:50]

I recommend creating a WeakPtr here in the constructor, and handing out copies
of it from AsWeakPtrForIO().

[chfremer, 2017/03/30 21:19:45]

Done.

     DCHECK(CalledOnValidThread());
   }
 
-  ~Receiver() override { DCHECK(CalledOnValidThread()); }
+  ~Receiver() override {
+    DCHECK(CalledOnValidThread());
+    if (weak_factory_for_io_->HasWeakPtrs())

[sandersd (OOO until July 31), 2017/03/30 20:58:49]

I recommend always posting this task, it would be more clear if this factory was
only ever destructed on the IO thread.

Might be worth a comment that this leaks if the IO thread is stopped, but that's
not something that's usually commented on. (Just possibly relevant in this case
since there has been a bug.)

[chfremer, 2017/03/30 21:19:45]

Done.

+      io_task_runner_->DeleteSoon(FROM_HERE, weak_factory_for_io_.release());
+  }
 
-  void InvalidateWeakPtr(base::WaitableEvent* event) {
+  void InvalidateWeakPtrOnIOThread(base::WaitableEvent* event) {
     DCHECK(io_task_runner_->BelongsToCurrentThread());
-    weak_factory_for_io_.InvalidateWeakPtrs();
+    weak_factory_for_io_->InvalidateWeakPtrs();
     event->Signal();
   }
 
   // IPC::Listener implementation.
   void OnChannelError() override {
     DCHECK(io_task_runner_->BelongsToCurrentThread());
 
     OnDecodeAck(kInvalidBitstreamBufferId, PLATFORM_FAILURE);
   }
 
   bool OnMessageReceived(const IPC::Message& msg) override {
     DCHECK(io_task_runner_->BelongsToCurrentThread());
 
     bool handled = true;
     IPC_BEGIN_MESSAGE_MAP(GpuJpegDecodeAcceleratorHost::Receiver, msg)
       IPC_MESSAGE_HANDLER(AcceleratedJpegDecoderHostMsg_DecodeAck, OnDecodeAck)
       IPC_MESSAGE_UNHANDLED(handled = false)
     IPC_END_MESSAGE_MAP()
     DCHECK(handled);
     return handled;
   }
 
   base::WeakPtr<IPC::Listener> AsWeakPtrForIO() {
-    return weak_factory_for_io_.GetWeakPtr();
+    return weak_factory_for_io_->GetWeakPtr();
   }
 
  private:
   void OnDecodeAck(int32_t bitstream_buffer_id, Error error) {
     DCHECK(io_task_runner_->BelongsToCurrentThread());
 
     if (!client_)
       return;
 
     if (error == JpegDecodeAccelerator::NO_ERRORS) {
       client_->VideoFrameReady(bitstream_buffer_id);
     } else {
       // Only NotifyError once.
       // Client::NotifyError() may trigger deletion of |this| (on another
       // thread), so calling it needs to be the last thing done on this stack!
       JpegDecodeAccelerator::Client* client = nullptr;
       std::swap(client, client_);
       client->NotifyError(bitstream_buffer_id, error);
     }
   }
 
   Client* client_;
 
   // GPU IO task runner.
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
 
   // Weak pointers will be invalidated on IO thread.
-  base::WeakPtrFactory<Receiver> weak_factory_for_io_;
+  std::unique_ptr<base::WeakPtrFactory<Receiver>> weak_factory_for_io_;
 
   DISALLOW_COPY_AND_ASSIGN(Receiver);
 };
 
 GpuJpegDecodeAcceleratorHost::GpuJpegDecodeAcceleratorHost(
     scoped_refptr<gpu::GpuChannelHost> channel,
     int32_t route_id,
     const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner)
     : channel_(std::move(channel)),
       decoder_route_id_(route_id),
       io_task_runner_(io_task_runner) {
   DCHECK(channel_);
   DCHECK_NE(decoder_route_id_, MSG_ROUTING_NONE);
 }
 
 GpuJpegDecodeAcceleratorHost::~GpuJpegDecodeAcceleratorHost() {
   DCHECK(CalledOnValidThread());
   Send(new AcceleratedJpegDecoderMsg_Destroy(decoder_route_id_));
 
   if (receiver_) {
     channel_->RemoveRoute(decoder_route_id_);
 
     // Invalidate weak ptr of |receiver_|. After that, no more messages will be
     // routed to |receiver_| on IO thread.
     base::WaitableEvent event(base::WaitableEvent::ResetPolicy::AUTOMATIC,
                               base::WaitableEvent::InitialState::NOT_SIGNALED);
     bool task_expected_to_run = io_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&Receiver::InvalidateWeakPtr,
+        FROM_HERE, base::Bind(&Receiver::InvalidateWeakPtrOnIOThread,
                               base::Unretained(receiver_.get()),

[sandersd (OOO until July 31), 2017/03/30 20:58:50]

I'd add a comment here explaining that Unretained is safe because we always wait
until the method finishes executing if there is any chance the task actually
executes.

[chfremer, 2017/03/30 21:19:45]

Done.

                               base::Unretained(&event)));
     // If the current call is happening during the browser shutdown, the
     // |io_task_runner_| may no longer be accepting tasks.
     if (task_expected_to_run)
       event.Wait();
   }
 }
 
 bool GpuJpegDecodeAcceleratorHost::Initialize(
     JpegDecodeAccelerator::Client* client) {
@@ skipping 65 matching lines @@
   if (!channel_->Send(message)) {
     DLOG(ERROR) << "Send(" << message->type() << ") failed";
   }
 }
 
 base::WeakPtr<IPC::Listener> GpuJpegDecodeAcceleratorHost::GetReceiver() {
   return receiver_->AsWeakPtrForIO();
 }
 
 }  // namespace media