OOPIF: Use the same uniqueName when swapping between local and remote.

OOPIF: Use the same uniqueName when swapping between local and remote.

When swapping from a RemoteFrame to a LocalFrame, we now preserve both
the name and unique name.

BUG=502317
TEST=Unique name doesn't change when an OOPIF navigates into parent process.

Committed: https://crrev.com/52fc30ea9456e3aa2bc7cab45bff11db6d6c00ed
Cr-Commit-Position: refs/heads/master@{#361805}

[Charlie Reis, 2015-11-06 23:02:22]

Description was changed from

==========
OOPIF: Use the same uniqueName when swapping between local and remote.

BUG=502317
TEST=Unique name doesn't change when an OOPIF navigates into parent process.
==========

to

==========
OOPIF: Use the same uniqueName when swapping between local and remote.

BUG=502317
TEST=Unique name doesn't change when an OOPIF navigates into parent process.
==========

[Charlie Reis, 2015-11-06 23:04:48]

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

[Charlie Reis, 2015-11-06 23:04:48]

Daniel, here's the change I had been considering, for context in the discussion.

It does make sense to start moving uniqueName to the browser process, but it
will continue to live in the renderer for a little while (for default Chrome,
for layout tests, and for DocumentLoader::prepareSubframeArchiveLoadIfNeeded). 
Seems like it might be saner to have the browser and renderer agree on the
uniqueName while we're doing the move.

https://codereview.chromium.org/1413093007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1413093007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2006: // TODO(creis): Remove
|name| parameter.
We would need to be able to call WebRemoteFrame::name() for this.

https://codereview.chromium.org/1413093007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/1413093007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:90: return
frame()->tree().uniqueName();
I'm not sure why name and uniqueName aren't allowed on WebRemoteFrame.  We
already have setReplicatedName, which kind of gets around these
ASSERT_NOT_REACHEDs.  Names might be something that belong on Frame rather than
LocalFrame?

[dcheng, 2015-11-09 06:01:21]

I feel pretty strongly that we shouldn't add a new callback for this. The order
of callbacks that the embedder receives in the process of navigating and loading
a new document is already very complex. The fact that this happens to be called
right before we start building HistoryItem state isn't really very apparent (and
it's quite likely that other people that start using this callback will add hard
dependencies on a certain order of operations, making future cleanup in this
area harder).

If FrameTree were shareable among frames, this would solve the problem too,
right?

[Charlie Reis, 2015-11-09 07:09:00]

On 2015/11/09 06:01:21, dcheng wrote:
> I feel pretty strongly that we shouldn't add a new callback for this. The
order
> of callbacks that the embedder receives in the process of navigating and
loading
> a new document is already very complex. The fact that this happens to be
called
> right before we start building HistoryItem state isn't really very apparent
(and
> it's quite likely that other people that start using this callback will add
hard
> dependencies on a certain order of operations, making future cleanup in this
> area harder).
> 
> If FrameTree were shareable among frames, this would solve the problem too,
> right?

Yes, that would let local and remote frames for the same FrameTree keep the same
name, etc.

I know we have other code dealing with provisional frames that are not in the
tree yet (i.e., their parents don't know about them).  Would such a change make
that simpler or harder?  Is it hard for other reasons, like giving FrameTree a
notion of a provisional frame?  I'm not really sure what the implications would
be here.

(FWIW, patch set 2 was my earlier approach, which forcibly gave the replacement
frame the same uniqueName.  This ran the risk of having a stale name if the old
frame's name changed after the provisional frame was created, though.)

[dcheng, 2015-11-09 09:03:06]

On 2015/11/09 at 07:09:00, creis wrote:
> On 2015/11/09 06:01:21, dcheng wrote:
> > I feel pretty strongly that we shouldn't add a new callback for this. The
order
> > of callbacks that the embedder receives in the process of navigating and
loading
> > a new document is already very complex. The fact that this happens to be
called
> > right before we start building HistoryItem state isn't really very apparent
(and
> > it's quite likely that other people that start using this callback will add
hard
> > dependencies on a certain order of operations, making future cleanup in this
> > area harder).
> > 
> > If FrameTree were shareable among frames, this would solve the problem too,
> > right?
> 
> Yes, that would let local and remote frames for the same FrameTree keep the
same name, etc.
> 
> I know we have other code dealing with provisional frames that are not in the
tree yet (i.e., their parents don't know about them).  Would such a change make
that simpler or harder?  Is it hard for other reasons, like giving FrameTree a
notion of a provisional frame?  I'm not really sure what the implications would
be here.

My proposal is that FrameTree would just be something that happened to be
shareable between two frames (ideally with assertions that one of the frames is
actually in the tree at any given time), rather than explicitly adding the
provisional frame concept to Blink.

It's not clear to me that it would be an overall simplification of the code: I
think it's probably a win for name / unique name, since they're already stored
there, but things like FrameOwner might be a little more awkward to share in a
similar manner.

> 
> (FWIW, patch set 2 was my earlier approach, which forcibly gave the
replacement frame the same uniqueName.  This ran the risk of having a stale name
if the old frame's name changed after the provisional frame was created,
though.)

[Charlie Reis, 2015-11-09 20:01:37]

On 2015/11/09 09:03:06, dcheng wrote:
> On 2015/11/09 at 07:09:00, creis wrote:
> > On 2015/11/09 06:01:21, dcheng wrote:
> > > I feel pretty strongly that we shouldn't add a new callback for this. The
> order
> > > of callbacks that the embedder receives in the process of navigating and
> loading
> > > a new document is already very complex. The fact that this happens to be
> called
> > > right before we start building HistoryItem state isn't really very
apparent
> (and
> > > it's quite likely that other people that start using this callback will
add
> hard
> > > dependencies on a certain order of operations, making future cleanup in
this
> > > area harder).
> > > 
> > > If FrameTree were shareable among frames, this would solve the problem
too,
> > > right?
> > 
> > Yes, that would let local and remote frames for the same FrameTree keep the
> same name, etc.
> > 
> > I know we have other code dealing with provisional frames that are not in
the
> tree yet (i.e., their parents don't know about them).  Would such a change
make
> that simpler or harder?  Is it hard for other reasons, like giving FrameTree a
> notion of a provisional frame?  I'm not really sure what the implications
would
> be here.
> 
> My proposal is that FrameTree would just be something that happened to be
> shareable between two frames (ideally with assertions that one of the frames
is
> actually in the tree at any given time), rather than explicitly adding the
> provisional frame concept to Blink.
> 
> It's not clear to me that it would be an overall simplification of the code: I
> think it's probably a win for name / unique name, since they're already stored
> there, but things like FrameOwner might be a little more awkward to share in a
> similar manner.
> 
> > 
> > (FWIW, patch set 2 was my earlier approach, which forcibly gave the
> replacement frame the same uniqueName.  This ran the risk of having a stale
name
> if the old frame's name changed after the provisional frame was created,
> though.)

This seems a little subtle, since Frame and FrameTree have pointers to each
other.  You're suggesting that the replacement LocalFrame get a pointer to the
RemoteFrame's FrameTree while it's provisional, and then the FrameTree pointer
gets updated from the RemoteFrame to the LocalFrame during swap?

Also, we would need to do this for Local to Remote swaps as well, right?  

I'm not super familiar with the swap code, but I could see how it goes.

[dcheng, 2015-11-09 20:35:57]

On 2015/11/09 at 20:01:37, creis wrote:
> On 2015/11/09 09:03:06, dcheng wrote:
> > On 2015/11/09 at 07:09:00, creis wrote:
> > > On 2015/11/09 06:01:21, dcheng wrote:
> > > > I feel pretty strongly that we shouldn't add a new callback for this.
The
> > order
> > > > of callbacks that the embedder receives in the process of navigating and
> > loading
> > > > a new document is already very complex. The fact that this happens to be
> > called
> > > > right before we start building HistoryItem state isn't really very
apparent
> > (and
> > > > it's quite likely that other people that start using this callback will
add
> > hard
> > > > dependencies on a certain order of operations, making future cleanup in
this
> > > > area harder).
> > > > 
> > > > If FrameTree were shareable among frames, this would solve the problem
too,
> > > > right?
> > > 
> > > Yes, that would let local and remote frames for the same FrameTree keep
the
> > same name, etc.
> > > 
> > > I know we have other code dealing with provisional frames that are not in
the
> > tree yet (i.e., their parents don't know about them).  Would such a change
make
> > that simpler or harder?  Is it hard for other reasons, like giving FrameTree
a
> > notion of a provisional frame?  I'm not really sure what the implications
would
> > be here.
> > 
> > My proposal is that FrameTree would just be something that happened to be
> > shareable between two frames (ideally with assertions that one of the frames
is
> > actually in the tree at any given time), rather than explicitly adding the
> > provisional frame concept to Blink.
> > 
> > It's not clear to me that it would be an overall simplification of the code:
I
> > think it's probably a win for name / unique name, since they're already
stored
> > there, but things like FrameOwner might be a little more awkward to share in
a
> > similar manner.
> > 
> > > 
> > > (FWIW, patch set 2 was my earlier approach, which forcibly gave the
> > replacement frame the same uniqueName.  This ran the risk of having a stale
name
> > if the old frame's name changed after the provisional frame was created,
> > though.)
> 
> This seems a little subtle, since Frame and FrameTree have pointers to each
other.  You're suggesting that the replacement LocalFrame get a pointer to the
RemoteFrame's FrameTree while it's provisional, and then the FrameTree pointer
gets updated from the RemoteFrame to the LocalFrame during swap?

There's several possibilities (in no particular order):
1. The proposed approach just mentioned (updating FrameTree::m_thisFrame in the
swap).
2. Moving m_name/m_uniqueName to a RefCountedWilLBeGarbageCollected class /
struct and just sharing that.
3. Moving the m_parent/m_nextSibling/m_previousSibling/m_firstChild/m_lastChild
fields back into FrameTree. Then FrameTree wouldn't need m_thisFrame anymore...
but the flip side is WebFrame::swap() would need a FrameTree::swap() helper. For
consistency, we'd probably also want to move OpenedFrameTracker back into
Source/core then.

I think #2 is probably the easiest and I'd be OK with that. I need to think more
about #3 (I think it might be an interesting idea, but it's more involved).

> 
> Also, we would need to do this for Local to Remote swaps as well, right?  
> 
> I'm not super familiar with the swap code, but I could see how it goes.

[dcheng, 2015-11-25 05:59:57]

Charlie, are you OK with resurrecting PS2? I'd rather see this +
https://codereview.chromium.org/1422333009 landed: I can rebase my patch on top
of this pretty easily (and clean it up).

[dcheng, 2015-11-25 18:14:51]

https://codereview.chromium.org/1413093007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/1413093007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:90: return
frame()->tree().uniqueName();
On 2015/11/06 at 23:04:48, Charlie Reis wrote:
> I'm not sure why name and uniqueName aren't allowed on WebRemoteFrame.  We
already have setReplicatedName, which kind of gets around these
ASSERT_NOT_REACHEDs.  Names might be something that belong on Frame rather than
LocalFrame?

Actually, let's try to limit the API surface of WebRemoteFrame. Since we're
implementing this in web/, we can just reach directly into blink::Frame::tree()
to get the name and uniqueName.

[Charlie Reis, 2015-11-25 18:22:43]

On 2015/11/25 05:59:57, dcheng wrote:
> Charlie, are you OK with resurrecting PS2? I'd rather see this +
> https://codereview.chromium.org/1422333009 landed: I can rebase my patch on
top
> of this pretty easily (and clean it up).

As a short-term fix, right?  The approach in PS2 will use a stale name if the
frame's name changes during navigation.

I'll dust it off and upload a new patch.

https://codereview.chromium.org/1413093007/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/1413093007/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:90: return
frame()->tree().uniqueName();
On 2015/11/25 18:14:51, dcheng wrote:
> On 2015/11/06 at 23:04:48, Charlie Reis wrote:
> > I'm not sure why name and uniqueName aren't allowed on WebRemoteFrame.  We
> already have setReplicatedName, which kind of gets around these
> ASSERT_NOT_REACHEDs.  Names might be something that belong on Frame rather
than
> LocalFrame?
> 
> Actually, let's try to limit the API surface of WebRemoteFrame. Since we're
> implementing this in web/, we can just reach directly into
blink::Frame::tree()
> to get the name and uniqueName.

Ok, will do.

[Charlie Reis, 2015-11-25 23:35:01]

Ok, we're back to the approach from patch set 2, without changes to
WebRemoteFrameImpl::uniqueName.

[dcheng, 2015-11-25 23:43:26]

https://codereview.chromium.org/1413093007/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1413093007/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:7493: // Swap back to a
LocalFrame.
Do we actually swap the local frame back in?

[Charlie Reis, 2015-11-25 23:54:59]

https://codereview.chromium.org/1413093007/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1413093007/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:7493: // Swap back to a
LocalFrame.
On 2015/11/25 23:43:26, dcheng wrote:
> Do we actually swap the local frame back in?

Yep.  I just confirmed that the FrameTestHelpers::loadFrame gets to
WebFrame::swap().  That's also what the other swap tests appear to use (e.g.,
HistoryCommitTypeAfterExistingRemoteToLocalSwap).

[dcheng, 2015-11-26 00:29:38]

lgtm

[Charlie Reis, 2015-11-26 00:34:43]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-11-26 00:35:12]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2015-11-26 00:35:59]

Description was changed from

==========
OOPIF: Use the same uniqueName when swapping between local and remote.

When swapping from a RemoteFrame to a LocalFrame, we now set the name
at commit time, just before the target on the HistoryItem is set.
This ensures that the uniqueName for the frame stays the same.

BUG=502317
TEST=Unique name doesn't change when an OOPIF navigates into parent process.
==========

to

==========
OOPIF: Use the same uniqueName when swapping between local and remote.

When swapping from a RemoteFrame to a LocalFrame, we now preserve both
the name and unique name.

BUG=502317
TEST=Unique name doesn't change when an OOPIF navigates into parent process.
==========

[Charlie Reis, 2015-11-26 00:36:15]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-11-26 00:42:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413093007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413093007/80001

[dcheng, 2015-11-26 01:01:50]

https://codereview.chromium.org/1413093007/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1413093007/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:7493: // Swap back to a
LocalFrame.
On 2015/11/25 at 23:54:59, Charlie Reis wrote:
> On 2015/11/25 23:43:26, dcheng wrote:
> > Do we actually swap the local frame back in?
> 
> Yep.  I just confirmed that the FrameTestHelpers::loadFrame gets to
WebFrame::swap().  That's also what the other swap tests appear to use (e.g.,
HistoryCommitTypeAfterExistingRemoteToLocalSwap).

Looking around more, it seems like some tests explicitly swap it in. Are you
saying that's not required? If you have this in a debugger, do you mind
including the stack where we hit WebFrame::swap() in the loadFrame() call?

[commit-bot: I haz the power, 2015-11-26 01:37:10]

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

[commit-bot: I haz the power, 2015-11-26 01:37:11]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[dcheng, 2015-11-26 02:02:34]

https://codereview.chromium.org/1413093007/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1413093007/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:7493: // Swap back to a
LocalFrame.
On 2015/11/26 at 01:01:50, dcheng wrote:
> On 2015/11/25 at 23:54:59, Charlie Reis wrote:
> > On 2015/11/25 23:43:26, dcheng wrote:
> > > Do we actually swap the local frame back in?
> > 
> > Yep.  I just confirmed that the FrameTestHelpers::loadFrame gets to
WebFrame::swap().  That's also what the other swap tests appear to use (e.g.,
HistoryCommitTypeAfterExistingRemoteToLocalSwap).
> 
> Looking around more, it seems like some tests explicitly swap it in. Are you
saying that's not required? If you have this in a debugger, do you mind
including the stack where we hit WebFrame::swap() in the loadFrame() call?

OK, I see how this works now. This is fine (it happens magically inside the
frame client). This is too complicated =(

[dcheng, 2015-11-26 02:02:40]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-11-26 02:06:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1413093007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1413093007/80001

[commit-bot: I haz the power, 2015-11-26 02:44:02]

Description was changed from

==========
OOPIF: Use the same uniqueName when swapping between local and remote.

When swapping from a RemoteFrame to a LocalFrame, we now preserve both
the name and unique name.

BUG=502317
TEST=Unique name doesn't change when an OOPIF navigates into parent process.
==========

to

==========
OOPIF: Use the same uniqueName when swapping between local and remote.

When swapping from a RemoteFrame to a LocalFrame, we now preserve both
the name and unique name.

BUG=502317
TEST=Unique name doesn't change when an OOPIF navigates into parent process.
==========

[commit-bot: I haz the power, 2015-11-26 02:44:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-26 02:45:10]

Description was changed from

==========
OOPIF: Use the same uniqueName when swapping between local and remote.

When swapping from a RemoteFrame to a LocalFrame, we now preserve both
the name and unique name.

BUG=502317
TEST=Unique name doesn't change when an OOPIF navigates into parent process.
==========

to

==========
OOPIF: Use the same uniqueName when swapping between local and remote.

When swapping from a RemoteFrame to a LocalFrame, we now preserve both
the name and unique name.

BUG=502317
TEST=Unique name doesn't change when an OOPIF navigates into parent process.

Committed: https://crrev.com/52fc30ea9456e3aa2bc7cab45bff11db6d6c00ed
Cr-Commit-Position: refs/heads/master@{#361805}
==========

[commit-bot: I haz the power, 2015-11-26 02:45:11]

Patchset 5 (id:??) landed as
https://crrev.com/52fc30ea9456e3aa2bc7cab45bff11db6d6c00ed
Cr-Commit-Position: refs/heads/master@{#361805}