Streamline frame detach

Streamline frame detach

Share a bit more code between LocalFrame and RemoteFrame, including moving the detach-time client notification down to FrameClient.

Move the max frame counter to FrameHost instead of Page.

Move detach logic from FrameLoader to LocalFrame, unless it manipulates FrameLoader members.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182999

[Nate Chapin, 2014-09-16 19:28:55]

japhet@chromium.org changed reviewers:
+ abarth@chromium.org, dcheng@chromium.org

[dcheng, 2014-09-16 19:46:11]

(I didn't really look at the CL in too much detail, so just one quick comment).

https://codereview.chromium.org/551973005/diff/20001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/551973005/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:99: // FIXME: Understand why this is happening so we
can document this insanity.
I think (but could be wrong) that this is actually related to
https://crbug.com/371084 -- so once I disable plugins from executing script
during frame detach, this /should/ go away. Would you mind adding a reference to
that bug here?

[sof, 2014-09-16 19:56:35]

A minor plea would be not to change/tidy the logic too much here while
https://codereview.chromium.org/517043003/ is in play. Changing the detach &
finalization logic has a direct bearing on it.

[Nate Chapin, 2014-09-16 20:01:10]

On 2014/09/16 19:56:35, sof wrote:
> A minor plea would be not to change/tidy the logic too much here while
> https://codereview.chromium.org/517043003/ is in play. Changing the detach &
> finalization logic has a direct bearing on it.

I'll see what we can do. Do you have an ETA on
https://codereview.chromium.org/517043003/ ?

[sof, 2014-09-16 20:53:54]

On 2014/09/16 20:01:10, Nate Chapin wrote:
> On 2014/09/16 19:56:35, sof wrote:
> > A minor plea would be not to change/tidy the logic too much here while
> > https://codereview.chromium.org/517043003/ is in play. Changing the detach &
> > finalization logic has a direct bearing on it.
> 
> I'll see what we can do. Do you have an ETA on
> https://codereview.chromium.org/517043003/ ?

thanks, we'll just have to adjust if you do blaze ahead.

ETA - it's hard to say, spending most of my time on it, but correctly recasting
the shutdown/finalization steps into Oilpan is detail filled.

[Nate Chapin, 2014-09-16 21:00:49]

On 2014/09/16 20:53:54, sof wrote:
> On 2014/09/16 20:01:10, Nate Chapin wrote:
> > On 2014/09/16 19:56:35, sof wrote:
> > > A minor plea would be not to change/tidy the logic too much here while
> > > https://codereview.chromium.org/517043003/ is in play. Changing the detach
&
> > > finalization logic has a direct bearing on it.
> > 
> > I'll see what we can do. Do you have an ETA on
> > https://codereview.chromium.org/517043003/ ?
> 
> thanks, we'll just have to adjust if you do blaze ahead.
> 
> ETA - it's hard to say, spending most of my time on it, but correctly
recasting
> the shutdown/finalization steps into Oilpan is detail filled.

Sounds good. This patch isn't especially urgent, it's part streamlining and part
helping me make sure I know the detach process well enough to do site isolation
work :)

[sof, 2014-09-19 11:46:38]

On 2014/09/16 21:00:49, Nate Chapin wrote:
> On 2014/09/16 20:53:54, sof wrote:
> > On 2014/09/16 20:01:10, Nate Chapin wrote:
> > > On 2014/09/16 19:56:35, sof wrote:
> > > > A minor plea would be not to change/tidy the logic too much here while
> > > > https://codereview.chromium.org/517043003/ is in play. Changing the
detach
> &
> > > > finalization logic has a direct bearing on it.
> > > 
> > > I'll see what we can do. Do you have an ETA on
> > > https://codereview.chromium.org/517043003/ ?
> > 
> > thanks, we'll just have to adjust if you do blaze ahead.
> > 
> > ETA - it's hard to say, spending most of my time on it, but correctly
> recasting
> > the shutdown/finalization steps into Oilpan is detail filled.
> 
> Sounds good. This patch isn't especially urgent, it's part streamlining and
part
> helping me make sure I know the detach process well enough to do site
isolation
> work :)

https://codereview.chromium.org/517043003/ is now in the clear as far as frame
detach&finalization goes, I think. The worry was that we had to "atomize" the
detach actions so as to make them fit with Oilpan finalization, which would have
been harder to reconcile with this concurrent change. But thankfully not needed,
so no need to hold back good work :)

Thanks for holding off though & agreeing to the unusual request.

[Nate Chapin, 2014-09-23 20:00:35]

On 2014/09/19 11:46:38, sof wrote:
> On 2014/09/16 21:00:49, Nate Chapin wrote:
> > On 2014/09/16 20:53:54, sof wrote:
> > > On 2014/09/16 20:01:10, Nate Chapin wrote:
> > > > On 2014/09/16 19:56:35, sof wrote:
> > > > > A minor plea would be not to change/tidy the logic too much here while
> > > > > https://codereview.chromium.org/517043003/ is in play. Changing the
> detach
> > &
> > > > > finalization logic has a direct bearing on it.
> > > > 
> > > > I'll see what we can do. Do you have an ETA on
> > > > https://codereview.chromium.org/517043003/ ?
> > > 
> > > thanks, we'll just have to adjust if you do blaze ahead.
> > > 
> > > ETA - it's hard to say, spending most of my time on it, but correctly
> > recasting
> > > the shutdown/finalization steps into Oilpan is detail filled.
> > 
> > Sounds good. This patch isn't especially urgent, it's part streamlining and
> part
> > helping me make sure I know the detach process well enough to do site
> isolation
> > work :)
> 
> https://codereview.chromium.org/517043003/ is now in the clear as far as frame
> detach&finalization goes, I think. The worry was that we had to "atomize" the
> detach actions so as to make them fit with Oilpan finalization, which would
have
> been harder to reconcile with this concurrent change. But thankfully not
needed,
> so no need to hold back good work :)
> 
> Thanks for holding off though & agreeing to the unusual request.

Rebased post-oilpanization, added comment about re-entrant detach per dcheng's
request.

[dcheng, 2014-09-25 23:35:44]

https://codereview.chromium.org/551973005/diff/40001/Source/core/dom/NodeRare...
File Source/core/dom/NodeRareData.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/dom/NodeRare...
Source/core/dom/NodeRareData.cpp:80: COMPILE_ASSERT(FrameHost::maxNumberOfFrames
< (1 << NodeRareData::ConnectedFrameCountBits),
Frame_limit_should_fit_in_rare_data_count);
static_assert? =)

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:108: // client() should never be null because that
means we somehow re-entered
This was a little hard for me to follow. We needed this comment originally
because this happened after all the other stuff in
FrameLoader::detachFromParent. This still happens 'last' so to speak, but it's
not so obvious. I don't have any good suggestions though...

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:115: m_host = nullptr;
I don't think we need to do it in this patch, but I wonder if it's worth
enforcing the invariant that host(), client() and page() must all go to null at
the same time?

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:169: m_loader.closeURL();
Out of curiosity, is there a reason you chose to reorder this? This results in a
web-visible change, doesn't it? I think unload events in parents are supposed to
fire before unload events in children:
https://html.spec.whatwg.org/multipage/browsers.html#unloading-documents ("When
a user agent is to unload a document, it must run the following steps.")

https://codereview.chromium.org/551973005/diff/40001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1091: Frame* top = m_frame->tree().top();
Was this a necessary change or just a cleanup change?

https://codereview.chromium.org/551973005/diff/40001/Source/web/RemoteFrameCl...
File Source/web/RemoteFrameClient.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/web/RemoteFrameCl...
Source/web/RemoteFrameClient.cpp:21: // FIXME: Implement.
What do you have in mind here? Or is this just so we can pull the detached()
call up into Frame?

[Nate Chapin, 2014-09-25 23:57:51]

https://codereview.chromium.org/551973005/diff/40001/Source/core/dom/NodeRare...
File Source/core/dom/NodeRareData.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/dom/NodeRare...
Source/core/dom/NodeRareData.cpp:80: COMPILE_ASSERT(FrameHost::maxNumberOfFrames
< (1 << NodeRareData::ConnectedFrameCountBits),
Frame_limit_should_fit_in_rare_data_count);
On 2014/09/25 23:35:43, dcheng wrote:
> static_assert? =)

Done.

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:108: // client() should never be null because that
means we somehow re-entered
On 2014/09/25 23:35:43, dcheng wrote:
> This was a little hard for me to follow. We needed this comment originally
> because this happened after all the other stuff in
> FrameLoader::detachFromParent. This still happens 'last' so to speak, but it's
> not so obvious. I don't have any good suggestions though...

It may be that this comment should be on the client() null-check in
LocalFrame::detach().

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:115: m_host = nullptr;
On 2014/09/25 23:35:43, dcheng wrote:
> I don't think we need to do it in this patch, but I wonder if it's worth
> enforcing the invariant that host(), client() and page() must all go to null
at
> the same time?

That seems like a good idea for the future.

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:169: m_loader.closeURL();
On 2014/09/25 23:35:43, dcheng wrote:
> Out of curiosity, is there a reason you chose to reorder this? This results in
a
> web-visible change, doesn't it? I think unload events in parents are supposed
to
> fire before unload events in children:
> https://html.spec.whatwg.org/multipage/browsers.html#unloading-documents
("When
> a user agent is to unload a document, it must run the following steps.")

So it appears. I was grumpy about the 2 stopAllLoaders() calls during detach,
and didn't think to check the spec before trying to optimize one of them away :(

https://codereview.chromium.org/551973005/diff/40001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:1091: Frame* top = m_frame->tree().top();
On 2014/09/25 23:35:43, dcheng wrote:
> Was this a necessary change or just a cleanup change?

I think it might have been necessary as a result of a mistake that I then later
fixed? I can't remember anymore :(

I'll revert it for now.

https://codereview.chromium.org/551973005/diff/40001/Source/web/RemoteFrameCl...
File Source/web/RemoteFrameClient.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/web/RemoteFrameCl...
Source/web/RemoteFrameClient.cpp:21: // FIXME: Implement.
On 2014/09/25 23:35:44, dcheng wrote:
> What do you have in mind here? Or is this just so we can pull the detached()
> call up into Frame?

Mostly so we can pull the detached() call up into Frame.

But also, I figured the odds are decent that RemoteFrames will need to do some
work when detach occurs eventually.

[dcheng, 2014-09-26 00:40:25]

LGTM with one nit and compile error fixed (I think the second argument to
static_assert is a string).

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:108: // client() should never be null because that
means we somehow re-entered
On 2014/09/25 at 23:57:50, Nate Chapin wrote:
> On 2014/09/25 23:35:43, dcheng wrote:
> > This was a little hard for me to follow. We needed this comment originally
> > because this happened after all the other stuff in
> > FrameLoader::detachFromParent. This still happens 'last' so to speak, but
it's
> > not so obvious. I don't have any good suggestions though...
> 
> It may be that this comment should be on the client() null-check in
LocalFrame::detach().

I'm not sure it really makes sense there either. I guess let's just leave it
here =/

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:115: m_host = nullptr;
On 2014/09/25 at 23:57:50, Nate Chapin wrote:
> On 2014/09/25 23:35:43, dcheng wrote:
> > I don't think we need to do it in this patch, but I wonder if it's worth
> > enforcing the invariant that host(), client() and page() must all go to null
at
> > the same time?
> 
> That seems like a good idea for the future.

Mind adding a FIXME?

[Nate Chapin, 2014-09-26 20:25:51]

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:115: m_host = nullptr;
On 2014/09/26 00:40:25, dcheng wrote:
> On 2014/09/25 at 23:57:50, Nate Chapin wrote:
> > On 2014/09/25 23:35:43, dcheng wrote:
> > > I don't think we need to do it in this patch, but I wonder if it's worth
> > > enforcing the invariant that host(), client() and page() must all go to
null
> at
> > > the same time?
> > 
> > That seems like a good idea for the future.
> 
> Mind adding a FIXME?

Now that I look at it, because page() is directly derived from host(), I think
this invariant is already true.

[dcheng, 2014-09-26 21:32:03]

On 2014/09/26 at 20:25:51, japhet wrote:
>
https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame.cpp
> File Source/core/frame/Frame.cpp (right):
> 
>
https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame....
> Source/core/frame/Frame.cpp:115: m_host = nullptr;
> On 2014/09/26 00:40:25, dcheng wrote:
> > On 2014/09/25 at 23:57:50, Nate Chapin wrote:
> > > On 2014/09/25 23:35:43, dcheng wrote:
> > > > I don't think we need to do it in this patch, but I wonder if it's worth
> > > > enforcing the invariant that host(), client() and page() must all go to
null
> > at
> > > > the same time?
> > > 
> > > That seems like a good idea for the future.
> > 
> > Mind adding a FIXME?
> 
> Now that I look at it, because page() is directly derived from host(), I think
this invariant is already true.

Technically, FrameClient::detached (and any code called by it) sees a view of
the world where host and page are null, but client is not.

[Nate Chapin, 2014-09-26 21:49:26]

On 2014/09/26 21:32:03, dcheng wrote:
> On 2014/09/26 at 20:25:51, japhet wrote:
> >
>
https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame.cpp
> > File Source/core/frame/Frame.cpp (right):
> > 
> >
>
https://codereview.chromium.org/551973005/diff/40001/Source/core/frame/Frame....
> > Source/core/frame/Frame.cpp:115: m_host = nullptr;
> > On 2014/09/26 00:40:25, dcheng wrote:
> > > On 2014/09/25 at 23:57:50, Nate Chapin wrote:
> > > > On 2014/09/25 23:35:43, dcheng wrote:
> > > > > I don't think we need to do it in this patch, but I wonder if it's
worth
> > > > > enforcing the invariant that host(), client() and page() must all go
to
> null
> > > at
> > > > > the same time?
> > > > 
> > > > That seems like a good idea for the future.
> > > 
> > > Mind adding a FIXME?
> > 
> > Now that I look at it, because page() is directly derived from host(), I
think
> this invariant is already true.
> 
> Technically, FrameClient::detached (and any code called by it) sees a view of
> the world where host and page are null, but client is not.

Fair point. Will add the FIXME before landing.

[Nate Chapin, 2014-09-29 18:43:35]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-29 18:44:54]

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

[commit-bot: I haz the power, 2014-09-29 18:59:18]

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

[commit-bot: I haz the power, 2014-09-29 18:59:19]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)

[Nate Chapin, 2014-09-29 19:50:36]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-29 19:51:26]

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

[commit-bot: I haz the power, 2014-09-29 21:05:44]

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

[commit-bot: I haz the power, 2014-09-29 21:05:46]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[Nate Chapin, 2014-09-29 23:49:59]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-29 23:50:22]

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

[commit-bot: I haz the power, 2014-09-29 23:54:02]

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

[commit-bot: I haz the power, 2014-09-29 23:54:03]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/25198)

[Nate Chapin, 2014-09-30 20:41:11]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-30 20:41:56]

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

[commit-bot: I haz the power, 2014-09-30 22:57:42]

Committed patchset #8 (id:140001) as 182999

[eustas, 2014-10-01 08:28:37]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/618253002/ by eustas@chromium.org.

The reason for reverting is: This patch breaks
SitePerProcessBrowserTest.NavigateRemoteFrame on swarming bots:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_....