Revert "Fix UAF in TtsPlatformImpl if a BrowserContext is deleted."

This reverts commit 9cca480.

Reason for revert: <INSERT REASONING HERE>
See flakey failures
https://analysis.chromium.org/p/chromium/flake-portal/flakes/occurrences?key=ag9zfmZpbmRpdC1mb3ItbWVyTwsSBUZsYWtlIkRjaHJvbWl1bUBicm93c2VyX3Rlc3RzQENocm9tZVZveEJyYWlsbGVUYWJsZVRlc3QudGVzdEdldFVuY29udHJhY3RlZAw

[7369:7369:0522/203957.919406:FATAL:chrome_extensions_browser_client.cc(148)] Check failed: context.
#0 0x55cfe9dfc549 base::debug::CollectStackTrace()
#1 0x55cfe9d09be3 base::debug::StackTrace::StackTrace()
#2 0x55cfe9d20d7f logging::LogMessage::~LogMessage()
#3 0x55cfe9d2156e logging::LogMessage::~LogMessage()
#4 0x55cfed7b0e42 extensions::ChromeExtensionsBrowserClient::GetOriginalContext()
#5 0x55cfed58f4c3 BrowserContextKeyedServiceFactory::GetContextToUse()
#6 0x55cfebd733c3 KeyedServiceFactory::GetServiceForContext()
#7 0x55cfea38f279 extensions::TtsExtensionEventHandler::OnTtsEvent()
#8 0x55cfe73698c8 content::TtsUtteranceImpl::OnTtsEvent()
#9 0x55cfe7365120 content::TtsControllerImpl::ClearUtteranceQueue()
#10 0x55cfe7365f03 content::TtsControllerImpl::StopInternal()
#11 0x55cfe7367071 content::TtsControllerImpl::OnBrowserContextDestroyed()
#12 0x55cfe6d1d18c content::BrowserContext::~BrowserContext()
#13 0x55cfea0f813f ProfileImpl::~ProfileImpl()
#14 0x55cfea0f816e ProfileImpl::~ProfileImpl()
#15 0x55cfea0f1f72 ProfileDestroyer::DestroyRegularProfileNow()
#16 0x55cfea0f1d00 ProfileDestroyer::DestroyProfileWhenAppropriate()
#17 0x55cfea1137e8 std::__1::unique_ptr<>::~unique_ptr()
#18 0x55cfea114ee3 std::__1::__tree<>::destroy()
#19 0x55cfea11473b ProfileManager::~ProfileManager()
#20 0x55cfea10bfae ProfileManager::~ProfileManager()
#21 0x55cfe9eac15a BrowserProcessImpl::StartTearDown()
#22 0x55cfe9eaab2a ChromeBrowserMainParts::PostMainMessageLoopRun()
#23 0x55cfe51c9e74 chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun()
#24 0x55cfe6d41ccf content::BrowserMainLoop::ShutdownThreadsAndCleanUp()
#25 0x55cfe6d43b6d content::BrowserMainRunnerImpl::Shutdown()
#26 0x55cfe6d3eb02 content::BrowserMain()
#27 0x55cfe97ac717 content::ContentMainRunnerImpl::RunServiceManager()
#28 0x55cfe97ac2cf content::ContentMainRunnerImpl::Run()
#29 0x55cfec99806a service_manager::Main()
#30 0x55cfe7f60444 content::ContentMain()
#31 0x55cfea5622a4 content::BrowserTestBase::SetUp()
#32 0x55cfe9cf633b InProcessBrowserTest::SetUp()
#33 0x55cfe579858e testing::Test::Run()
#34 0x55cfe5799948 testing::TestInfo::Run()
#35 0x55cfe579a5e7 testing::TestSuite::Run()
chromium#36 0x55cfe57aa747 testing::internal::UnitTestImpl::RunAllTests()
#37 0x55cfe57aa179 testing::UnitTest::Run()
#38 0x55cfe9e477e2 base::TestSuite::Run()
chromium#39 0x55cfe9cdf0e7 BrowserTestSuiteRunnerChromeOS::RunTestSuite()
#40 0x55cfea5a72f4 content::LaunchTests()
#41 0x55cfe9cdf494 LaunchChromeTests()
chromium#42 0x55cfe9cdf032 main
#43 0x7f421a16f830 __libc_start_main
#44 0x55cfe29b442a _start

Fixed: Fixed: 1085878, 1085877
Fixed: 1085878, 1085877

Original change's description:
> Fix UAF in TtsPlatformImpl if a BrowserContext is deleted.
>
> Bug: 1081350
> Change-Id: I2b1824abefbd7fc3e8ce1c0cb433896161bab4e5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211123
> Reviewed-by: David Tseng <dtseng@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#771222}

TBR=dmazzoni@chromium.org,dtseng@chromium.org,jam@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1081350
Change-Id: I88ec7e523fbe56845b8480b112535b2f8e18a520
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2213553
Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: David Tseng <dtseng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771379}

chrome/browser/speech/extension_api/tts_extension_apitest.cc

@@ -30,17 +30,16 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using ::testing::_;
 using ::testing::AnyNumber;
 using ::testing::DoAll;
-using ::testing::InSequence;
 using ::testing::Invoke;
+using ::testing::InSequence;
 using ::testing::InvokeWithoutArgs;
-using ::testing::NiceMock;
 using ::testing::Return;
 using ::testing::SaveArg;
 using ::testing::SetArgPointee;
 using ::testing::StrictMock;
+using ::testing::_;
 
 namespace {
 int g_saved_utterance_id;
@@ -285,13 +284,7 @@ class TtsApiTest : public ExtensionApiTest {
     return false;
   }
 
-  void IgnoreAnyAdditionalCallsToMockPlatformImpl() {
-    content::TtsController::GetInstance()->SetTtsPlatform(
-        &nice_mock_platform_impl_);
-  }
-
   StrictMock<MockTtsPlatformImpl> mock_platform_impl_;
-  NiceMock<MockTtsPlatformImpl> nice_mock_platform_impl_;
 };
 
 IN_PROC_BROWSER_TEST_F(TtsApiTest, PlatformSpeakOptionalArgs) {
@@ -318,7 +311,6 @@ IN_PROC_BROWSER_TEST_F(TtsApiTest, PlatformSpeakOptionalArgs) {
   EXPECT_CALL(mock_platform_impl_, DoSpeak(_, "Echo", _, _, _))
       .WillOnce(Return());
   ASSERT_TRUE(RunExtensionTest("tts/optional_args")) << message_;
-  IgnoreAnyAdditionalCallsToMockPlatformImpl();
 }
 
 IN_PROC_BROWSER_TEST_F(TtsApiTest, PlatformSpeakFinishesImmediately) {

content/browser/browser_context.cc

@@ -43,7 +43,6 @@
 #include "content/browser/media/browser_feature_provider.h"
 #include "content/browser/permissions/permission_controller_impl.h"
 #include "content/browser/push_messaging/push_messaging_router.h"
-#include "content/browser/speech/tts_controller_impl.h"
 #include "content/browser/storage_partition_impl_map.h"
 #include "content/common/child_process_host_impl.h"
 #include "content/public/browser/blob_handle.h"
@@ -485,8 +484,6 @@ BrowserContext::~BrowserContext() {
   if (GetUserData(kDownloadManagerKeyName))
     GetDownloadManager(this)->Shutdown();
 
-  TtsControllerImpl::GetInstance()->OnBrowserContextDestroyed(this);
-
   TRACE_EVENT_NESTABLE_ASYNC_END1(
       "shutdown", "BrowserContext::NotifyWillBeDestroyed() called.", this,
       "browser_context", this);

content/browser/speech/tts_controller_impl.cc

@@ -98,32 +98,26 @@ void TtsControllerImpl::SpeakOrEnqueue(
   // If we're paused and we get an utterance that can't be queued,
   // flush the queue but stay in the paused state.
   if (paused_ && !utterance->GetCanEnqueue()) {
-    utterance_deque_.emplace_back(std::move(utterance));
+    utterance_queue_.emplace(std::move(utterance));
     Stop();
     paused_ = true;
     return;
   }
 
   if (paused_ || (IsSpeaking() && utterance->GetCanEnqueue())) {
-    utterance_deque_.emplace_back(std::move(utterance));
+    utterance_queue_.emplace(std::move(utterance));
   } else {
     Stop();
     SpeakNow(std::move(utterance));
   }
 }
 
 void TtsControllerImpl::Stop() {
-  StopInternal(GURL(), true);
+  Stop(GURL());
 }
 
 void TtsControllerImpl::Stop(const GURL& source_url) {
-  StopInternal(source_url, true);
-}
-
-void TtsControllerImpl::StopInternal(const GURL& source_url,
-                                     bool record_user_action) {
-  if (record_user_action)
-    base::RecordAction(base::UserMetricsAction("TextToSpeech.Stop"));
+  base::RecordAction(base::UserMetricsAction("TextToSpeech.Stop"));
 
   paused_ = false;
 
@@ -278,13 +272,13 @@ void TtsControllerImpl::RemoveVoicesChangedDelegate(
 void TtsControllerImpl::RemoveUtteranceEventDelegate(
     UtteranceEventDelegate* delegate) {
   // First clear any pending utterances with this delegate.
-  std::deque<std::unique_ptr<TtsUtterance>> old_deque;
-  utterance_deque_.swap(old_deque);
-  while (!old_deque.empty()) {
-    std::unique_ptr<TtsUtterance> utterance = std::move(old_deque.front());
-    old_deque.pop_front();
+  base::queue<std::unique_ptr<TtsUtterance>> old_queue;
+  utterance_queue_.swap(old_queue);
+  while (!old_queue.empty()) {
+    std::unique_ptr<TtsUtterance> utterance = std::move(old_queue.front());
+    old_queue.pop();
     if (utterance->GetEventDelegate() != delegate)
-      utterance_deque_.emplace_back(std::move(utterance));
+      utterance_queue_.emplace(std::move(utterance));
   }
 
   if (current_utterance_ &&
@@ -319,38 +313,12 @@ TtsEngineDelegate* TtsControllerImpl::GetTtsEngineDelegate() {
   return GetTtsControllerDelegate()->GetTtsEngineDelegate();
 }
 
-void TtsControllerImpl::OnBrowserContextDestroyed(
-    BrowserContext* browser_context) {
-  bool did_clear_utterances = false;
-
-  // First clear the BrowserContext from any utterances.
-  for (std::unique_ptr<TtsUtterance>& utterance : utterance_deque_) {
-    if (utterance->GetBrowserContext() == browser_context) {
-      utterance->ClearBrowserContext();
-      did_clear_utterances = true;
-    }
-  }
-
-  if (current_utterance_ &&
-      current_utterance_->GetBrowserContext() == browser_context) {
-    current_utterance_->ClearBrowserContext();
-    did_clear_utterances = true;
-  }
-
-  // If we cleared the BrowserContext from any utterances, stop speech
-  // just to be safe. Don't record user actions because that crashes in
-  // tests because last_active_profile_ isn't cleared.
-  if (did_clear_utterances) {
-    StopInternal(GURL(), false);
-  }
-}
-
 void TtsControllerImpl::SetTtsPlatform(TtsPlatform* tts_platform) {
   tts_platform_ = tts_platform;
 }
 
 int TtsControllerImpl::QueueSize() {
-  return static_cast<int>(utterance_deque_.size());
+  return static_cast<int>(utterance_queue_.size());
 }
 
 TtsPlatform* TtsControllerImpl::GetTtsPlatform() {
@@ -449,7 +417,7 @@ void TtsControllerImpl::OnSpeakFinished(int utterance_id, bool success) {
   // the browser has built-in TTS that isn't loaded yet.
   if (GetTtsPlatform()->LoadBuiltInTtsEngine(
           current_utterance_->GetBrowserContext())) {
-    utterance_deque_.emplace_back(std::move(current_utterance_));
+    utterance_queue_.emplace(std::move(current_utterance_));
     return;
   }
 
@@ -459,10 +427,10 @@ void TtsControllerImpl::OnSpeakFinished(int utterance_id, bool success) {
 }
 
 void TtsControllerImpl::ClearUtteranceQueue(bool send_events) {
-  while (!utterance_deque_.empty()) {
+  while (!utterance_queue_.empty()) {
     std::unique_ptr<TtsUtterance> utterance =
-        std::move(utterance_deque_.front());
-    utterance_deque_.pop_front();
+        std::move(utterance_queue_.front());
+    utterance_queue_.pop();
     if (send_events) {
       utterance->OnTtsEvent(TTS_EVENT_CANCELLED, kInvalidCharIndex,
                             kInvalidLength, std::string());
@@ -487,10 +455,10 @@ void TtsControllerImpl::SpeakNextUtterance() {
 
   // Start speaking the next utterance in the queue.  Keep trying in case
   // one fails but there are still more in the queue to try.
-  while (!utterance_deque_.empty() && !current_utterance_) {
+  while (!utterance_queue_.empty() && !current_utterance_) {
     std::unique_ptr<TtsUtterance> utterance =
-        std::move(utterance_deque_.front());
-    utterance_deque_.pop_front();
+        std::move(utterance_queue_.front());
+    utterance_queue_.pop();
     SpeakNow(std::move(utterance));
   }
 }

content/browser/speech/tts_controller_impl.h

@@ -5,12 +5,12 @@
 #ifndef CONTENT_BROWSER_SPEECH_TTS_CONTROLLER_IMPL_H_
 #define CONTENT_BROWSER_SPEECH_TTS_CONTROLLER_IMPL_H_
 
-#include <deque>
 #include <memory>
 #include <set>
 #include <string>
 #include <vector>
 
+#include "base/containers/queue.h"
 #include "base/gtest_prod_util.h"
 #include "base/json/json_reader.h"
 #include "base/macros.h"
@@ -59,10 +59,6 @@ class CONTENT_EXPORT TtsControllerImpl : public TtsController {
   void SetTtsEngineDelegate(TtsEngineDelegate* delegate) override;
   TtsEngineDelegate* GetTtsEngineDelegate() override;
 
-  // Called directly by ~BrowserContext, because a raw BrowserContext pointer
-  // is stored in an Utterance.
-  void OnBrowserContextDestroyed(BrowserContext* browser_context);
-
   // Testing methods
   void SetTtsPlatform(TtsPlatform* tts_platform) override;
   int QueueSize() override;
@@ -81,7 +77,6 @@ class CONTENT_EXPORT TtsControllerImpl : public TtsController {
   FRIEND_TEST_ALL_PREFIXES(TtsControllerTest, TestGetMatchingVoice);
   FRIEND_TEST_ALL_PREFIXES(TtsControllerTest,
                            TestTtsControllerUtteranceDefaults);
-  FRIEND_TEST_ALL_PREFIXES(TtsControllerTest, TestBrowserContextRemoved);
 
   friend struct base::DefaultSingletonTraits<TtsControllerImpl>;
 
@@ -92,10 +87,6 @@ class CONTENT_EXPORT TtsControllerImpl : public TtsController {
   // |utterance| or delete it if there's an error. Returns true on success.
   void SpeakNow(std::unique_ptr<TtsUtterance> utterance);
 
-  // Implementation of Stop(), with an optional flag to indicate whether
-  // user actions should be recorded.
-  void StopInternal(const GURL& source_url, bool record_user_action);
-
   // Clear the utterance queue. If send_events is true, will send
   // TTS_EVENT_CANCELLED events on each one.
   void ClearUtteranceQueue(bool send_events);
@@ -140,7 +131,7 @@ class CONTENT_EXPORT TtsControllerImpl : public TtsController {
   TtsPlatform* tts_platform_;
 
   // A queue of utterances to speak after the current one finishes.
-  std::deque<std::unique_ptr<TtsUtterance>> utterance_deque_;
+  base::queue<std::unique_ptr<TtsUtterance>> utterance_queue_;
 
   DISALLOW_COPY_AND_ASSIGN(TtsControllerImpl);
 };

content/browser/speech/tts_controller_unittest.cc

@@ -9,8 +9,6 @@
 #include "content/browser/speech/tts_controller_impl.h"
 #include "content/public/browser/tts_controller_delegate.h"
 #include "content/public/browser/tts_platform.h"
-#include "content/public/test/browser_task_environment.h"
-#include "content/public/test/test_browser_context.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/mojom/speech/speech_synthesis.mojom.h"
 
@@ -52,15 +50,9 @@ class MockTtsControllerDelegate : public TtsControllerDelegate {
   MockTtsControllerDelegate() {}
   ~MockTtsControllerDelegate() override {}
 
-  BrowserContext* GetLastBrowserContext() {
-    BrowserContext* result = last_browser_context_;
-    last_browser_context_ = nullptr;
-    return result;
-  }
-
   int GetMatchingVoice(content::TtsUtterance* utterance,
                        std::vector<content::VoiceData>& voices) override {
-    last_browser_context_ = utterance->GetBrowserContext();
+    // Below 0 implies a "native" voice.
     return -1;
   }
 
@@ -74,9 +66,6 @@ class MockTtsControllerDelegate : public TtsControllerDelegate {
   content::TtsEngineDelegate* GetTtsEngineDelegate() override {
     return nullptr;
   }
-
- private:
-  BrowserContext* last_browser_context_ = nullptr;
 };
 
 // Subclass of TtsController with a public ctor and dtor.
@@ -112,45 +101,6 @@ TEST_F(TtsControllerTest, TestTtsControllerShutdown) {
   delete delegate;
 }
 
-TEST_F(TtsControllerTest, TestBrowserContextRemoved) {
-  // Create a controller, mock other stuff, and create a test
-  // browser context.
-  TtsControllerImpl* controller = TtsControllerImpl::GetInstance();
-  MockTtsPlatformImpl platform_impl;
-  MockTtsControllerDelegate delegate;
-  controller->delegate_ = &delegate;
-  controller->SetTtsPlatform(&platform_impl);
-  content::BrowserTaskEnvironment task_environment;
-  auto browser_context = std::make_unique<TestBrowserContext>();
-
-  // Speak an utterances associated with this test browser context.
-  std::unique_ptr<TtsUtterance> utterance1 =
-      TtsUtterance::Create(browser_context.get());
-  utterance1->SetCanEnqueue(true);
-  utterance1->SetSrcId(1);
-  controller->SpeakOrEnqueue(std::move(utterance1));
-
-  // Assert that the delegate was called and it got our browser context.
-  ASSERT_EQ(browser_context.get(), delegate.GetLastBrowserContext());
-
-  // Now queue up a second utterance to be spoken, also associated with
-  // this browser context.
-  std::unique_ptr<TtsUtterance> utterance2 =
-      TtsUtterance::Create(browser_context.get());
-  utterance2->SetCanEnqueue(true);
-  utterance2->SetSrcId(2);
-  controller->SpeakOrEnqueue(std::move(utterance2));
-
-  // Destroy the browser context before the utterance is spoken.
-  browser_context.reset();
-
-  // Now speak the next utterance, and ensure that we don't get the
-  // destroyed browser context.
-  controller->FinishCurrentUtterance();
-  controller->SpeakNextUtterance();
-  ASSERT_EQ(nullptr, delegate.GetLastBrowserContext());
-}
-
 #if !defined(OS_CHROMEOS)
 TEST_F(TtsControllerTest, TestTtsControllerUtteranceDefaults) {
   std::unique_ptr<TtsControllerForTesting> controller =

content/browser/speech/tts_utterance_impl.cc

@@ -182,10 +182,6 @@ BrowserContext* TtsUtteranceImpl::GetBrowserContext() {
   return browser_context_;
 }
 
-void TtsUtteranceImpl::ClearBrowserContext() {
-  browser_context_ = nullptr;
-}
-
 int TtsUtteranceImpl::GetId() {
   return id_;
 }

content/browser/speech/tts_utterance_impl.h

@@ -68,8 +68,6 @@ class CONTENT_EXPORT TtsUtteranceImpl : public TtsUtterance {
   UtteranceEventDelegate* GetEventDelegate() override;
 
   BrowserContext* GetBrowserContext() override;
-  void ClearBrowserContext() override;
-
   int GetId() override;
   bool IsFinished() override;
 
@@ -104,7 +102,7 @@ class CONTENT_EXPORT TtsUtteranceImpl : public TtsUtterance {
   GURL src_url_;
 
   // The delegate to be called when an utterance event is fired.
-  UtteranceEventDelegate* event_delegate_ = nullptr;
+  UtteranceEventDelegate* event_delegate_;
 
   // The parsed options.
   std::string voice_name_;

content/public/browser/tts_utterance.h

@@ -118,7 +118,6 @@ class CONTENT_EXPORT TtsUtterance {
 
   // Getters and setters for internal state.
   virtual BrowserContext* GetBrowserContext() = 0;
-  virtual void ClearBrowserContext() = 0;
   virtual int GetId() = 0;
   virtual bool IsFinished() = 0;
 };