Store the last committed origin in RenderFrameHostImpl.

Store the last committed origin in RenderFrameHostImpl.

The allows using RenderFrameHost::GetLastCommittedOrigin() correctly
when the RenderFrameHost is different from a frame's current RFH, such
as when it's pending or pending deletion.

BUG=590035, 663740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e47445d4e35de16d2e68ecc34b7d2289a60f1c64
Cr-Commit-Position: refs/heads/master@{#437660}

[alexmos, 2016-12-03 01:57:04]

Description was changed from

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RFH::GetLastCommittedOrigin() correctly when the
RFH isn't a current RFH for a frame, such as when it's pending
or pending delete.

BUG=590035
==========

to

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RFH::GetLastCommittedOrigin() correctly when the
RFH isn't a current RFH for a frame, such as when it's pending
or pending delete.

BUG=590035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-12-03 01:57:10]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 01:57:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 02:30:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 02:30:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[alexmos, 2016-12-05 22:27:54]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 22:28:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 23:17:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 23:17:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alexmos, 2016-12-06 06:30:19]

Description was changed from

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RFH::GetLastCommittedOrigin() correctly when the
RFH isn't a current RFH for a frame, such as when it's pending
or pending delete.

BUG=590035
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RFH::GetLastCommittedOrigin() correctly when the
RFH isn't a current RFH for a frame, such as when it's pending
or pending delete.

BUG=590035, 663740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-12-06 06:37:34]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 06:37:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2016-12-06 06:39:58]

Description was changed from

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RFH::GetLastCommittedOrigin() correctly when the
RFH isn't a current RFH for a frame, such as when it's pending
or pending delete.

BUG=590035, 663740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RenderFrameHost::GetLastCommittedOrigin() correctly
when the RenderFrameHost is different from a frame's current RFH, such
as when it's pending or pending deletion.

BUG=590035, 663740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-12-06 07:28:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 07:28:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alexmos, 2016-12-06 17:22:39]

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

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

Charlie, can you please take a look?

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:144: // current one for this frame
(such as when it's pending deletion).
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.

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:146: return
replication_state_.origin;
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).

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:648:
render_frame_host->set_last_committed_origin(params.origin);
I originally tried to do this inside SetCurrentOrigin, which matches what we did
for SetCurrentURL.  But that didn't work -- SetCurrentOrigin is called above
this, intentionally before CommitPending when oopifs are on, so that the
replication state already has the latest origin when it's sent with
FrameMsg_SwapOut.  But at that point, current_frame_host() is still wrong, so
FTN::SetCurrentOrigin can't use it to update the last committed origin.

I didn't do this next to SetCurrentOrigin above, since I thought it's better to
set this on the RFH after it really becomes the current RFH.

https://codereview.chromium.org/2546533007/diff/40001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2546533007/diff/40001/content/public/browser/...
content/public/browser/render_frame_host.h:114: virtual const url::Origin&
GetLastCommittedOrigin() = 0;
We should be able to remove the hack introduced in
https://codereview.chromium.org/2545523008 to work around pending delete RFHs,
but I left that for a separate CL, or maybe even just reverting that CL.

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

Thanks!  Lots to think through here, but it seems like a worthwhile change to
have a saner API.  LGTM.

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:144: // current one for this frame
(such as when it's pending deletion).
On 2016/12/06 17:22:40, alexmos (OOO until 12-8) wrote:
> 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.

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.

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:146: return
replication_state_.origin;
On 2016/12/06 17:22:40, alexmos (OOO until 12-8) wrote:
> 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).

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

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:648:
render_frame_host->set_last_committed_origin(params.origin);
On 2016/12/06 17:22:40, alexmos (OOO until 12-8) wrote:
> I originally tried to do this inside SetCurrentOrigin, which matches what we
did
> for SetCurrentURL.  But that didn't work -- SetCurrentOrigin is called above
> this, intentionally before CommitPending when oopifs are on, so that the
> replication state already has the latest origin when it's sent with
> FrameMsg_SwapOut.  But at that point, current_frame_host() is still wrong, so
> FTN::SetCurrentOrigin can't use it to update the last committed origin.
> 
> I didn't do this next to SetCurrentOrigin above, since I thought it's better
to
> set this on the RFH after it really becomes the current RFH.

Interesting.  Yes, I agree with your decisions.

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2546533007/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:138: const url::Origin&
GetLastCommittedOrigin() override;
Good catch.

https://codereview.chromium.org/2546533007/diff/40001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2546533007/diff/40001/content/public/browser/...
content/public/browser/render_frame_host.h:114: virtual const url::Origin&
GetLastCommittedOrigin() = 0;
On 2016/12/06 17:22:40, alexmos (OOO until 12-8) wrote:
> We should be able to remove the hack introduced in
> https://codereview.chromium.org/2545523008 to work around pending delete RFHs,
> but I left that for a separate CL, or maybe even just reverting that CL.

Acknowledged.

[alexmos, 2016-12-09 17:30:34]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2016-12-09 17:31:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 21:24:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481304634120090,
"parent_rev": "f5076083f6cabfb0e02c73104b85ec02ba7b2da7", "commit_rev":
"923a1b85277cc63cca9e2100bb38b4e8e0283fe1"}

[commit-bot: I haz the power, 2016-12-09 21:25:11]

Description was changed from

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RenderFrameHost::GetLastCommittedOrigin() correctly
when the RenderFrameHost is different from a frame's current RFH, such
as when it's pending or pending deletion.

BUG=590035, 663740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RenderFrameHost::GetLastCommittedOrigin() correctly
when the RenderFrameHost is different from a frame's current RFH, such
as when it's pending or pending deletion.

BUG=590035, 663740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2546533007
==========

[commit-bot: I haz the power, 2016-12-09 21:25:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-12 14:41:09]

Description was changed from

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RenderFrameHost::GetLastCommittedOrigin() correctly
when the RenderFrameHost is different from a frame's current RFH, such
as when it's pending or pending deletion.

BUG=590035, 663740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2546533007
==========

to

==========
Store the last committed origin in RenderFrameHostImpl.

The allows using RenderFrameHost::GetLastCommittedOrigin() correctly
when the RenderFrameHost is different from a frame's current RFH, such
as when it's pending or pending deletion.

BUG=590035, 663740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e47445d4e35de16d2e68ecc34b7d2289a60f1c64
Cr-Commit-Position: refs/heads/master@{#437660}
==========

[commit-bot: I haz the power, 2016-12-12 14:41:11]

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