Fix potential UAF in usage of WeakPtrFactory

On destruction of AssistantAudioDecoder, the first member to be
destroyed is the WeakPtrFactory, which happens before the media thread.
However, if the media thread is currently running a task, that task's
could call WeakPtrFactory::GetWeakPtr() on an already destroyed
WeakPtrFactory. In general, WeakPtrs should only be bound/dereferenced
on the same thread, although an existing WeakPtr can be copied on any
thread.

Bug: None
Change-Id: I182292e8a51bf1e91934fbce237245bcf58be177
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2542932
Commit-Queue: Anand K Mistry <amistry@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Jeroen Dhollander <jeroendh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828568}

chromeos/services/assistant/audio_decoder/assistant_audio_decoder.cc

@@ -33,6 +33,7 @@ AssistantAudioDecoder::AssistantAudioDecoder(
       data_source_(std::make_unique<IPCDataSource>(std::move(data_source))),
       media_thread_(std::make_unique<base::Thread>("media_thread")),
       weak_factory_(this) {
+  weak_this_ = weak_factory_.GetWeakPtr();
   CHECK(media_thread_->Start());
   client_.set_disconnect_handler(base::BindOnce(
       &AssistantAudioDecoder::OnConnectionError, base::Unretained(this)));
@@ -83,7 +84,7 @@ void AssistantAudioDecoder::OpenDecoderOnMediaThread() {
   task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&AssistantAudioDecoder::OnDecoderInitializedOnThread,
-                     weak_factory_.GetWeakPtr(), decoder_->sample_rate(),
+                     weak_this_, decoder_->sample_rate(),
                      decoder_->channels()));
 }
 
@@ -100,8 +101,7 @@ void AssistantAudioDecoder::DecodeOnMediaThread() {
 
   task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&AssistantAudioDecoder::OnBufferDecodedOnThread,
-                                weak_factory_.GetWeakPtr(),
-                                std::move(decoded_audio_packets)));
+                                weak_this_, std::move(decoded_audio_packets)));
 }
 
 void AssistantAudioDecoder::CloseDecoderOnMediaThread() {
@@ -111,8 +111,8 @@ void AssistantAudioDecoder::CloseDecoderOnMediaThread() {
 
   closed_ = true;
   task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&AssistantAudioDecoder::RunCallbacksAsClosed,
-                                weak_factory_.GetWeakPtr()));
+      FROM_HERE,
+      base::BindOnce(&AssistantAudioDecoder::RunCallbacksAsClosed, weak_this_));
 }
 
 void AssistantAudioDecoder::OnDecoderInitializedOnThread(

chromeos/services/assistant/audio_decoder/assistant_audio_decoder.h

@@ -60,6 +60,12 @@ class AssistantAudioDecoder : public mojom::AssistantAudioDecoder {
   bool closed_ = false;
   bool read_error_ = false;
 
+  // Weak reference to |this| for use by the media thread. Note, ordering is
+  // important here. This _must_ appear before |media_thread_| so that the media
+  // thread is destroyed (and joined) first, and hence any attempt to copy
+  // |weak_this_| happens before it is destroyed.
+  base::WeakPtr<AssistantAudioDecoder> weak_this_;
+
   std::unique_ptr<media::DataSource> data_source_;
   std::unique_ptr<media::BlockingUrlProtocol> protocol_;
   std::unique_ptr<media::AudioFileReader> decoder_;