Maintain HostZoom connection per-frame on browser side

content/browser/host_zoom_map_observer.cc

Index: content/browser/host_zoom_map_observer.cc
diff --git a/content/browser/host_zoom_map_observer.cc b/content/browser/host_zoom_map_observer.cc
index 88d42dfb6b825f27965b4206b8cf3f5e40ddb139..2c2d2af66ad3c0fe38bd5f9490569c2874e1cc4a 100644
--- a/content/browser/host_zoom_map_observer.cc
+++ b/content/browser/host_zoom_map_observer.cc
@@ -23,23 +23,40 @@ void HostZoomMapObserver::ReadyToCommitNavigation(
   if (!navigation_handle->IsInMainFrame())
     return;
 
-  DCHECK(host_zoom_.is_bound());
-  if (!host_zoom_.is_bound())
-    return;
-
   RenderFrameHost* render_frame_host =
       navigation_handle->GetRenderFrameHost();
+  const auto& entry = host_zoom_ptrs_.find(render_frame_host);
+  if (entry == host_zoom_ptrs_.end())
+    return;
+
+  const mojom::HostZoomAssociatedPtr& host_zoom = entry->second;
+  DCHECK(host_zoom.is_bound());
+  DCHECK(!host_zoom.encountered_error());
+
   RenderProcessHost* render_process_host = render_frame_host->GetProcess();
   HostZoomMapImpl* host_zoom_map = static_cast<HostZoomMapImpl*>(
       render_process_host->GetStoragePartition()->GetHostZoomMap());
   double zoom_level = host_zoom_map->GetZoomLevelForView(
       navigation_handle->GetURL(), render_process_host->GetID(),
       render_frame_host->GetRenderViewHost()->GetRoutingID());
-  host_zoom_->SetHostZoomLevel(navigation_handle->GetURL(), zoom_level);
+  host_zoom->SetHostZoomLevel(navigation_handle->GetURL(), zoom_level);
 }
 
 void HostZoomMapObserver::RenderFrameCreated(RenderFrameHost* rfh) {
-  rfh->GetRemoteAssociatedInterfaces()->GetInterface(&host_zoom_);
+  mojom::HostZoomAssociatedPtr host_zoom;
+  rfh->GetRemoteAssociatedInterfaces()->GetInterface(&host_zoom);
+  host_zoom.set_connection_error_handler(
+      base::Bind(&HostZoomMapObserver::OnConnectionError,
+                 base::Unretained(this), base::Unretained(rfh)));
+  host_zoom_ptrs_[rfh] = std::move(host_zoom);
+}
+
+void HostZoomMapObserver::OnConnectionError(RenderFrameHost* rfh) {
+  // NOTE: It is not safe to do anything with |rfh| other than use it as a
+  // key here.

[wjmaclean, 2016/12/16 14:45:35]

Presumably this is on account of a rfh being deleted ... is there any
possibility of a new rhf with the same pointer being given the wrong
host_zoom_ptr in the meantime, on account of a 'stale' entry?

[ncarter (slow), 2016/12/16 18:21:12]

The scenario wjmaclean describes seems plausible.

In general WCO::RenderFrameCreated overrides ought to be balanced by a
WCO::RenderFrameDeleted override. rfh pointer references ought not be kept past
RenderFrameDeleted.

[blundell, 2016/12/19 17:03:14]

Changed. For my own curiosity: the case where this would occur would be if a
WCO::ReadyToCommitNavigation() came in for a given RenderFrame before
WCO::RenderFrameCreated() came in for that RenderFrame. Is that possible?
Understood that in any case this is the right change to make.

[ncarter (slow), 2016/12/19 19:21:16]

I would hope we don't do ReadyToCommitNavigation on non-live RFH's. That's not
exactly what I was worried about. It's more a general skepticism of patterns
like "It is not safe to do anything with |pointer| other than use it as a key"
because of the possibility of the memory being reallocated to a new object of
the same type as the deleted object.

The worry is more that RenderFrameDeleted (and RFH deletion /
~RenderFrameHostImpl) can occur basically at any point in time. One particularly
abrupt example is: if the RFH is an oopif, and the process of the main frame
crashes, we would delete the oopif RFH immediately since the document containing
them has gone away.  This event is triggered by something completely outside the
RFH's operation, so it could really happen at any moment.

In this case, if HostZoomMapObserver does not handle RenderFrameDeleted, I worry
about whether OnConnectionError would run after the memory for the RFH has been
freed, or else RenderFrameCreated happening on a new RFH that happened to be
reallocated to the same pointer address, overwriting old the map entry.

It's possible that there's some mojo behavior I don't know about, that would
cause OnConnectionError to always (and only ever) happen before (or during)
~RenderFrameHostImpl. If that's the case then I'd be interested to learn about
it.

+  const auto& entry = host_zoom_ptrs_.find(rfh);
+  DCHECK(entry != host_zoom_ptrs_.end());
+  host_zoom_ptrs_.erase(entry);
 }
 
 }  // namespace content