Avoid mutating frame owner when detaching a provisional frame.

Avoid mutating frame owner's when detaching a provisional frame.

When detaching a provisional frame, Blink was incorrectly clearing the
content frame of the provisional frame's frame owner. A provisional
frame is only partially attached to a frame tree. While it may have a
frame owner set, the frame owner's content frame will not point back
at the provisional frame. Similarly, though it may have a parent frame,
the provisional frame will not appear in the parent frame's list of
child nodes. Thus, it is important not to affect the frame tree
structure when detaching a provisional frame.

BUG=681077, 578349
Review-Url: https://codereview.chromium.org/2631233003
Cr-Commit-Position: refs/heads/master@{#444021}
Committed: https://chromium.googlesource.com/chromium/src/+/0401e95afc66f0b87733d04fad5d233c04d35aa3

[dcheng, 2017-01-17 08:17:21]

Description was changed from

==========
Avoid mutating frame owner's when detaching a provisional frame.

A provisional frame is only partially attached to a frame tree. While it
may have a frame owner set, its frame owner's content frame will not
point back at the provisional frame, and similarly for the parent and
sibling frame pointers.

BUG=
==========

to

==========
Avoid mutating frame owner's when detaching a provisional frame.

When detaching a provisional frame, Blink was incorrectly clearing the
content frame of the provisional frame's frame owner. A provisional
frame is only partially attached to a frame tree. While it may have a
frame owner set, the frame owner's content frame will not point back
at the provisional frame. Similarly, though it may have a parent frame,
the provisional frame will not appear in the parent frame's list of
child nodes. Thus, it is important not to affect the frame tree
structure when detaching a provisional frame.

BUG=681077,578349
==========

[dcheng, 2017-01-17 08:17:22]

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

[dcheng, 2017-01-17 09:22:59]

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

[commit-bot: I haz the power, 2017-01-17 09:23:13]

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

[haraken, 2017-01-17 09:30:23]

LGTM

https://codereview.chromium.org/2631233003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/2631233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Frame.cpp:90: if (m_owner->contentFrame()
== this)

Is there any way to add something like DCHECK(isProvisionalFrame())?

[dcheng, 2017-01-17 09:38:29]

https://codereview.chromium.org/2631233003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/2631233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Frame.cpp:90: if (m_owner->contentFrame()
== this)
On 2017/01/17 09:30:22, haraken wrote:
> 
> Is there any way to add something like DCHECK(isProvisionalFrame())?

I considered something like that, but didn't add it in the end for several
reasons:
- The provisional frame state is unique to local frames, so putting it on Frame
is a bit weird.
- If it's LocalFrame only, then disconnectContentFrame() has to be virtual or we
need to downcast to LocalFrame here. That also feels a bit awkward.

In the end, the provisional frame state is supposed to be temporary (i.e. after
PlzNavigate ships, we can clean up the architecture to make this eventually
unnecessary). For that reason, I'm trying to avoid making this concept too
formal for now (since I don't want too much code to depend on it: that's why
there's a TODO in this CL to clean up the random dependencies on
DummyFrameOwner).

WDYT?

[dcheng, 2017-01-17 09:39:03]

On 2017/01/17 09:38:29, dcheng wrote:
>
https://codereview.chromium.org/2631233003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/Frame.cpp (right):
> 
>
https://codereview.chromium.org/2631233003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/Frame.cpp:90: if (m_owner->contentFrame()
> == this)
> On 2017/01/17 09:30:22, haraken wrote:
> > 
> > Is there any way to add something like DCHECK(isProvisionalFrame())?
> 
> I considered something like that, but didn't add it in the end for several
> reasons:
> - The provisional frame state is unique to local frames, so putting it on
Frame
> is a bit weird.
> - If it's LocalFrame only, then disconnectContentFrame() has to be virtual or
we
> need to downcast to LocalFrame here. That also feels a bit awkward.
> 
> In the end, the provisional frame state is supposed to be temporary (i.e.
after
> PlzNavigate ships, we can clean up the architecture to make this eventually
> unnecessary). For that reason, I'm trying to avoid making this concept too
> formal for now (since I don't want too much code to depend on it: that's why
> there's a TODO in this CL to clean up the random dependencies on
> DummyFrameOwner).
> 
> WDYT?

That being said, if this appears to be a problem in other places in the future,
I think it will probably make sense to just add it. But right now, the hacks are
fairly limited in scope and I'm hoping they'll stay that way.

[haraken, 2017-01-17 09:39:40]

On 2017/01/17 09:39:03, dcheng wrote:
> On 2017/01/17 09:38:29, dcheng wrote:
> >
>
https://codereview.chromium.org/2631233003/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/frame/Frame.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2631233003/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/frame/Frame.cpp:90: if
(m_owner->contentFrame()
> > == this)
> > On 2017/01/17 09:30:22, haraken wrote:
> > > 
> > > Is there any way to add something like DCHECK(isProvisionalFrame())?
> > 
> > I considered something like that, but didn't add it in the end for several
> > reasons:
> > - The provisional frame state is unique to local frames, so putting it on
> Frame
> > is a bit weird.
> > - If it's LocalFrame only, then disconnectContentFrame() has to be virtual
or
> we
> > need to downcast to LocalFrame here. That also feels a bit awkward.
> > 
> > In the end, the provisional frame state is supposed to be temporary (i.e.
> after
> > PlzNavigate ships, we can clean up the architecture to make this eventually
> > unnecessary). For that reason, I'm trying to avoid making this concept too
> > formal for now (since I don't want too much code to depend on it: that's why
> > there's a TODO in this CL to clean up the random dependencies on
> > DummyFrameOwner).
> > 
> > WDYT?
> 
> That being said, if this appears to be a problem in other places in the
future,
> I think it will probably make sense to just add it. But right now, the hacks
are
> fairly limited in scope and I'm hoping they'll stay that way.

Makes sense.

[dcheng, 2017-01-17 09:41:58]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-01-17 09:41:59]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-01-17 09:42:11]

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, 2017-01-17 11:07:04]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484646119096700,
"parent_rev": "17d77116e97bade618e29ce717c26ed88e439dc7", "commit_rev":
"0401e95afc66f0b87733d04fad5d233c04d35aa3"}

[commit-bot: I haz the power, 2017-01-17 11:07:31]

Description was changed from

==========
Avoid mutating frame owner's when detaching a provisional frame.

When detaching a provisional frame, Blink was incorrectly clearing the
content frame of the provisional frame's frame owner. A provisional
frame is only partially attached to a frame tree. While it may have a
frame owner set, the frame owner's content frame will not point back
at the provisional frame. Similarly, though it may have a parent frame,
the provisional frame will not appear in the parent frame's list of
child nodes. Thus, it is important not to affect the frame tree
structure when detaching a provisional frame.

BUG=681077,578349
==========

to

==========
Avoid mutating frame owner's when detaching a provisional frame.

When detaching a provisional frame, Blink was incorrectly clearing the
content frame of the provisional frame's frame owner. A provisional
frame is only partially attached to a frame tree. While it may have a
frame owner set, the frame owner's content frame will not point back
at the provisional frame. Similarly, though it may have a parent frame,
the provisional frame will not appear in the parent frame's list of
child nodes. Thus, it is important not to affect the frame tree
structure when detaching a provisional frame.

BUG=681077,578349

Review-Url: https://codereview.chromium.org/2631233003
Cr-Commit-Position: refs/heads/master@{#444021}
Committed:
https://chromium.googlesource.com/chromium/src/+/0401e95afc66f0b87733d04fad5d...
==========

[commit-bot: I haz the power, 2017-01-17 11:07:32]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0401e95afc66f0b87733d04fad5d...