Fix heap use-after-free in PrintingMessageFilter

chrome/browser/printing/printing_message_filter.cc

Index: chrome/browser/printing/printing_message_filter.cc
diff --git a/chrome/browser/printing/printing_message_filter.cc b/chrome/browser/printing/printing_message_filter.cc
index a4f2597779073a05f469dee5c38d2eb5dc0199ec..0bb82c0f3c2053a9d8f396d3eecb2d9f41a25a03 100644
--- a/chrome/browser/printing/printing_message_filter.cc
+++ b/chrome/browser/printing/printing_message_filter.cc
@@ -9,12 +9,14 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/memory/singleton.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/printing/print_job_manager.h"
 #include "chrome/browser/printing/printer_query.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
+#include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
 #include "components/printing/browser/print_manager_utils.h"
 #include "components/printing/common/print_messages.h"
 #include "content/public/browser/browser_thread.h"
@@ -39,6 +41,25 @@ namespace printing {
 
 namespace {
 
+class ShutdownNotifierFactory
+    : public BrowserContextKeyedServiceShutdownNotifierFactory {
+ public:
+  static ShutdownNotifierFactory* GetInstance() {
+    return base::Singleton<ShutdownNotifierFactory>::get();
+  }
+
+ private:
+  friend struct base::DefaultSingletonTraits<ShutdownNotifierFactory>;
+
+  ShutdownNotifierFactory()
+      : BrowserContextKeyedServiceShutdownNotifierFactory(
+          "PrintingMessageFilter") {}
+
+  ~ShutdownNotifierFactory() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(ShutdownNotifierFactory);
+};
+
 #if defined(OS_ANDROID)
 content::WebContents* GetWebContentsForRenderFrame(int render_process_id,
                                                    int render_frame_id) {
@@ -63,16 +84,26 @@ PrintViewManagerBasic* GetPrintManager(int render_process_id,
 PrintingMessageFilter::PrintingMessageFilter(int render_process_id,
                                              Profile* profile)
     : BrowserMessageFilter(PrintMsgStart),
-      is_printing_enabled_(new BooleanPrefMember),
       render_process_id_(render_process_id),
       queue_(g_browser_process->print_job_manager()->queue()) {
   DCHECK(queue_.get());
-  is_printing_enabled_->Init(prefs::kPrintingEnabled, profile->GetPrefs());
-  is_printing_enabled_->MoveToThread(
+  printing_shutdown_notifier_ =
+      ShutdownNotifierFactory::GetInstance()->Get(profile)->Subscribe(
+          base::Bind(&PrintingMessageFilter::ShutdownOnUIThread,
+                     base::Unretained(this)));
+  is_printing_enabled_.Init(prefs::kPrintingEnabled, profile->GetPrefs());
+  is_printing_enabled_.MoveToThread(

[Vitaly Buka (NO REVIEWS), 2016/12/02 18:51:00]

Thy it's moved to IO thread here, but destroyed on UI later?

[rbpotter, 2016/12/02 19:28:34]

It's moved to the IO thread so that GetValue can operate from the IO thread, but
it has to be destroyed on the UI thread regardless of whether it has been moved,
per the function description (line 187 of
https://cs.chromium.org/chromium/src/components/prefs/pref_member.h):
"Unsubscribes the PrefMember from the PrefService...This method should only be
called on the UI thread."

       BrowserThread::GetTaskRunnerForThread(BrowserThread::IO));
 }
 
 PrintingMessageFilter::~PrintingMessageFilter() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+}
+
+void PrintingMessageFilter::ShutdownOnUIThread() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  is_printing_enabled_.Destroy();

[Vitaly Buka (NO REVIEWS), 2016/12/02 18:51:00]

what is guarantee that OnMessageReceived is not going to be called after this?

[rbpotter, 2016/12/02 19:28:34]

If it does get called I think it will be okay as the Destroy() function
description states (line 187 of
https://cs.chromium.org/chromium/src/components/prefs/pref_member.h): "...the
PrefMember may not be used any more on the UI thread. Assuming |MoveToThread|
was previously called, |GetValue|, |IsManaged|, and |IsUserModifiable| can still
be called from the other thread but the results will no longer update from the
PrefService."
Since GetValue is called from the IO thread, it won't update any more after the
profile is destroyed, but it will not cause a crash. The profile gets destroyed
when the browser closes so the message filter should destruct shortly afterward
anyway, so I don't expect that the value not updating would cause a problem.
What do you think? I can add some changes to exit from OnMessageReceived early
if this has already been called, just not sure if it is necessary or not.

+  printing_shutdown_notifier_.reset();
 }
 
 void PrintingMessageFilter::OverrideThreadForMessage(
@@ -85,6 +116,10 @@ void PrintingMessageFilter::OverrideThreadForMessage(
 #endif
 }
 
+void PrintingMessageFilter::OnDestruct() const {
+  BrowserThread::DeleteOnUIThread::Destruct(this);
+}
+
 bool PrintingMessageFilter::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(PrintingMessageFilter, message)
@@ -137,7 +172,7 @@ void PrintingMessageFilter::OnTempFileForPrintingWritten(int render_frame_id,
 void PrintingMessageFilter::OnGetDefaultPrintSettings(IPC::Message* reply_msg) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   scoped_refptr<PrinterQuery> printer_query;
-  if (!is_printing_enabled_->GetValue()) {
+  if (!is_printing_enabled_.GetValue()) {
     // Reply with NULL query.
     OnGetDefaultPrintSettingsReply(printer_query, reply_msg);
     return;
@@ -250,7 +285,7 @@ void PrintingMessageFilter::OnUpdatePrintSettings(
   std::unique_ptr<base::DictionaryValue> new_settings(job_settings.DeepCopy());
 
   scoped_refptr<PrinterQuery> printer_query;
-  if (!is_printing_enabled_->GetValue()) {
+  if (!is_printing_enabled_.GetValue()) {
     // Reply with NULL query.
     OnUpdatePrintSettingsReply(printer_query, reply_msg);
     return;