Store the last committed origin in RenderFrameHostImpl.

content/browser/frame_host/frame_tree_node.h

Index: content/browser/frame_host/frame_tree_node.h
diff --git a/content/browser/frame_host/frame_tree_node.h b/content/browser/frame_host/frame_tree_node.h
index 6ee699e87346b7059c202012dbc7136aaed37b77..701787120d6ce6863e0ea4672ceeeb46e037e361 100644
--- a/content/browser/frame_host/frame_tree_node.h
+++ b/content/browser/frame_host/frame_tree_node.h
@@ -138,6 +138,10 @@ class CONTENT_EXPORT FrameTreeNode {
   }
 
   // Returns the origin of the last committed page in this frame.
+  // WARNING: To get the last committed origin for a particular
+  // RenderFrameHost, use RenderFrameHost::GetLastCommittedOrigin() instead,
+  // which will behave correctly even when the RenderFrameHost is not the
+  // current one for this frame (such as when it's pending deletion).

[alexmos, 2016/12/06 17:22:40]

I'm not thrilled about storing the origin in two places (on RFH and in
replication state), but I don't know if there's a good way around it, given that
we have to send the origin as part of the replication state anyway, and the
quirk in the comment below.  I didn't really like some other options (don't keep
the current origin in replication state and fill it in from
current_frame_host()->GetLastCommittedOrigin() whenever we need to send the
replication state; separate origin out of the replication state). I'm hoping
this big warning clarifies when to use each, but let me know if you have
thoughts about it.

[Charlie Reis, 2016/12/08 19:51:47]

Yeah, I see the dilemma.  I'm ok with this at least for now, since we're adding
a way to avoid the original problem.  People tend not to notice these comments,
but if it proves to be a problem we can deprecate this later.

   const url::Origin& current_origin() const {
     return replication_state_.origin;

[alexmos, 2016/12/06 17:22:40]

I tried putting a DCHECK here to see if any callers use this when this value
doesn't match current_frame_host()->GetLastCommittedOrigin().  Interestingly,
it's possible that for a brief time this is actually not true.  This is due to
the early CommitPending call in UpdateStateForNavigate if we're on the sad tab
-- we make the pending RFH the current one without waiting for it to commit, so
if we check current_origin() before the actual commit, the replication state
will have the old origin (from crashed page), and the current_frame_host() will
return the origin of the RFH about to be committed, i.e., null.  Sure enough,
that happens when we check current_origin() in
NavigationControllerImpl::IsURLInPageNavigation, as part of DidNavigate. 
Reading IsURLInPageNavigation, I *think* its use of current_origin() (and not
GetLastCommittedOrigin) is intentional, but it'd be nice if you can verify (
https://codereview.chromium.org/1457053004 introduced it).  It also seems the
value returned by current_origin() in this specific time window/situation is
actually correct (returning the old committed origin rather than the new
not-yet-committed-but-current RFH's null origin).

[Charlie Reis, 2016/12/08 19:51:47]

I agree that IsURLInPageNavigation needs to get the old origin.  Interesting
that the optimization for sad tab interferes; I guess that's ok.

   }