PlzNavigate: Move the navigation logic to NavigatorImpl

PlzNavigate: Move the navigation logic to NavigatorImpl

This CL moves the navigation logic for PlzNavigate from RenderFrameHostImpl to
NavigatorImpl. The logic for selecting a RenderFrameHost for the naviagtion
remains in RenderFrameHostManager.

BUG=376006, 376091, 376014
Committed: https://crrev.com/71a42ec15abb539b215e075e645e940bf1084398
Cr-Commit-Position: refs/heads/master@{#297871}

[clamy, 2014-09-30 00:18:11]

clamy@chromium.org changed reviewers:
+ carlosk@chromium.org, nasko@chromium.org

[clamy, 2014-09-30 00:21:09]

@carlosk, nasko: PTAL. Based on our discussion today, a CL to move the
navigation logic from RFHM to NavigatorImpl. I have kept the logic for selecting
the renderer upon commit in RFHM (since that seems to be RFHM role). I am not
completely sure I get what you want to do with RFHM as a state machine, so I did
not make any changes towards that direction. I also did not update the unit
tests, and will do so once we get to an agreement about the general design of
the change.

[nasko, 2014-10-01 15:59:50]

Thanks for putting this together! I think there is only one main point to
discuss and the rest is mostly mechanics.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:811:
it->second->CancelNavigation();
Shouldn't we also erase the element from the map?

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.h:115: // different FrameTreeNode,
based on the frame_tree_node_id.
nit: nodes should be plural, based on the sentence structure.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.h:116: typedef base::hash_map<int64,
linked_ptr<NavigationRequest>>
Why linked_ptr? Why not scoped_ptr?

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:518:
RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
I just realized that this method acts as though --site-per-process is passed in,
aka it will navigate each frame to a new process if SiteInstance changes. I'm
not quite sure if this is the right thing to do yet, though definitely needed
long term.

Also, this should be the equivalent to UpdateStateForNavigate in the PlzNavigate
world, right? Is it possible to just use UpdateStateForNavigate?

[clamy, 2014-10-01 17:10:43]

Thanks! Please find my answers to your comments below. Additionally, I am not
quite sure what to do with the unit tests for PlzNavigate. Should I create a
navigator_impl_unittests file and put them there, or do you think it's ok if
they remain in render_frame_host_manager_unittests?

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:811:
it->second->CancelNavigation();
On 2014/10/01 15:59:49, nasko wrote:
> Shouldn't we also erase the element from the map?

We are replacing it just afterwards so it should be fine, I think.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.h:115: // different FrameTreeNode,
based on the frame_tree_node_id.
On 2014/10/01 15:59:49, nasko wrote:
> nit: nodes should be plural, based on the sentence structure.

Done.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.h:116: typedef base::hash_map<int64,
linked_ptr<NavigationRequest>>
On 2014/10/01 15:59:50, nasko wrote:
> Why linked_ptr? Why not scoped_ptr?

I was under the impression that you should not put a scoped_ptr inside a
hash_map due to the copies performed inside it.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:518:
RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
On 2014/10/01 15:59:50, nasko wrote:
> I just realized that this method acts as though --site-per-process is passed
in,
> aka it will navigate each frame to a new process if SiteInstance changes. I'm
> not quite sure if this is the right thing to do yet, though definitely needed
> long term.
> 
> Also, this should be the equivalent to UpdateStateForNavigate in the
PlzNavigate
> world, right? Is it possible to just use UpdateStateForNavigate?

Well it's pretty close, but UpdateStateForNavigate has a few extra things that
we don't want to use here. For example it handles transfers, or tells the old
renderer to execute its BeforeUnload event in cross process navigation. What I
could do is extract the parts of UpdateStateForNavigate that we are interested
in in helper functions, or have part of UpdateStateForNavigate not execute if we
have the browser-side navigations flag on, as well as remove its dependency on
the NavigationEntry.

As to the first concern, I think it will only be a problem once we start
handling renderer initiated navigations. As of now we only handle browser
initiated navigations, which come from WebContentsImpl::NavigateToPendingEntry.
AFAIU the code in WebContentsImpl makes sure that Navigator::NavigateToEntry is
only called on a top-level frame if --site-per-process is not passed in, right?

[nasko, 2014-10-01 18:20:19]

Response to comments.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:811:
it->second->CancelNavigation();
On 2014/10/01 17:10:43, clamy wrote:
> On 2014/10/01 15:59:49, nasko wrote:
> > Shouldn't we also erase the element from the map?
> 
> We are replacing it just afterwards so it should be fine, I think.

AFAIU, insert will not overwrite existing entries. It might be safer to delete
or check the return value from insert.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.h:116: typedef base::hash_map<int64,
linked_ptr<NavigationRequest>>
On 2014/10/01 17:10:43, clamy wrote:
> On 2014/10/01 15:59:50, nasko wrote:
> > Why linked_ptr? Why not scoped_ptr?
> 
> I was under the impression that you should not put a scoped_ptr inside a
> hash_map due to the copies performed inside it.

Why not use ScopedPtrHashMap then? Scoped pointers are simpler to understand and
if we can use those, it will be nicer. Up to you.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:518:
RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
On 2014/10/01 17:10:43, clamy wrote:
> On 2014/10/01 15:59:50, nasko wrote:
> > I just realized that this method acts as though --site-per-process is passed
> in,
> > aka it will navigate each frame to a new process if SiteInstance changes.
I'm
> > not quite sure if this is the right thing to do yet, though definitely
needed
> > long term.
> > 
> > Also, this should be the equivalent to UpdateStateForNavigate in the
> PlzNavigate
> > world, right? Is it possible to just use UpdateStateForNavigate?
> 
> Well it's pretty close, but UpdateStateForNavigate has a few extra things that
> we don't want to use here. For example it handles transfers, or tells the old
> renderer to execute its BeforeUnload event in cross process navigation. What I
> could do is extract the parts of UpdateStateForNavigate that we are interested
> in in helper functions, or have part of UpdateStateForNavigate not execute if
we
> have the browser-side navigations flag on, as well as remove its dependency on
> the NavigationEntry.

Either helper functions or flag checks will be fine. Whichever is more readable
and easier to follow. Looking over the code, the block that deals with
transfers/beforeunload is the only thing that we won't need if I'm not missing
something else.
 
> As to the first concern, I think it will only be a problem once we start
> handling renderer initiated navigations. As of now we only handle browser
> initiated navigations, which come from
WebContentsImpl::NavigateToPendingEntry.
> AFAIU the code in WebContentsImpl makes sure that Navigator::NavigateToEntry
is
> only called on a top-level frame if --site-per-process is not passed in,
right?

Correct. Maybe a todo somewhere that we depend on this and things will change
once we start handling renderer initiated navigations?

[carlosk, 2014-10-01 18:31:10]

Cool! 

Regarding your question about tests, it makes sense to move them to a new
unittest file as the navigation responsibilities are moving out of RHFM. But it
might be that the work to get this done be quite some given the amount of shared
helper code already in place. So I don't have a clear opinion on this.

Also, I'm going to preemptively LGTM this CL as we'll depart for the 2-day
offsite tomorrow and I don't want to have you blocked until Monday.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:695: NavigationRequest*
navigation_request = it->second.get();
In the case of a hijacked renderer process it could send bogus
FrameHostMsg_BeginNavigation (if there's no navigation going on) that would
cause a crash/undefined behavior here because the find wouldn't find anything.
Is this something we normally worry about?

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:815: frame_tree_node_id,
navigation_request));
I was also looking into this matter of the entry already existing in the map. It
seems that for some implementations of hash_map.insert -- and IDK precisely
which one we use even after finding it's "absorbed" from the
compiler-standard-one in hash_tables.h -- it *won't* replace an element if it
already exists.

My suggestion is, if the element does exist, to use the returned iterator to
replace it->second with the new value. I think that works (again depending on
our implementation of hash_map)...

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.h:118: NavigationRequestMap
navigation_request_map_;
More of a design discussion: this is in fact replicating the FrameTreeNode tree
to keep navigation state for each node into a single NavigatorImpl instance.

I imagine NavigatorImpl instances are currently 1-to-1 with FrameTree because of
historical reasons, from before RenderFrameHost existed. But now that that's not
the case anymore and navigations exist/have state on a per-node basis are we
considering making them 1-to-1 with FrameTreeNode instances instead? 

I am also guessing that "navigation history" exists in the context of the
tab/web-contents and not for each frame,  am I right? If that's correct I'd
think NavigationControllerImpl would still be kept that way but would
"translate" history based navigations into calls to the proper NavigatorImpl
instances.

Is that something that is being considered?

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1029: frame_tree_node(),
params, common_params);
Noob question: if we really want to move all of navigation responsibilities out
of RFH, does it makes sense to make Navigator and IPC receiver (and maybe also
sender)? Does this implies we'll need a Navigator-mirror class running at the
renderer side?

[clamy, 2014-10-01 22:28:42]

Patchset #2 (id:20001) has been deleted

[clamy, 2014-10-01 22:30:47]

Thanks! Here is the version with the unit tests.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:695: NavigationRequest*
navigation_request = it->second.get();
On 2014/10/01 18:31:09, carlosk wrote:
> In the case of a hijacked renderer process it could send bogus
> FrameHostMsg_BeginNavigation (if there's no navigation going on) that would
> cause a crash/undefined behavior here because the find wouldn't find anything.
> Is this something we normally worry about?

Yes, but this is actually covered by the TODO above. When we receive a
FrameHostMsg_BeginNavigation we should create a NavigationRequest if we don't
have one. I intend to change that in a follow up CL.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:811:
it->second->CancelNavigation();
On 2014/10/01 18:20:18, nasko wrote:
> On 2014/10/01 17:10:43, clamy wrote:
> > On 2014/10/01 15:59:49, nasko wrote:
> > > Shouldn't we also erase the element from the map?
> > 
> > We are replacing it just afterwards so it should be fine, I think.
> 
> AFAIU, insert will not overwrite existing entries. It might be safer to delete
> or check the return value from insert.

Indeed it does not. But I am now using ScopedPtrHashMap::set which does.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.h:116: typedef base::hash_map<int64,
linked_ptr<NavigationRequest>>
On 2014/10/01 18:20:19, nasko wrote:
> On 2014/10/01 17:10:43, clamy wrote:
> > On 2014/10/01 15:59:50, nasko wrote:
> > > Why linked_ptr? Why not scoped_ptr?
> > 
> > I was under the impression that you should not put a scoped_ptr inside a
> > hash_map due to the copies performed inside it.
> 
> Why not use ScopedPtrHashMap then? Scoped pointers are simpler to understand
and
> if we can use those, it will be nicer. Up to you.

Because I did not know we had a ScopedPtrHashMap :). I am now using it.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.h:118: NavigationRequestMap
navigation_request_map_;
On 2014/10/01 18:31:10, carlosk wrote:
> More of a design discussion: this is in fact replicating the FrameTreeNode
tree
> to keep navigation state for each node into a single NavigatorImpl instance.
> 
> I imagine NavigatorImpl instances are currently 1-to-1 with FrameTree because
of
> historical reasons, from before RenderFrameHost existed. But now that that's
not
> the case anymore and navigations exist/have state on a per-node basis are we
> considering making them 1-to-1 with FrameTreeNode instances instead? 
> 
> I am also guessing that "navigation history" exists in the context of the
> tab/web-contents and not for each frame,  am I right? If that's correct I'd
> think NavigationControllerImpl would still be kept that way but would
> "translate" history based navigations into calls to the proper NavigatorImpl
> instances.
> 
> Is that something that is being considered? 

I don't think there is such a plan. And we are not exactly replicating the frame
tree, since we just keep a hashmap of current navigation requests.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1029: frame_tree_node(),
params, common_params);
On 2014/10/01 18:31:10, carlosk wrote:
> Noob question: if we really want to move all of navigation responsibilities
out
> of RFH, does it makes sense to make Navigator and IPC receiver (and maybe also
> sender)? Does this implies we'll need a Navigator-mirror class running at the
> renderer side?

I think it is better to keep the IPC handling in RenderFrameHost and
RenderFrame. After all, they are frame messages.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:518:
RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
On 2014/10/01 18:20:19, nasko wrote:
> On 2014/10/01 17:10:43, clamy wrote:
> > On 2014/10/01 15:59:50, nasko wrote:
> > > I just realized that this method acts as though --site-per-process is
passed
> > in,
> > > aka it will navigate each frame to a new process if SiteInstance changes.
> I'm
> > > not quite sure if this is the right thing to do yet, though definitely
> needed
> > > long term.
> > > 
> > > Also, this should be the equivalent to UpdateStateForNavigate in the
> > PlzNavigate
> > > world, right? Is it possible to just use UpdateStateForNavigate?
> > 
> > Well it's pretty close, but UpdateStateForNavigate has a few extra things
that
> > we don't want to use here. For example it handles transfers, or tells the
old
> > renderer to execute its BeforeUnload event in cross process navigation. What
I
> > could do is extract the parts of UpdateStateForNavigate that we are
interested
> > in in helper functions, or have part of UpdateStateForNavigate not execute
if
> we
> > have the browser-side navigations flag on, as well as remove its dependency
on
> > the NavigationEntry.
> 
> Either helper functions or flag checks will be fine. Whichever is more
readable
> and easier to follow. Looking over the code, the block that deals with
> transfers/beforeunload is the only thing that we won't need if I'm not missing
> something else.
>  
> > As to the first concern, I think it will only be a problem once we start
> > handling renderer initiated navigations. As of now we only handle browser
> > initiated navigations, which come from
> WebContentsImpl::NavigateToPendingEntry.
> > AFAIU the code in WebContentsImpl makes sure that Navigator::NavigateToEntry
> is
> > only called on a top-level frame if --site-per-process is not passed in,
> right?
> 
> Correct. Maybe a todo somewhere that we depend on this and things will change
> once we start handling renderer initiated navigations?

Done.

https://codereview.chromium.org/615633005/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/615633005/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl_unittest.cc:215:
EXPECT_FALSE(node->render_manager()->pending_frame_host());
This was changed from the original unit test since the patch does not swap in
the new renderer in RFHM::CommitNavigation (but just creates it and initializes
it).

[Charlie Reis, 2014-10-01 23:28:50]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2014-10-01 23:28:50]

Just a high level drive-by review, but I'm excited to see this stuff move to
Navigator (and get its own unit test)!  I'll defer to Nasko for the details, but
otherwise LGTM with one nit below.

https://codereview.chromium.org/615633005/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/615633005/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1424: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
This is a bit awkward to read.  Maybe move the "We now have a pending RFH" block
from 1449 to 1415, and then return early if we're in PlzNavigate?

[nasko, 2014-10-02 00:09:39]

Once the comment by Charlie to move code in UpdateStateForNavigate to make it
more readable is complete, LGTM!

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:811:
it->second->CancelNavigation();
On 2014/10/01 22:30:46, clamy wrote:
> On 2014/10/01 18:20:18, nasko wrote:
> > On 2014/10/01 17:10:43, clamy wrote:
> > > On 2014/10/01 15:59:49, nasko wrote:
> > > > Shouldn't we also erase the element from the map?
> > > 
> > > We are replacing it just afterwards so it should be fine, I think.
> > 
> > AFAIU, insert will not overwrite existing entries. It might be safer to
delete
> > or check the return value from insert.
> 
> Indeed it does not. But I am now using ScopedPtrHashMap::set which does.

Acknowledged.

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/615633005/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.h:118: NavigationRequestMap
navigation_request_map_;
On 2014/10/01 22:30:46, clamy wrote:
> On 2014/10/01 18:31:10, carlosk wrote:
> > More of a design discussion: this is in fact replicating the FrameTreeNode
> tree
> > to keep navigation state for each node into a single NavigatorImpl instance.
> > 
> > I imagine NavigatorImpl instances are currently 1-to-1 with FrameTree
because
> of
> > historical reasons, from before RenderFrameHost existed. But now that that's
> not
> > the case anymore and navigations exist/have state on a per-node basis are we
> > considering making them 1-to-1 with FrameTreeNode instances instead? 
> > 
> > I am also guessing that "navigation history" exists in the context of the
> > tab/web-contents and not for each frame,  am I right? If that's correct I'd
> > think NavigationControllerImpl would still be kept that way but would
> > "translate" history based navigations into calls to the proper NavigatorImpl
> > instances.
> > 
> > Is that something that is being considered? 
> 
> I don't think there is such a plan. And we are not exactly replicating the
frame
> tree, since we just keep a hashmap of current navigation requests.

There is no plan to have Navigator be 1-1 with FrameTreeNode. Navigator
represents a context for top-level navigations, so there should only be one for
web pages. The reason it was decoupled from WebContents is that for <webview>,
it represents a top-level navigation context as well, so we can have two
top-level navigations contexts in the same frame tree.

Session history will be refactored as part of Site Isolation to move to browser
process and there will be linear session history entry per frame as well as tree
like session history for the WebContents, which is what ultimately shows up as
the history users see. All the glory details are in the session history design
doc:
https://docs.google.com/a/chromium.org/document/d/1Z-24Xmb5A00eMSfaBB5MQLTrp9...

[clamy, 2014-10-02 17:39:06]

clamy@chromium.org changed reviewers:
+ davidben@chromium.org

[clamy, 2014-10-02 17:39:54]

Thanks!

+davidben: FYI - you will most likely need to rebase the IO thread patch after
this patch lands.

https://codereview.chromium.org/615633005/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/615633005/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1424: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/10/01 23:28:50, Charlie Reis wrote:
> This is a bit awkward to read.  Maybe move the "We now have a pending RFH"
block
> from 1449 to 1415, and then return early if we're in PlzNavigate?

Done.

[clamy, 2014-10-02 17:40:02]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-10-02 17:40:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615633005/60001

[commit-bot: I haz the power, 2014-10-02 18:43:38]

Committed patchset #3 (id:60001) as e38afa381117773eafd9bd5e4d46233a6491e16e

[commit-bot: I haz the power, 2014-10-02 18:44:37]

Patchset 3 (id:??) landed as
https://crrev.com/71a42ec15abb539b215e075e645e940bf1084398
Cr-Commit-Position: refs/heads/master@{#297871}