Have GeolocationServiceImpl refer to RFHI via a weak pointer

content/browser/frame_host/render_frame_host_impl.cc

 // Copyright 2013 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 "content/browser/frame_host/render_frame_host_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/containers/hash_tables.h"
 #include "base/lazy_instance.h"
@@ skipping 1678 matching lines @@
 }
 #endif
 
 void RenderFrameHostImpl::RegisterMojoServices() {
   GeolocationServiceContext* geolocation_service_context =
       delegate_ ? delegate_->GetGeolocationServiceContext() : NULL;
   if (geolocation_service_context) {
     // TODO(creis): Bind process ID here so that GeolocationServiceImpl
     // can perform permissions checks once site isolation is complete.
     // crbug.com/426384
+    // NOTE: At shutdown, there is no guaranteed ordering between destruction of
+    // this object and destruction of any GeolocationServicesImpls created via
+    // the below service registry, the reason being that the destruction of the
+    // latter is triggered by receiving a message that the pipe was closed from
+    // the renderer side. Hence, supply the reference to this object as a weak
+    // pointer.

[Michael van Ouwerkerk, 2016/02/18 09:37:57]

Nit: maybe also refer to crbug.com/556749 in this comment.

[blundell, 2016/02/26 12:34:12]

I think the comment is sufficient to explain the issue as-is, but happy to
follow up if you feel strongly.

     GetServiceRegistry()->AddService<GeolocationService>(
         base::Bind(&GeolocationServiceContext::CreateService,
                    base::Unretained(geolocation_service_context),
                    base::Bind(&RenderFrameHostImpl::DidUseGeolocationPermission,
-                              base::Unretained(this))));
+                              weak_ptr_factory_.GetWeakPtr())));

[ncarter (slow), 2016/02/19 00:44:36]

The RenderFrameHostImpl owns the ServiceRegistry, so I am a bit surprised to
learn that that this callback, which we hand to the ServiceRegistry, can be
invoked after we've destroyed the ServiceRegistry.

So what's actually going on here? Are we sure this isn't a bug in the
ServiceRegistry?

[ncarter (slow), 2016/02/19 23:09:44]

I looked at this a little more, and am inferring that it's not the
ServiceRegistry that is outliving the RFH, but the inner callback here, which is
retained by the GeolocationServiceImpl, which is what actually winds up
outliving the RFH.

In that case the WeakPtr here seems fine -- so lgtm in the sense that this fixes
a UaF.

I do wish there was a way to actively kill off the services that so that they
couldn't outlive their associated RenderFrameHost or WebContents, and breed this
kind of UaF possibility. Without a live RFH we really ought not to be allowing
any kind of IPCs in (see also: https://codereview.chromium.org/1534933002/ and
its associated bugs)

[blundell, 2016/02/26 12:32:50]

Correct.
https://codereview.chromium.org/1411063007 is related, although I don't think it
would be a complete silver bullet.

   }
 
   WakeLockServiceContext* wake_lock_service_context =
       delegate_ ? delegate_->GetWakeLockServiceContext() : nullptr;
   if (wake_lock_service_context) {
     // WakeLockServiceContext is owned by WebContentsImpl so it will outlive
     // this RenderFrameHostImpl, hence a raw pointer can be bound to service
     // factory callback.
     GetServiceRegistry()->AddService<WakeLockService>(
         base::Bind(&WakeLockServiceContext::CreateService,
                    base::Unretained(wake_lock_service_context),
                    GetProcess()->GetID(), GetRoutingID()));
   }
 
   if (!permission_service_context_)
     permission_service_context_.reset(new PermissionServiceContext(this));
 
   GetServiceRegistry()->AddService<PermissionService>(
       base::Bind(&PermissionServiceContext::CreateService,
                  base::Unretained(permission_service_context_.get())));
 
   GetServiceRegistry()->AddService<presentation::PresentationService>(
       base::Bind(&PresentationServiceImpl::CreateMojoService,
                  base::Unretained(this)));

[Charlie Reis, 2016/02/18 22:51:16]

Won't this be a problem for all of these Mojo services as well?

Also, what processing do all these services do after the RFH is gone and before
their pipes are closed, even if they have a weak pointer and don't call back to
the RFH?

I'm wondering if they should be listening for the RFH's deletion to clean
themselves up instead (e.g., via something like
WebContentsObserver::RenderFrameDeleted, though you have to be careful with that
one).

[Charlie Reis, 2016/02/25 21:00:57]

Ok, Nick explained that these other services shouldn't have the same problem. 
Works for me, though it might be nice (as he mentions) to have a way to avoid
this type of bug in the future.

[blundell, 2016/02/26 12:32:50]

Good catch on pointing out the other cases here that are potentially
problematic. I also double-checked that all of the other services that take in a
reference to the RFHI handle lifetime properly (in various different custom
ways, as Nick pointed out).

 
   if (!frame_mojo_shell_)
     frame_mojo_shell_.reset(new FrameMojoShell(this));
 
   GetServiceRegistry()->AddService<mojo::Shell>(base::Bind(
       &FrameMojoShell::BindRequest, base::Unretained(frame_mojo_shell_.get())));
 
 #if defined(ENABLE_WEBVR)
   const base::CommandLine& browser_command_line =
       *base::CommandLine::ForCurrentProcess();
@@ skipping 640 matching lines @@
   *dst = src;
 
   if (src.routing_id != -1)
     dst->tree_id = RoutingIDToAXTreeID(src.routing_id);
 
   if (src.parent_routing_id != -1)
     dst->parent_tree_id = RoutingIDToAXTreeID(src.parent_routing_id);
 }
 
 }  // namespace content