Remove OwnedInterface

chrome/browser/search_engines/search_provider_install_state_impl.cc

 // Copyright (c) 2012 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 "chrome/browser/search_engines/search_provider_install_state_impl.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "chrome/browser/google/google_url_tracker_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/search_engines/ui_thread_search_terms_data.h"
 #include "chrome/common/search_provider.mojom.h"
-#include "content/public/browser/owned_interface.h"
 #include "content/public/browser/render_process_host.h"
 #include "services/shell/public/cpp/interface_registry.h"
 #include "url/gurl.h"
 
 using content::BrowserThread;
 
 SearchProviderInstallStateImpl::SearchProviderInstallStateImpl(
     int render_process_id,
     Profile* profile)
     : provider_data_(TemplateURLServiceFactory::GetForProfile(profile),

[Sam McNally, 2016/07/26 01:46:34]

TemplateURLService is a KeyedService stored as a raw pointer in
|provider_data_|, which uses it without any lifetime checks. It's accessed when
handling an IPC so it could be a problem, depending on what happens during
shutdown.

[tibell, 2016/07/26 02:40:48]

dcheng@ is going to try to delete this code. I will simply wait until that's
done and just keep the OwnerInterface removal in this CL. That seems like less
work than actually trying to figure out if this is safe. :(

[jam, 2016/07/26 05:25:36]

This is safe, check out the implementation of provider_data_. It posts a task to
the UI thread to call it.

However when profiles are destructed (i.e. when keyed services are destructed),
the main message loop has stopped running so this task posted from the IO loop
wouldn't run.

                      UIThreadSearchTermsData(profile).GoogleBaseURLValue(),
                      GoogleURLTrackerFactory::GetForProfile(profile),
                      content::RenderProcessHost::FromID(render_process_id)),
       is_off_the_record_(profile->IsOffTheRecord()),
+      binding_(this),
       weak_factory_(this) {
   // This is initialized by RenderProcessHostImpl. Do not add any non-trivial
   // initialization here. Instead do it lazily when required.
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 }
 
 SearchProviderInstallStateImpl::~SearchProviderInstallStateImpl() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 }
 
-void SearchProviderInstallStateImpl::Bind(
+// static
+void SearchProviderInstallStateImpl::Create(
+    int render_process_id,
+    Profile* profile,
     chrome::mojom::SearchProviderInstallStateRequest request) {
-  binding_set_.AddBinding(this, std::move(request));
+  // BindOnIOThread takes ownership on the impl object:
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&SearchProviderInstallStateImpl::BindOnIOThread,
+                 base::Unretained(new SearchProviderInstallStateImpl(

[leonhsl(Using Gerrit), 2016/07/26 02:28:34]

Why don't we do new/bind together on IO thread? If we created the impl instance
here, then before it is bound on IO thread later, no one owns it.

[tibell, 2016/07/26 02:40:48]

The fields in the class need to be initialized on the UI thread (that's why
there's a DCHECK_CURRENTLY_ON in the constructor).

+                     render_process_id, profile)),
+                 base::Passed(&request)));
+}
+
+void SearchProviderInstallStateImpl::BindOnIOThread(
+    chrome::mojom::SearchProviderInstallStateRequest request) {
+  binding_.Bind(std::move(request));
 }
 
 chrome::mojom::InstallState
 SearchProviderInstallStateImpl::GetSearchProviderInstallState(
     const GURL& page_location,
     const GURL& requested_host) {
   GURL requested_origin = requested_host.GetOrigin();
 
   // Do the security check before any others to avoid information leaks.
   if (page_location.GetOrigin() != requested_origin)
@@ skipping 32 matching lines @@
 
 void SearchProviderInstallStateImpl::ReplyWithProviderInstallState(
     const GURL& page_location,
     const GURL& requested_host,
     const GetInstallStateCallback& callback) {
   chrome::mojom::InstallState install_state =
       GetSearchProviderInstallState(page_location, requested_host);
 
   callback.Run(install_state);
 }