Remove swapped out state from RenderFrameHost.

Remove swapped out state from RenderFrameHost.

Since swapped out RenderFrameHosts are no longer used, its swapped out
state is no longer necessary. This CL removes it along with a public
API that exposed it outside of content/.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7ecc9a63ae68521e2189683fae86f5e89405986b
Cr-Commit-Position: refs/heads/master@{#381601}

[nasko, 2016-03-14 17:29:14]

Description was changed from

==========
Remove swapped out state from RenderFrameHost.

Since swapped out RenderFrameHosts are no longer used, its swapped out
state is no longer necessary. This CL removes it along with a public
API that exposed it outside of content/.

BUG=357747
==========

to

==========
Remove swapped out state from RenderFrameHost.

Since swapped out RenderFrameHosts are no longer used, its swapped out
state is no longer necessary. This CL removes it along with a public
API that exposed it outside of content/.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-03-14 19:02:23]

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

[nasko, 2016-03-14 19:02:23]

Hey Charlie,
This CL removes the swapped out state of RenderFrameHost. Can you review it for
me?
Thanks in advance!
Nasko

[nasko, 2016-03-14 19:02:40]

Description was changed from

==========
Remove swapped out state from RenderFrameHost.

Since swapped out RenderFrameHosts are no longer used, its swapped out
state is no longer necessary. This CL removes it along with a public
API that exposed it outside of content/.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove swapped out state from RenderFrameHost.

Since swapped out RenderFrameHosts are no longer used, its swapped out
state is no longer necessary. This CL removes it along with a public
API that exposed it outside of content/.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-14 20:12:58]

Great!  Very excited for these.  A few questions below.

https://codereview.chromium.org/1799163002/diff/1/chrome/browser/ssl/ssl_brow...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1799163002/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:945: // the previous page.  Prevents
regression of http://crbug.com/82667.
This comment is a bit stale.  Also, maybe we can test for regression of that bug
by checking that the RFH wasn't deleted or replaced?  (Not sure if that's easy.)

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

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:622: if
(!render_frame_host->GetSiteInstance()->IsRelatedSiteInstance(
This looks wrong.  Before this block was unreachable because is_swapped_out()
was always false.  Now we can get to it.  Should we be removing the block
instead?

Alternatively, if this is a check that makes sense to have, we should update the
comment above it.  (I'm not sure off the top of my head if we do this
BrowsingInstance check sufficiently elsewhere.)

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:475: // Filter out most IPC
messages if this frame is swapped out.
I am so excited about removing this.  :)

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

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1343: CHECK(false);
Might be clearer if we CHECK the return value of DeleteFromPendingList instead
of having an early return above.

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1869: // of a
RenderFrameHost gets flipped to/from active.
Is it even possible for the state to go from inactive to active now?  Seems like
this can get even simpler.

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:94: CREATE_RF_SWAPPED_OUT =
1 << 0,
This can be removed now, right?  That would make the change to RFHI's
constructor easier to reason about.  (It looks unused in codesearch.)

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:116: // other active frames
in its SiteInstance) or it will be deleted.
Let's update this comment, too.

(Maybe sometime we can make this a bool rather than a state enum, but no rush
for that.)

[nasko, 2016-03-14 21:23:39]

Patchset #2 (id:20001) has been deleted

[nasko, 2016-03-14 21:32:29]

nasko@chromium.org changed reviewers:
+ sky@chromium.org

[nasko, 2016-03-14 21:32:30]

Fixes for the previous round of reviews.

sky@, can you do an OWNERS review of chrome/?

https://codereview.chromium.org/1799163002/diff/1/chrome/browser/ssl/ssl_brow...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1799163002/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:945: // the previous page.  Prevents
regression of http://crbug.com/82667.
On 2016/03/14 20:12:57, Charlie Reis (slow til 3-15) wrote:
> This comment is a bit stale.  Also, maybe we can test for regression of that
bug
> by checking that the RFH wasn't deleted or replaced?  (Not sure if that's
easy.)

Done.

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

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:622: if
(!render_frame_host->GetSiteInstance()->IsRelatedSiteInstance(
On 2016/03/14 20:12:57, Charlie Reis (slow til 3-15) wrote:
> This looks wrong.  Before this block was unreachable because is_swapped_out()
> was always false.  Now we can get to it.  Should we be removing the block
> instead?
> 
> Alternatively, if this is a check that makes sense to have, we should update
the
> comment above it.  (I'm not sure off the top of my head if we do this
> BrowsingInstance check sufficiently elsewhere.)

Ah, good point. We don't need the check overall, as if a RenderFrameHost is
getting this message, it must be alive or about to be deleted. If it is in the
process of swapping out, I think we still need to allow the call to go through
to keep it equivalent.

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:475: // Filter out most IPC
messages if this frame is swapped out.
On 2016/03/14 20:12:57, Charlie Reis (slow til 3-15) wrote:
> I am so excited about removing this.  :)

Same here :).

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

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1343: CHECK(false);
On 2016/03/14 20:12:57, Charlie Reis (slow til 3-15) wrote:
> Might be clearer if we CHECK the return value of DeleteFromPendingList instead
> of having an early return above.

Good idea. Done, but keeping a variable around to ensure that the call is made
unconditionally. Being paranoid about what CHECK might do in the future.

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1869: // of a
RenderFrameHost gets flipped to/from active.
On 2016/03/14 20:12:57, Charlie Reis (slow til 3-15) wrote:
> Is it even possible for the state to go from inactive to active now?  Seems
like
> this can get even simpler.

Done.

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:94: CREATE_RF_SWAPPED_OUT =
1 << 0,
On 2016/03/14 20:12:57, Charlie Reis (slow til 3-15) wrote:
> This can be removed now, right?  That would make the change to RFHI's
> constructor easier to reason about.  (It looks unused in codesearch.)

It is already done in a separate CL -
https://codereview.chromium.org/1801783003/. It will go away once I rebase this
one.

https://codereview.chromium.org/1799163002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.h:116: // other active frames
in its SiteInstance) or it will be deleted.
On 2016/03/14 20:12:57, Charlie Reis (slow til 3-15) wrote:
> Let's update this comment, too.
> 
> (Maybe sometime we can make this a bool rather than a state enum, but no rush
> for that.)

Yeah, I'd convert this to boolean separately.

[nasko, 2016-03-14 21:32:57]

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

[commit-bot: I haz the power, 2016-03-14 21:33:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1799163002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1799163002/60001

[Charlie Reis, 2016-03-14 21:49:15]

Thanks, LGTM.

https://codereview.chromium.org/1799163002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1799163002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1875: // Whenever we change
the RFH state to and from active state, we should not be
Let's come back to cleaning up this method after this CL lands.  This part is
stale too, and I'm not convinced we need a general SetState method anymore. 
RFHs only go from default to pending deletion and then are deleted.

Makes sense to do that in a separate CL.

[sky, 2016-03-14 23:26:53]

LGTM

[nasko, 2016-03-14 23:31:43]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2016-03-14 23:31:51]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2016-03-14 23:32:14]

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

[commit-bot: I haz the power, 2016-03-15 03:19:56]

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

[commit-bot: I haz the power, 2016-03-15 03:19:57]

Try jobs failed on following builders:
  linux_site_isolation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[nasko, 2016-03-16 22:52:31]

Hey Charlie,
Can you take another look? Patchset 3 fixes the problematic layout test and adds
an explicit test for the failure. There was a rebase in the meantime, but it was
very clean and you can ignore navigator_impl.cc, the rest is as though there was
no rebase.
Thanks in advance!
Nasko

[Charlie Reis, 2016-03-16 23:18:46]

Thanks for adding the test!  Still LGTM, but one request for a check below.

https://codereview.chromium.org/1799163002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1799163002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1874:
render_view_host_->set_is_swapped_out(false);
Let's definitely come back and update this method soon, since it's in a
dangerous state right now.  We're assuming that it can only be called to go from
default to pending swap out here, though the contract doesn't look like that and
we don't assert it.  (It just so happens that it's true based on how we call
it.)

In fact, I think I'd prefer if we added an assertion that |rfh_state| is
STATE_PENDING_SWAP_OUT at the start of the method in this CL.  Just seems safer
than risking running this line when |rfh_state| is somehow STATE_DEFAULT.

[Charlie Reis, 2016-03-16 23:47:02]

https://codereview.chromium.org/1799163002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1799163002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1874:
render_view_host_->set_is_swapped_out(false);
On 2016/03/16 23:18:46, Charlie Reis wrote:
> Let's definitely come back and update this method soon, since it's in a
> dangerous state right now.  We're assuming that it can only be called to go
from
> default to pending swap out here, though the contract doesn't look like that
and
> we don't assert it.  (It just so happens that it's true based on how we call
> it.)
> 
> In fact, I think I'd prefer if we added an assertion that |rfh_state| is
> STATE_PENDING_SWAP_OUT at the start of the method in this CL.  Just seems
safer
> than risking running this line when |rfh_state| is somehow STATE_DEFAULT.

Nevermind-- as you pointed out offline, this is indeed called with STATE_DEFAULT
in some cases, and that's not a problem.  It's not trying to toggle, and we set
this to false for both STATE_DEFAULT and STATE_PENDING_SWAP_OUT.

The method is still in a bit of a confusing state, but as long as we come back
to it in the next CL, I'm ok with it as is.

[nasko, 2016-03-16 23:51:01]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-03-16 23:51:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1799163002/#ps80001
(title: "Fixing failing layout test and adding an explicit test.")

[commit-bot: I haz the power, 2016-03-16 23:51:32]

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

[commit-bot: I haz the power, 2016-03-17 00:22:30]

Description was changed from

==========
Remove swapped out state from RenderFrameHost.

Since swapped out RenderFrameHosts are no longer used, its swapped out
state is no longer necessary. This CL removes it along with a public
API that exposed it outside of content/.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove swapped out state from RenderFrameHost.

Since swapped out RenderFrameHosts are no longer used, its swapped out
state is no longer necessary. This CL removes it along with a public
API that exposed it outside of content/.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-17 00:22:31]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-17 00:24:22]

Description was changed from

==========
Remove swapped out state from RenderFrameHost.

Since swapped out RenderFrameHosts are no longer used, its swapped out
state is no longer necessary. This CL removes it along with a public
API that exposed it outside of content/.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove swapped out state from RenderFrameHost.

Since swapped out RenderFrameHosts are no longer used, its swapped out
state is no longer necessary. This CL removes it along with a public
API that exposed it outside of content/.

BUG=357747
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7ecc9a63ae68521e2189683fae86f5e89405986b
Cr-Commit-Position: refs/heads/master@{#381601}
==========

[commit-bot: I haz the power, 2016-03-17 00:24:23]

Patchset 4 (id:??) landed as
https://crrev.com/7ecc9a63ae68521e2189683fae86f5e89405986b
Cr-Commit-Position: refs/heads/master@{#381601}