Refactor sudden termination

Refactor sudden termination

This CL is based on the blink patch https://chromiumcodereview.appspot.com/861773003/ and refactors the sudden termination API so that the presence of BeforeUnload and Unload handlers preventing it is accounted on a per-frame basis instead of a per-process basis.

BUG=365039
Committed: https://crrev.com/df1975e5b02f1d53b35d7fceaa14c081a6097a03
Cr-Commit-Position: refs/heads/master@{#314848}

[clamy, 2015-01-20 16:49:12]

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

[clamy, 2015-01-20 16:54:15]

@nasko, creis: PTAL. This is the chromium side version of the sudden termination
refactor.

I had to keep part of the old logic because of the DOM local storage, which we
apparently cannot track per-RenderFrame.

Also, I noticed that we did not really have much infrastructure for unit tests
of the RenderProcessHost. In particular the MockRenderProcessHost is not
inheriting from RenderProcessHostImpl, so I could not write a unit test for the
logic in RenderProcessHostImpl. Is there a way to do it, apart from resorting to
the browser test?

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/r...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/r...
content/browser/renderer_host/render_process_host_impl.h:429: bool
sudden_termination_enabled_;
After discussing with michaeln@ it seems we cannot completely switch to tracking
the sudden termination status per RenderFrame, and still have to disable it per
process for the DOM local storage. I'll update the design doc accordingly.

[Charlie Reis, 2015-01-23 06:01:33]

Hmm, I think we can simplify this in some important ways, although it might
require an update to your new logic in Blink.

I left several comments as I went, but I think the key points are:
1) Blink should just be telling us about changes from zero to non-zero handlers.
2) RPH shouldn't need to track the number of frames.

The goal is to make this as simple as possible, minimizing future maintenance
for it.

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:165:
GetProcess()->SuddenTerminationChangedForFrame(true);
I don't think RPH should be tracking this state.  See my comment in RPHI.

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:344:
IPC_MESSAGE_HANDLER(FrameHostMsg_BeforeUnloadHandlersPresenceChanged,
nit: Put these just before OnBeforeUnloadACK, since they're conceptually
related.

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1675: void
RenderFrameHostImpl::HandleSuddenTerminationDisablerPresenceChange(
Hmm, this method seems awkward to me.
 - The method name is a bit unwieldy.
 - Taking in a pointer to a bool to modify seems unnecessarily risky.
 - The parameter names are not intuitive.

Can we find a better way to handle this?  It's small enough that I'd be ok
duplicating the code in the two call sites instead, rather than making the
reader try to follow this.

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:255: // Whether sudden
termination is allowed for this frame.
Briefly mention what conditions determine this.

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:258: // Called by the
browser to override or not the sudden termination status of
nit: (or not)

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:260: // some disablers are
present for the frame.
"disablers" isn't a well-defined term in the code.  Maybe:

even if it would otherwise be prevented.

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:482: void
OnUnloadHandlersPresenceChanged(bool present);
These names are a bit unwieldy.  Would On{Before}UnloadHandlersPresent(bool
present) work?  Same for FrameHostMsg_{Before}UnloadHandlersPresent.

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:648: // waiting for the
execution of BeforeUnload and Unload if they are false. The
...to avoid asking the renderer process to run BeforeUnload or Unload during
navigation.

https://codereview.chromium.org/857213003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:653: bool
has_beforeunload_handlers_;
nit: Please put these near the other *unload members (around 607).

https://codereview.chromium.org/857213003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/857213003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:429: bool
sudden_termination_enabled_;
On 2015/01/20 16:54:15, clamy wrote:
> After discussing with michaeln@ it seems we cannot completely switch to
tracking
> the sudden termination status per RenderFrame, and still have to disable it
per
> process for the DOM local storage. I'll update the design doc accordingly.

That's fine, but I don't understand why we're adding a count of frames here. 
RPH should just track the presence of DOM storage callbacks, and
RFH::SuddenTerminationAllowed() can include a call to
GetProcess()->SuddenTerminationAllowed(), as we used to do in RVH.

Keeping a count in this class feels overly complex and bug-prone to me, after
fighting a long string of bugs with a similar counter for DidStartLoading (see
https://crbug.com/429399).

https://codereview.chromium.org/857213003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/857213003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1104:
!static_cast<RenderFrameHostImpl*>(
We shouldn't need this cast.  Just return RenderFrameHostImpl from
WebContentsImpl::GetMainFrame (will work with covariant return types).

https://codereview.chromium.org/857213003/diff/20001/content/public/browser/r...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/857213003/diff/20001/content/public/browser/r...
content/public/browser/render_process_host.h:183: virtual void
SuddenTerminationChangedForFrame(bool allowed) = 0;
This doesn't belong in the public API (nor did SetSuddenTerminationAllowed,
which was only called within RPHI and was unnecessarily overidden in MockRPH).

https://codereview.chromium.org/857213003/diff/20001/content/public/browser/r...
content/public/browser/render_process_host.h:184: // Returns true if the process
can be abnormally terminated.
abnormally terminated -> immediately terminated

https://codereview.chromium.org/857213003/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/857213003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:480: // Returns true if the disablers
tracked by counter go from being absent to
nit: |counter|

https://codereview.chromium.org/857213003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3555: void
RenderFrameImpl::suddenTerminationDisablerChanged(
This seems unnecessarily complex.  Why do we need to track counters in
RenderFrameImpl?  We could avoid storing any state here (and the complex
HandleSuddenTerminationDisablerChange method) if Blink did that work for us.

In other words, just have Blink tell WebFrameClient if it goes between zero and
non-zero handlers for beforeunload or unload.  Should be easy, since it's just
checking set.isEmpty() before and after.

Then RenderFrameImpl just passes that message on to RFH without any extra logic
needed.

https://codereview.chromium.org/857213003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3581: routing_id_,
beforeunload_handlers_));
This is a count, not a bool.  Prefix with !! to make the intention clear (and it
might be necessary for compiling on Windows anyway).

[clamy, 2015-01-26 16:38:40]

Thanks! I modifed the logic in blink in
https://chromiumcodereview.appspot.com/874283003/, and that part is much clearer
now. I did as requested for RenderProcessHostImpl, however I'm a little worried
that we may run into cases where we tell the process to shutdown while still
having handlers to run, and RPHI would not know about them (since the
information about that is kept in RenderFrameHostImpl).

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:165:
GetProcess()->SuddenTerminationChangedForFrame(true);
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> I don't think RPH should be tracking this state.  See my comment in RPHI.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:344:
IPC_MESSAGE_HANDLER(FrameHostMsg_BeforeUnloadHandlersPresenceChanged,
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> nit: Put these just before OnBeforeUnloadACK, since they're conceptually
> related.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:1675: void
RenderFrameHostImpl::HandleSuddenTerminationDisablerPresenceChange(
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> Hmm, this method seems awkward to me.
>  - The method name is a bit unwieldy.
>  - Taking in a pointer to a bool to modify seems unnecessarily risky.
>  - The parameter names are not intuitive.
> 
> Can we find a better way to handle this?  It's small enough that I'd be ok
> duplicating the code in the two call sites instead, rather than making the
> reader try to follow this.

Done. If we don't signal the RPHI then only the boolean values need to be
updated.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:255: // Whether sudden
termination is allowed for this frame.
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> Briefly mention what conditions determine this.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:258: // Called by the
browser to override or not the sudden termination status of
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> nit: (or not)

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:260: // some disablers are
present for the frame.
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> "disablers" isn't a well-defined term in the code.  Maybe:
> 
> even if it would otherwise be prevented.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:482: void
OnUnloadHandlersPresenceChanged(bool present);
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> These names are a bit unwieldy.  Would On{Before}UnloadHandlersPresent(bool
> present) work?  Same for FrameHostMsg_{Before}UnloadHandlersPresent.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:648: // waiting for the
execution of BeforeUnload and Unload if they are false. The
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> ...to avoid asking the renderer process to run BeforeUnload or Unload during
> navigation.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:653: bool
has_beforeunload_handlers_;
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> nit: Please put these near the other *unload members (around 607).

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/r...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/r...
content/browser/renderer_host/render_process_host_impl.h:429: bool
sudden_termination_enabled_;
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> On 2015/01/20 16:54:15, clamy wrote:
> > After discussing with michaeln@ it seems we cannot completely switch to
> tracking
> > the sudden termination status per RenderFrame, and still have to disable it
> per
> > process for the DOM local storage. I'll update the design doc accordingly.
> 
> That's fine, but I don't understand why we're adding a count of frames here. 
> RPH should just track the presence of DOM storage callbacks, and
> RFH::SuddenTerminationAllowed() can include a call to
> GetProcess()->SuddenTerminationAllowed(), as we used to do in RVH.
> 
> Keeping a count in this class feels overly complex and bug-prone to me, after
> fighting a long string of bugs with a similar counter for DidStartLoading (see
> https://crbug.com/429399).

I removed the count of frames that have sudden termination disallowed. My main
concern is that the RenderProcessHostImpl may attempt to shutdown fastly even
though there are some handlers to run (in FastShutdownIfPossible
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...).
Is there a handy way for the RPHI to access all its associated RFH (and then
check their sudden termination status)? I couldn't find one, which is why I
introduced the tracking in the first place.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:1104:
!static_cast<RenderFrameHostImpl*>(
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> We shouldn't need this cast.  Just return RenderFrameHostImpl from
> WebContentsImpl::GetMainFrame (will work with covariant return types).

WebContentsImpl::GetMainFrame returns a RenderFrameHost and is an implementation
of WebContents::GetMainFrame which is part of the public content API. I don't
think I can modify it to return a RenderFrameHostImpl (or am I not understanding
this comment?).

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/public/br...
File content/public/browser/render_process_host.h (right):

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/public/br...
content/public/browser/render_process_host.h:183: virtual void
SuddenTerminationChangedForFrame(bool allowed) = 0;
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> This doesn't belong in the public API (nor did SetSuddenTerminationAllowed,
> which was only called within RPHI and was unnecessarily overidden in MockRPH).

The problem with not having these functions in the public API is that calling
them will require a cast to RenderProcessHostImpl in the middle of the code.
Since MockProcessHost does not inherit from RenderProcessHostImpl, this will
cause breakages in unit tests.

Given that this particular one was removed, that solves the problem though.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/public/br...
content/public/browser/render_process_host.h:184: // Returns true if the process
can be abnormally terminated.
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> abnormally terminated -> immediately terminated

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/renderer/...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/renderer/...
content/renderer/render_frame_impl.cc:480: // Returns true if the disablers
tracked by counter go from being absent to
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> nit: |counter|

Removed this function which was no longer needed.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/renderer/...
content/renderer/render_frame_impl.cc:3555: void
RenderFrameImpl::suddenTerminationDisablerChanged(
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> This seems unnecessarily complex.  Why do we need to track counters in
> RenderFrameImpl?  We could avoid storing any state here (and the complex
> HandleSuddenTerminationDisablerChange method) if Blink did that work for us.
> 
> In other words, just have Blink tell WebFrameClient if it goes between zero
and
> non-zero handlers for beforeunload or unload.  Should be easy, since it's just
> checking set.isEmpty() before and after.
> 
> Then RenderFrameImpl just passes that message on to RFH without any extra
logic
> needed.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/20001/content/renderer/...
content/renderer/render_frame_impl.cc:3581: routing_id_,
beforeunload_handlers_));
On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> This is a count, not a bool.  Prefix with !! to make the intention clear (and
it
> might be necessary for compiling on Windows anyway).

Switched to booleans.

[Charlie Reis, 2015-01-29 23:20:42]

Great!  Now that I'm back from the summit I had some more time to look closely
at this.  It's getting better, but I think there's a few things we'll need to
update.

(Also, I'm guessing that you plan to actually use the new frame-specific state
for skipping beforeunload/unload in a separate CL.  That sounds perfect to me.)

The main things below are about the state in RPH and the change in behavior to
NeedToFireBeforeUnload.

https://codereview.chromium.org/857213003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/857213003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:429: bool
sudden_termination_enabled_;
On 2015/01/26 16:38:40, clamy wrote:
> On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> > On 2015/01/20 16:54:15, clamy wrote:
> > > After discussing with michaeln@ it seems we cannot completely switch to
> > tracking
> > > the sudden termination status per RenderFrame, and still have to disable
it
> > per
> > > process for the DOM local storage. I'll update the design doc accordingly.
> > 
> > That's fine, but I don't understand why we're adding a count of frames here.

> > RPH should just track the presence of DOM storage callbacks, and
> > RFH::SuddenTerminationAllowed() can include a call to
> > GetProcess()->SuddenTerminationAllowed(), as we used to do in RVH.
> > 
> > Keeping a count in this class feels overly complex and bug-prone to me,
after
> > fighting a long string of bugs with a similar counter for DidStartLoading
(see
> > https://crbug.com/429399).
> 
> I removed the count of frames that have sudden termination disallowed. My main
> concern is that the RenderProcessHostImpl may attempt to shutdown fastly even
> though there are some handlers to run (in FastShutdownIfPossible
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...).
> Is there a handy way for the RPHI to access all its associated RFH (and then
> check their sudden termination status)? I couldn't find one, which is why I
> introduced the tracking in the first place.

I think my comments in WebContentsImpl::NeedToFireBeforeUnload and
RenderProcessHostImpl::FastShutdownIfPossible might resolve this.

At a high level:
RPH::SuddenTerminationAllowed() already returns true if any frame has either
type of handler, or if there's a DOM storage callback.  It shouldn't need to ask
the frames individually.

https://codereview.chromium.org/857213003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/857213003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1104:
!static_cast<RenderFrameHostImpl*>(
On 2015/01/26 16:38:40, clamy wrote:
> On 2015/01/23 06:01:32, Charlie Reis (slow until 1-29) wrote:
> > We shouldn't need this cast.  Just return RenderFrameHostImpl from
> > WebContentsImpl::GetMainFrame (will work with covariant return types).
> 
> WebContentsImpl::GetMainFrame returns a RenderFrameHost and is an
implementation
> of WebContents::GetMainFrame which is part of the public content API. I don't
> think I can modify it to return a RenderFrameHostImpl (or am I not
understanding
> this comment?).

You can make WebContentsImpl::GetMainFrame return RenderFrameHostImpl.  That
still matches the contract from WebContents::GetMainFrame, because all
RenderFrameHostImpls are RenderFrameHosts.  See
WebContentsImpl::GetSiteInstance() for a similar example.

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

https://codereview.chromium.org/857213003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:275: // would otherwise be
prevented.
nit: it would

https://codereview.chromium.org/857213003/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:540: void
HandleSuddenTerminationDisablerPresenceChange(bool* disabler_presence,
Don't forget to remove this, now that the implementation is gone.  :)

https://codereview.chromium.org/857213003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/857213003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1429: if
(!SuddenTerminationAllowed())
This confused me at first, but I think I follow it now.

This is the only call of RPH::SuddenTerminationAllowed.  At first I thought it
only tracked the DOM storage callback status, but it actually tracks whether
*any* reason is present (beforeunload, unload, DOM storage callback).  The count
is in RendererBlinkPlatformImpl.

This means that RPHI always has a global view of the conditions for the process.
 sudden_termination_allowed_ is true if there are any reasons not to exit (in
any frame or otherwise), and false otherwise.

Thus, it does not need to ask frames individually.

https://codereview.chromium.org/857213003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/857213003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:414: // Whether the
sudden termination of the process was enabled on a process
was enabled on -> is allowed at

https://codereview.chromium.org/857213003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:415: // level. This is
independant from the status of the various RenderFrames in
independent

https://codereview.chromium.org/857213003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:419: // process cannot
be suddenly terminated.
I don't think this comment is accurate, if I'm following things correctly.  This
flag tracks *all* of the beforeunload, unload, and non-frame-specific (i.e., DOM
storage callback) cases.

See RendererBlinkPlatformImpl::suddenTerminationChanged, and the fact that
LocalDOMWindow::enableSuddenTermination calls it.  That means that the presence
of beforeunload handlers is tracked both in RFH and RPH.

We should make that clear here.

https://codereview.chromium.org/857213003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:420: bool
sudden_termination_enabled_;
Let's stick with allowed rather than enabled, since they're basically equivalent
and we don't need the churn.

https://codereview.chromium.org/857213003/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/857213003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1100: bool
WebContentsImpl::NeedToFireBeforeUnload() {
I think I'm missing part of the picture of how this used to work, because this
method seems strange to me.  It's the only place we call
RVH::SuddenTerminationAllowed, but it's called in many places.

Things I've noticed:
1) The name is confusing, since it's about beforeunload + unload + other cases. 
No need to change that in this CL, but it's worth cleaning up next.

2) It's documented as returning true if there's a beforeunload handler but false
after the handler is run (even if it's still present).  And indeed, it looks
like RVH makes that happen using the various call sites of
ClosePageIgnoringUnloadEvents.  We're changing that behavior, which could be a
problem.  (I imagine it means we'd take the slow path even if the handler has
already been run.)

https://codereview.chromium.org/857213003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1105:
GetMainFrame())->SuddenTerminationAllowed();
This doesn't seem quite right to me.  What if the main frame doesn't have a
beforeunload handler but a subframe does?

I know we didn't handle all subframe cases correctly in the past, but I think
that simple case worked (since the subframe's beforeunload handler was tracked
on RVH).  It seems like we should be checking all frames here (using
ForEachFrame), and probably the process as well.

https://codereview.chromium.org/857213003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:4038:
rfhi->set_override_sudden_termination_status(true);
Again, this is only the main frame.  We'll eventually need to move this method
to work with RenderFrameHosts, but for now we should probably notify all RFHs
corresponding to render_view_host.

https://codereview.chromium.org/857213003/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/857213003/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3576: beforeunload_handlers_present_ =
present;
We don't need these members anymore, do we?  We're just sending |present| to the
browser any time we hear it from Blink now.

[clamy, 2015-02-02 16:11:09]

Patchset #4 (id:60001) has been deleted

[clamy, 2015-02-02 16:16:28]

Thanks! I think the main remaining thing to consider is whether we want to
remove the calls to RendererBlinkPlatformImpl::enableSuddenTermination in
LocalDOMWindow or not. That makes the code more complex in the browser side, but
then we don't duplicate the sudden termination state for Unload/beforeUnload
handlers.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:275: // would otherwise be
prevented.
On 2015/01/29 23:20:41, Charlie Reis wrote:
> nit: it would

Done.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:540: void
HandleSuddenTerminationDisablerPresenceChange(bool* disabler_presence,
On 2015/01/29 23:20:41, Charlie Reis wrote:
> Don't forget to remove this, now that the implementation is gone.  :)

Done.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/r...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/r...
content/browser/renderer_host/render_process_host_impl.cc:1429: if
(!SuddenTerminationAllowed())
On 2015/01/29 23:20:41, Charlie Reis wrote:
> This confused me at first, but I think I follow it now.
> 
> This is the only call of RPH::SuddenTerminationAllowed.  At first I thought it
> only tracked the DOM storage callback status, but it actually tracks whether
> *any* reason is present (beforeunload, unload, DOM storage callback).  The
count
> is in RendererBlinkPlatformImpl.
> 
> This means that RPHI always has a global view of the conditions for the
process.
>  sudden_termination_allowed_ is true if there are any reasons not to exit (in
> any frame or otherwise), and false otherwise.
> 
> Thus, it does not need to ask frames individually.

As I explained in the other comment, I was planing to remove the calls to
RendererBlinkPlatformImpl::enableSuddenTermination in LocalDOMWindow in order to
not duplicate the tracking of BeforeUnload/Unload handlers (but that has to go
in another blink patch after this one lands). In that case RPHI does not know
about unload and before unload handlers and needs to ask the frames
individually.

Now we may decide that this is too complex, or that tracking the handlers twice
is fine.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/r...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/r...
content/browser/renderer_host/render_process_host_impl.h:414: // Whether the
sudden termination of the process was enabled on a process
On 2015/01/29 23:20:41, Charlie Reis wrote:
> was enabled on -> is allowed at

Done.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/r...
content/browser/renderer_host/render_process_host_impl.h:415: // level. This is
independant from the status of the various RenderFrames in
On 2015/01/29 23:20:41, Charlie Reis wrote:
> independent

Done.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/r...
content/browser/renderer_host/render_process_host_impl.h:419: // process cannot
be suddenly terminated.
On 2015/01/29 23:20:41, Charlie Reis wrote:
> I don't think this comment is accurate, if I'm following things correctly. 
This
> flag tracks *all* of the beforeunload, unload, and non-frame-specific (i.e.,
DOM
> storage callback) cases.
> 
> See RendererBlinkPlatformImpl::suddenTerminationChanged, and the fact that
> LocalDOMWindow::enableSuddenTermination calls it.  That means that the
presence
> of beforeunload handlers is tracked both in RFH and RPH.
> 
> We should make that clear here.

My plan was to get rid of the calls to
RendererBlinkPlatformImpl::suddenTerminationChanged, in order not to have the
same thing tracked twice in RFH and RPH. If it turns out it is not possible I'll
update the comment.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/r...
content/browser/renderer_host/render_process_host_impl.h:420: bool
sudden_termination_enabled_;
On 2015/01/29 23:20:41, Charlie Reis wrote:
> Let's stick with allowed rather than enabled, since they're basically
equivalent
> and we don't need the churn.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:1100: bool
WebContentsImpl::NeedToFireBeforeUnload() {
On 2015/01/29 23:20:41, Charlie Reis wrote:
> I think I'm missing part of the picture of how this used to work, because this
> method seems strange to me.  It's the only place we call
> RVH::SuddenTerminationAllowed, but it's called in many places.
> 
> Things I've noticed:
> 1) The name is confusing, since it's about beforeunload + unload + other
cases. 
> No need to change that in this CL, but it's worth cleaning up next.
> 
> 2) It's documented as returning true if there's a beforeunload handler but
false
> after the handler is run (even if it's still present).  And indeed, it looks
> like RVH makes that happen using the various call sites of
> ClosePageIgnoringUnloadEvents.  We're changing that behavior, which could be a
> problem.  (I imagine it means we'd take the slow path even if the handler has
> already been run.)

To match what we were doing in 2, I now have
RenderViewHostImpl::ClosePageIgnoringUnloadEvents enable sudden termination for
all RFH in the tree. This should make sure that we don't take the slow path if
we don't need to.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:1105:
GetMainFrame())->SuddenTerminationAllowed();
On 2015/01/29 23:20:41, Charlie Reis wrote:
> This doesn't seem quite right to me.  What if the main frame doesn't have a
> beforeunload handler but a subframe does?
> 
> I know we didn't handle all subframe cases correctly in the past, but I think
> that simple case worked (since the subframe's beforeunload handler was tracked
> on RVH).  It seems like we should be checking all frames here (using
> ForEachFrame), and probably the process as well.

RenderFrameHostImpl::DispatchBeforeUnload skips executing the BeforeUnload event
for subframes (though there is a TODO there saying that we should). That said,
since this function is actually checking for more than BeforeUnload, I now check
all RFH in the FrameTree.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:4038:
rfhi->set_override_sudden_termination_status(true);
On 2015/01/29 23:20:42, Charlie Reis wrote:
> Again, this is only the main frame.  We'll eventually need to move this method
> to work with RenderFrameHosts, but for now we should probably notify all RFHs
> corresponding to render_view_host.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/renderer/...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/renderer/...
content/renderer/render_frame_impl.cc:3576: beforeunload_handlers_present_ =
present;
On 2015/01/29 23:20:42, Charlie Reis wrote:
> We don't need these members anymore, do we?  We're just sending |present| to
the
> browser any time we hear it from Blink now.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/80001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/80001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:208: bool
EnableSuddenTermination(RenderViewHost* render_view_host,
Another possibility would be to provide two functions in FrameTree, 1 to enable
sudden termination for all frames associated with render_view_host, and one to
check if sudden termiation is allowed for all frames associated with
render_view_host. That would hide the ForEach call down below and in
RenderViewHost.

[Charlie Reis, 2015-02-02 20:14:50]

Thanks.  I think we should stick with the existing RendererBlinkPlatformImpl
behavior, as explained below.

I've also recommended using SiteInstance rather than RenderViewHost, since a RFH
and its RVH necessarily have the same SiteInstance.

[Side note: These patches would be much easier for me to review incrementally if
you wouldn't mind uploading the rebase separately from the changes.  Thanks!]

https://codereview.chromium.org/857213003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/857213003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1429: if
(!SuddenTerminationAllowed())
On 2015/02/02 16:16:27, clamy wrote:
> On 2015/01/29 23:20:41, Charlie Reis wrote:
> > This confused me at first, but I think I follow it now.
> > 
> > This is the only call of RPH::SuddenTerminationAllowed.  At first I thought
it
> > only tracked the DOM storage callback status, but it actually tracks whether
> > *any* reason is present (beforeunload, unload, DOM storage callback).  The
> count
> > is in RendererBlinkPlatformImpl.
> > 
> > This means that RPHI always has a global view of the conditions for the
> process.
> >  sudden_termination_allowed_ is true if there are any reasons not to exit
(in
> > any frame or otherwise), and false otherwise.
> > 
> > Thus, it does not need to ask frames individually.
> 
> As I explained in the other comment, I was planing to remove the calls to
> RendererBlinkPlatformImpl::enableSuddenTermination in LocalDOMWindow in order
to
> not duplicate the tracking of BeforeUnload/Unload handlers (but that has to go
> in another blink patch after this one lands). In that case RPHI does not know
> about unload and before unload handlers and needs to ask the frames
> individually.
> 
> Now we may decide that this is too complex, or that tracking the handlers
twice
> is fine.

I think tracking the handlers twice is fine.  It's a smaller delta from what we
have today and it avoids a reason for RPHI to know about and loop through all
frames in the process.  Given that these handlers are rare, I'd rather notify
both RFH and RPHI when they're present than always require RPHI to iterate when
trying to exit.

https://codereview.chromium.org/857213003/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/857213003/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:131: bool
EnableSuddenTermination(RenderViewHost* render_view_host,
Let's pass the SiteInstance here instead of the RenderViewHost.  That will make
it easier to move out of RVH with the rest of the close logic.

https://codereview.chromium.org/857213003/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/857213003/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:208: bool
EnableSuddenTermination(RenderViewHost* render_view_host,
On 2015/02/02 16:16:28, clamy wrote:
> Another possibility would be to provide two functions in FrameTree, 1 to
enable
> sudden termination for all frames associated with render_view_host, and one to
> check if sudden termiation is allowed for all frames associated with
> render_view_host. That would hide the ForEach call down below and in
> RenderViewHost.

What you have is fine.  We probably shouldn't add sudden termination knowledge
to FrameTree.

https://codereview.chromium.org/857213003/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:211: == render_view_host) {
Let's pass in the SiteInstance rather than the RVH.  That's equivalent and
avoids adding a new dependency on RVH.

https://codereview.chromium.org/857213003/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:223: RenderViewHost*
render_view_host,
Same: Use SiteInstance here.

[clamy, 2015-02-03 12:54:03]

Thanks!

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/r...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/40001/content/browser/r...
content/browser/renderer_host/render_process_host_impl.cc:1429: if
(!SuddenTerminationAllowed())
On 2015/02/02 20:14:50, Charlie Reis wrote:
> On 2015/02/02 16:16:27, clamy wrote:
> > On 2015/01/29 23:20:41, Charlie Reis wrote:
> > > This confused me at first, but I think I follow it now.
> > > 
> > > This is the only call of RPH::SuddenTerminationAllowed.  At first I
thought
> it
> > > only tracked the DOM storage callback status, but it actually tracks
whether
> > > *any* reason is present (beforeunload, unload, DOM storage callback).  The
> > count
> > > is in RendererBlinkPlatformImpl.
> > > 
> > > This means that RPHI always has a global view of the conditions for the
> > process.
> > >  sudden_termination_allowed_ is true if there are any reasons not to exit
> (in
> > > any frame or otherwise), and false otherwise.
> > > 
> > > Thus, it does not need to ask frames individually.
> > 
> > As I explained in the other comment, I was planing to remove the calls to
> > RendererBlinkPlatformImpl::enableSuddenTermination in LocalDOMWindow in
order
> to
> > not duplicate the tracking of BeforeUnload/Unload handlers (but that has to
go
> > in another blink patch after this one lands). In that case RPHI does not
know
> > about unload and before unload handlers and needs to ask the frames
> > individually.
> > 
> > Now we may decide that this is too complex, or that tracking the handlers
> twice
> > is fine.
> 
> I think tracking the handlers twice is fine.  It's a smaller delta from what
we
> have today and it avoids a reason for RPHI to know about and loop through all
> frames in the process.  Given that these handlers are rare, I'd rather notify
> both RFH and RPHI when they're present than always require RPHI to iterate
when
> trying to exit.

Acknowledged.

https://chromiumcodereview.appspot.com/857213003/diff/80001/content/browser/r...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/80001/content/browser/r...
content/browser/renderer_host/render_view_host_impl.cc:131: bool
EnableSuddenTermination(RenderViewHost* render_view_host,
On 2015/02/02 20:14:50, Charlie Reis wrote:
> Let's pass the SiteInstance here instead of the RenderViewHost.  That will
make
> it easier to move out of RVH with the rest of the close logic.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/80001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/80001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:208: bool
EnableSuddenTermination(RenderViewHost* render_view_host,
On 2015/02/02 20:14:50, Charlie Reis wrote:
> On 2015/02/02 16:16:28, clamy wrote:
> > Another possibility would be to provide two functions in FrameTree, 1 to
> enable
> > sudden termination for all frames associated with render_view_host, and one
to
> > check if sudden termiation is allowed for all frames associated with
> > render_view_host. That would hide the ForEach call down below and in
> > RenderViewHost.
> 
> What you have is fine.  We probably shouldn't add sudden termination knowledge
> to FrameTree.

Acknowledged (that's why I did not do it in the first place).

https://chromiumcodereview.appspot.com/857213003/diff/80001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:211: == render_view_host) {
On 2015/02/02 20:14:50, Charlie Reis wrote:
> Let's pass in the SiteInstance rather than the RVH.  That's equivalent and
> avoids adding a new dependency on RVH.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/80001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:223: RenderViewHost*
render_view_host,
On 2015/02/02 20:14:50, Charlie Reis wrote:
> Same: Use SiteInstance here.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/120001/content/browser/...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/120001/content/browser/...
content/browser/renderer_host/render_view_host_impl.cc:131: bool
EnableSuddenTermination(scoped_refptr<SiteInstance> site_instance,
Note that since SiteInstance is refcounted, we need to have a scoped_refptr as
an argument of the function to be able to bind it.

[Charlie Reis, 2015-02-03 19:33:16]

Thanks!  LGTM if you make the changes below.

https://codereview.chromium.org/857213003/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/857213003/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:418: // level. This is
independent from the status of the various RenderFrames in
Please remember to update this comment to reflect the behavior of
RendererBlinkPlatformImpl.

https://codereview.chromium.org/857213003/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/857213003/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:131: bool
EnableSuddenTermination(scoped_refptr<SiteInstance> site_instance,
On 2015/02/03 12:54:03, clamy wrote:
> Note that since SiteInstance is refcounted, we need to have a scoped_refptr as
> an argument of the function to be able to bind it.

I don't think that's true, is it?  It only needs to be a scoped_refptr in cases
that ownership is involved.

There's no chance that the SiteInstance will be deleted in this function, since
it's purely used for comparison and there aren't any actions here that could
cause it to go away.

We should just pass a raw pointer to it from the call sites.

https://codereview.chromium.org/857213003/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:573:
frame_tree->ForEach(base::Bind(&EnableSuddenTermination, instance_));
instance_.get()

https://codereview.chromium.org/857213003/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/857213003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:208: bool
EnableSuddenTermination(scoped_refptr<SiteInstance> site_instance,
Again, just a raw pointer is fine here.

https://codereview.chromium.org/857213003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4099:
scoped_refptr<SiteInstance>(rvhi->GetSiteInstance())));
No scoped_refptr.

[clamy, 2015-02-04 12:51:04]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[clamy, 2015-02-04 13:16:19]

Thanks! I could not use a raw pointer to SiteInstance because base::Bind has a
compile time assert preventing binding a raw pointer to a refcounted class, so I
am comparing SiteInstance ids instead.

@nasko: PTAL at the changes in frame_messages.h

https://chromiumcodereview.appspot.com/857213003/diff/120001/content/browser/...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://chromiumcodereview.appspot.com/857213003/diff/120001/content/browser/...
content/browser/renderer_host/render_process_host_impl.h:418: // level. This is
independent from the status of the various RenderFrames in
On 2015/02/03 19:33:15, Charlie Reis wrote:
> Please remember to update this comment to reflect the behavior of
> RendererBlinkPlatformImpl.

Done.

https://chromiumcodereview.appspot.com/857213003/diff/120001/content/browser/...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/120001/content/browser/...
content/browser/renderer_host/render_view_host_impl.cc:131: bool
EnableSuddenTermination(scoped_refptr<SiteInstance> site_instance,
On 2015/02/03 19:33:15, Charlie Reis wrote:
> On 2015/02/03 12:54:03, clamy wrote:
> > Note that since SiteInstance is refcounted, we need to have a scoped_refptr
as
> > an argument of the function to be able to bind it.
> 
> I don't think that's true, is it?  It only needs to be a scoped_refptr in
cases
> that ownership is involved.
> 
> There's no chance that the SiteInstance will be deleted in this function,
since
> it's purely used for comparison and there aren't any actions here that could
> cause it to go away.
> 
> We should just pass a raw pointer to it from the call sites.

There is a static assert in base::Bind preventing the code to compile if trying
to bind a raw pointer of a ref_counted class
(https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&l=100&...).
I agree that the scoped_refptr makes no sense from an ownership point of view (I
had written the raw pointer version before hitting the compile assert). It seems
that SiteInstance has a unique ID, so I went with comparing that instead.
Another option would be to have SiteInstance support weak pointers and bind a
weak pointer but I don't think that's desirable.

https://chromiumcodereview.appspot.com/857213003/diff/120001/content/browser/...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/857213003/diff/120001/content/browser/...
content/browser/web_contents/web_contents_impl.cc:208: bool
EnableSuddenTermination(scoped_refptr<SiteInstance> site_instance,
On 2015/02/03 19:33:15, Charlie Reis wrote:
> Again, just a raw pointer is fine here.

As explained in the other file, I went with using the SiteInstance ID here.

[Charlie Reis, 2015-02-04 21:34:58]

I just noticed a bug in WebContents's EnableSuddenTermination, so let's fix that
before landing.  (A few other nits as well.)

https://codereview.chromium.org/857213003/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/857213003/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:131: bool
EnableSuddenTermination(scoped_refptr<SiteInstance> site_instance,
On 2015/02/04 13:16:19, clamy wrote:
> On 2015/02/03 19:33:15, Charlie Reis wrote:
> > On 2015/02/03 12:54:03, clamy wrote:
> > > Note that since SiteInstance is refcounted, we need to have a
scoped_refptr
> as
> > > an argument of the function to be able to bind it.
> > 
> > I don't think that's true, is it?  It only needs to be a scoped_refptr in
> cases
> > that ownership is involved.
> > 
> > There's no chance that the SiteInstance will be deleted in this function,
> since
> > it's purely used for comparison and there aren't any actions here that could
> > cause it to go away.
> > 
> > We should just pass a raw pointer to it from the call sites.
> 
> There is a static assert in base::Bind preventing the code to compile if
trying
> to bind a raw pointer of a ref_counted class
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/bind.h&l=100&...).
> I agree that the scoped_refptr makes no sense from an ownership point of view
(I
> had written the raw pointer version before hitting the compile assert). It
seems
> that SiteInstance has a unique ID, so I went with comparing that instead.
> Another option would be to have SiteInstance support weak pointers and bind a
> weak pointer but I don't think that's desirable.

Ah, base::Bind...  Sorry for missing that aspect before.

Yes, using the SiteInstance ID seems like a reasonable workaround here.

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:418: // level. This is
independent from the status of the various RenderFrames in
I don't think "This is independent" is very clear, since the status of the
RenderFrames is a subset of the conditions determining this flag.

The old comment seems clearer to me, once we change "RenderViews" and
"WebContents" to "RenderFrames."

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:571: FrameTree*
frame_tree = static_cast<RenderFrameHostImpl*>(GetMainFrame())
nit: I think we can skip this cast as well by making
RenderViewHostImpl::GetMainFrame return a RenderFrameHostImpl.  (Just use
RenderFrameHostImpl::FromID in the body instead of RenderFrameHost::FromID.)

https://codereview.chromium.org/857213003/diff/160001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/857213003/diff/160001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:220: // |frame_tree_node| (if
the ID of its SiteInstance is |site_instance_id|). Used
The SiteInstance comparison here seems broken to me.  See my comment below.

https://codereview.chromium.org/857213003/diff/160001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1147:
GetRenderViewHost()->GetSiteInstance()->GetId()));
Wait, I'm confused about this one again.  Why are we only looking at frames that
are in the same SiteInstance as the main frame?  That skips any OOPIFs that have
beforeunload handlers.

If this is because we don't have logic to run beforeunload in OOPIFs yet, then
this needs a TODO (similar to RFHI::DispatchBeforeUnload).

But I don't really see what it's buying us-- in default Chrome, all frames will
have the same SiteInstance ID, and in --site-per-process, it's fine to return
true here if an OOPIF has a beforeunload handler, even if we don't know how to
run it yet.

It seems to me that this should not pass SiteInstance ID.

https://codereview.chromium.org/857213003/diff/160001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4098:
&EnableSuddenTermination, rvhi->GetSiteInstance()->GetId()));
(It does make sense to pass SiteInstance ID here, because it's only one
RenderViewHost / process that has become unresponsive.)

[clamy, 2015-02-05 16:23:13]

Thanks!

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:418: // level. This is
independent from the status of the various RenderFrames in
On 2015/02/04 21:34:58, Charlie Reis wrote:
> I don't think "This is independent" is very clear, since the status of the
> RenderFrames is a subset of the conditions determining this flag.
> 
> The old comment seems clearer to me, once we change "RenderViews" and
> "WebContents" to "RenderFrames."

Done.

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:571: FrameTree*
frame_tree = static_cast<RenderFrameHostImpl*>(GetMainFrame())
On 2015/02/04 21:34:58, Charlie Reis wrote:
> nit: I think we can skip this cast as well by making
> RenderViewHostImpl::GetMainFrame return a RenderFrameHostImpl.  (Just use
> RenderFrameHostImpl::FromID in the body instead of RenderFrameHost::FromID.)

It is not possible because to have GetMainFrame return a RenderFrameHostImpl and
still be recognized a proper override a forward declaration of
RenderFrameHostImpl is not enough, and the header for RenderFrameHostImpl has to
be included in RenderViewHostImpl header file. This is explicitly against one of
the DEPS rule of content/browser/renderer_host/.

https://codereview.chromium.org/857213003/diff/160001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/857213003/diff/160001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:220: // |frame_tree_node| (if
the ID of its SiteInstance is |site_instance_id|). Used
On 2015/02/04 21:34:58, Charlie Reis wrote:
> The SiteInstance comparison here seems broken to me.  See my comment below.

Done.

https://codereview.chromium.org/857213003/diff/160001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1147:
GetRenderViewHost()->GetSiteInstance()->GetId()));
On 2015/02/04 21:34:58, Charlie Reis wrote:
> Wait, I'm confused about this one again.  Why are we only looking at frames
that
> are in the same SiteInstance as the main frame?  That skips any OOPIFs that
have
> beforeunload handlers.
> 
> If this is because we don't have logic to run beforeunload in OOPIFs yet, then
> this needs a TODO (similar to RFHI::DispatchBeforeUnload).
> 
> But I don't really see what it's buying us-- in default Chrome, all frames
will
> have the same SiteInstance ID, and in --site-per-process, it's fine to return
> true here if an OOPIF has a beforeunload handler, even if we don't know how to
> run it yet.
> 
> It seems to me that this should not pass SiteInstance ID.

Indeed. We now check all subframes.

https://codereview.chromium.org/857213003/diff/160001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4098:
&EnableSuddenTermination, rvhi->GetSiteInstance()->GetId()));
On 2015/02/04 21:34:58, Charlie Reis wrote:
> (It does make sense to pass SiteInstance ID here, because it's only one
> RenderViewHost / process that has become unresponsive.)

Acknowledged.

[Charlie Reis, 2015-02-05 17:12:17]

Thanks!  LGTM with nit.

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/857213003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:571: FrameTree*
frame_tree = static_cast<RenderFrameHostImpl*>(GetMainFrame())
On 2015/02/05 16:23:13, clamy wrote:
> On 2015/02/04 21:34:58, Charlie Reis wrote:
> > nit: I think we can skip this cast as well by making
> > RenderViewHostImpl::GetMainFrame return a RenderFrameHostImpl.  (Just use
> > RenderFrameHostImpl::FromID in the body instead of RenderFrameHost::FromID.)
> 
> It is not possible because to have GetMainFrame return a RenderFrameHostImpl
and
> still be recognized a proper override a forward declaration of
> RenderFrameHostImpl is not enough, and the header for RenderFrameHostImpl has
to
> be included in RenderViewHostImpl header file. This is explicitly against one
of
> the DEPS rule of content/browser/renderer_host/.

Acknowledged.

https://codereview.chromium.org/857213003/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/857213003/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:422: // registerd but
another RenderFrame in the same process doesn't.
nit: registered

[nasko, 2015-02-05 17:38:02]

IPC LGTM

https://codereview.chromium.org/857213003/diff/200001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/857213003/diff/200001/content/common/frame_me...
content/common/frame_messages.h:860: // for a frame. |present| is true if there
is at least one of the concerned
nit: s/concerned//

[clamy, 2015-02-05 17:48:25]

New patchsets have been uploaded after l-g-t-m from
creis@chromium.org,nasko@chromium.org

[clamy, 2015-02-05 17:48:46]

Thanks!

https://chromiumcodereview.appspot.com/857213003/diff/200001/content/browser/...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://chromiumcodereview.appspot.com/857213003/diff/200001/content/browser/...
content/browser/renderer_host/render_process_host_impl.h:422: // registerd but
another RenderFrame in the same process doesn't.
On 2015/02/05 17:12:17, Charlie Reis wrote:
> nit: registered

Done.

https://chromiumcodereview.appspot.com/857213003/diff/200001/content/common/f...
File content/common/frame_messages.h (right):

https://chromiumcodereview.appspot.com/857213003/diff/200001/content/common/f...
content/common/frame_messages.h:860: // for a frame. |present| is true if there
is at least one of the concerned
On 2015/02/05 17:38:02, nasko wrote:
> nit: s/concerned//

Done.

[clamy, 2015-02-05 17:48:54]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2015-02-05 17:49:13]

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

[commit-bot: I haz the power, 2015-02-05 19:12:54]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-02-05 19:15:01]

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