Skip to content

Commit

Permalink
Ensures SpVoice object is released before COM is uninitialized
Browse files Browse the repository at this point in the history
This CL is adding a Shutdown method to the TTS Platform.
Since the TTS Platform is a leaky object, it remains live
until the end of process.

On Windows, the implementation is using COM objects. During
the shutdown, COM is uninitialized. We observed crashes/hangs
during shutdown related to these objects being alive.

R=fdoray@chromium.org

Bug: 1136220
Change-Id: Ib6e43970e479077ec02f684cd56858d9f6028cdf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2468466
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Randy Rossi <rmrossi@chromium.org>
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818096}
  • Loading branch information
bergeret authored and Commit Bot committed Oct 16, 2020
1 parent c483525 commit 277604b
Show file tree
Hide file tree
Showing 15 changed files with 48 additions and 4 deletions.
2 changes: 2 additions & 0 deletions chrome/browser/chromeos/accessibility/speech_monitor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ void SpeechMonitor::SetError(const std::string& error) {
error_ = error;
}

void SpeechMonitor::Shutdown() {}

double SpeechMonitor::CalculateUtteranceDelayMS() {
std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now();
std::chrono::duration<double> time_span =
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/chromeos/accessibility/speech_monitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ class SpeechMonitor : public content::TtsPlatform {
std::string GetError() override;
void ClearError() override;
void SetError(const std::string& error) override;
void Shutdown() override;

void MaybeContinueReplay();
void MaybePrintExpectations();
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/speech/extension_api/tts_extension_apitest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ class MockTtsPlatformImpl : public content::TtsPlatform {
voices->push_back(voice);
}

void Shutdown() override {}

void set_should_fake_get_voices(bool val) { should_fake_get_voices_ = val; }

void SetErrorToEpicFail() { SetError("epic fail"); }
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/speech/tts_chromeos.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class TtsPlatformImplChromeOs : public content::TtsPlatform {
void WillSpeakUtteranceWithVoice(
content::TtsUtterance* utterance,
const content::VoiceData& voice_data) override {}
void Shutdown() override {}

// Get the single instance of this class.
static TtsPlatformImplChromeOs* GetInstance();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class MockTtsPlatformImpl : public TtsPlatform {
void ClearError() override {}
const std::string& GetLastSpokenUtterance() { return last_spoken_utterance_; }
void ClearLastSpokenUtterance() { last_spoken_utterance_ = ""; }
void Shutdown() override {}

private:
std::vector<VoiceData> voices_;
Expand Down
13 changes: 9 additions & 4 deletions content/browser/browser_main_loop.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
#include "content/browser/screenlock_monitor/screenlock_monitor_device_source.h"
#include "content/browser/sms/sms_provider.h"
#include "content/browser/speech/speech_recognition_manager_impl.h"
#include "content/browser/speech/tts_controller_impl.h"
#include "content/browser/startup_data_impl.h"
#include "content/browser/startup_task_runner.h"
#include "content/browser/tracing/background_tracing_manager_impl.h"
Expand Down Expand Up @@ -1063,10 +1064,14 @@ void BrowserMainLoop::ShutdownThreadsAndCleanUp() {
midi_service_->Shutdown();
}

TRACE_EVENT0("shutdown",
"BrowserMainLoop::Subsystem:SpeechRecognitionManager");
io_thread_->task_runner()->DeleteSoon(FROM_HERE,
speech_recognition_manager_.release());
{
TRACE_EVENT0("shutdown",
"BrowserMainLoop::Subsystem:SpeechRecognitionManager");
io_thread_->task_runner()->DeleteSoon(
FROM_HERE, speech_recognition_manager_.release());
}

TtsControllerImpl::GetInstance()->Shutdown();

memory_pressure_monitor_.reset();

Expand Down
5 changes: 5 additions & 0 deletions content/browser/speech/tts_controller_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,11 @@ TtsEngineDelegate* TtsControllerImpl::GetTtsEngineDelegate() {
return engine_delegate_;
}

void TtsControllerImpl::Shutdown() {
if (tts_platform_)
tts_platform_->Shutdown();
}

void TtsControllerImpl::OnBrowserContextDestroyed(
BrowserContext* browser_context) {
bool did_clear_utterances = false;
Expand Down
2 changes: 2 additions & 0 deletions content/browser/speech/tts_controller_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ class CONTENT_EXPORT TtsControllerImpl : public TtsController,
void SetTtsEngineDelegate(TtsEngineDelegate* delegate) override;
TtsEngineDelegate* GetTtsEngineDelegate() override;

void Shutdown();

// Called directly by ~BrowserContext, because a raw BrowserContext pointer
// is stored in an Utterance.
void OnBrowserContextDestroyed(BrowserContext* browser_context);
Expand Down
1 change: 1 addition & 0 deletions content/browser/speech/tts_controller_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ class MockTtsPlatformImpl : public TtsPlatform {
void SetError(const std::string& error) override { error_ = error; }
std::string GetError() override { return error_; }
void ClearError() override { error_.clear(); }
void Shutdown() override {}

// Returns the amount of calls to Mock API.
int pause_called() const { return pause_called_; }
Expand Down
2 changes: 2 additions & 0 deletions content/browser/speech/tts_platform_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,6 @@ void TtsPlatformImpl::SetError(const std::string& error) {
error_ = error;
}

void TtsPlatformImpl::Shutdown() {}

} // namespace content
1 change: 1 addition & 0 deletions content/browser/speech/tts_platform_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class TtsPlatformImpl : public TtsPlatform {
std::string GetError() override;
void ClearError() override;
void SetError(const std::string& error) override;
void Shutdown() override;

protected:
TtsPlatformImpl() {}
Expand Down
14 changes: 14 additions & 0 deletions content/browser/speech/tts_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class TtsPlatformImplWin : public TtsPlatformImpl {

void GetVoices(std::vector<VoiceData>* out_voices) override;

void Shutdown() override;

// Get the single instance of this class.
static TtsPlatformImplWin* GetInstance();

Expand Down Expand Up @@ -265,6 +267,9 @@ void TtsPlatformImplWin::GetVoices(std::vector<VoiceData>* out_voices) {
}

void TtsPlatformImplWin::OnSpeechEvent() {
if (!speech_synthesizer_.Get())
return;

TtsController* controller = TtsController::GetInstance();
SPEVENT event;
while (S_OK == speech_synthesizer_->GetEvents(1, &event, NULL)) {
Expand Down Expand Up @@ -306,6 +311,9 @@ void TtsPlatformImplWin::SetVoiceFromName(const std::string& name) {
if (name.empty() || name == last_voice_name_)
return;

if (!speech_synthesizer_.Get())
return;

last_voice_name_ = name;

Microsoft::WRL::ComPtr<IEnumSpObjectTokens> voice_tokens;
Expand All @@ -330,6 +338,12 @@ void TtsPlatformImplWin::SetVoiceFromName(const std::string& name) {
}
}

void TtsPlatformImplWin::Shutdown() {
// This is required to ensures the object is released before the COM is
// uninitialized. Otherwise, this is causing shutdown hangs.
speech_synthesizer_ = nullptr;
}

TtsPlatformImplWin::TtsPlatformImplWin() {
::CoCreateInstance(CLSID_SpVoice, nullptr, CLSCTX_ALL,
IID_PPV_ARGS(&speech_synthesizer_));
Expand Down
4 changes: 4 additions & 0 deletions content/public/browser/tts_platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ class CONTENT_EXPORT TtsPlatform {
virtual std::string GetError() = 0;
virtual void ClearError() = 0;
virtual void SetError(const std::string& error) = 0;

// If supported, the platform shutdown its internal state. After that call,
// other methods may no-op.
virtual void Shutdown() = 0;
};

} // namespace content
Expand Down
2 changes: 2 additions & 0 deletions content/web_test/browser/web_test_tts_platform.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ void WebTestTtsPlatform::ClearError() {}

void WebTestTtsPlatform::SetError(const std::string& error) {}

void WebTestTtsPlatform::Shutdown() {}

WebTestTtsPlatform::WebTestTtsPlatform() {}

WebTestTtsPlatform::~WebTestTtsPlatform() {}
1 change: 1 addition & 0 deletions content/web_test/browser/web_test_tts_platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class WebTestTtsPlatform : public content::TtsPlatform {
std::string GetError() override;
void ClearError() override;
void SetError(const std::string& error) override;
void Shutdown() override;

private:
WebTestTtsPlatform();
Expand Down

0 comments on commit 277604b

Please sign in to comment.