A page can't fire dialogs in unload handlers; remove code that handles that case.

A page can't fire dialogs in unload handlers; remove code that handles that case.

BUG=669881
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2719133002
Cr-Commit-Position: refs/heads/master@{#453971}
Committed: https://chromium.googlesource.com/chromium/src/+/e4115eb7314d12ffa24ae0be0d192f2712693cf0

[Avi (use Gerrit), 2017-02-27 19:52:23]

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

[Avi (use Gerrit), 2017-02-27 19:52:24]

Description was changed from

==========
A page can't fire dialogs in unload handlers; remove code that handles that
case.

BUG=669881
==========

to

==========
A page can't fire dialogs in unload handlers; remove code that handles that
case.

BUG=669881
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-02-27 19:52:43]

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

[commit-bot: I haz the power, 2017-02-27 20:27:16]

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

[commit-bot: I haz the power, 2017-02-27 20:27:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-02-27 20:31:02]

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

[commit-bot: I haz the power, 2017-02-27 20:31:27]

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

[commit-bot: I haz the power, 2017-02-27 21:40:36]

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

[commit-bot: I haz the power, 2017-02-27 21:40:37]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-02-27 21:57:57]

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

[Avi (use Gerrit), 2017-02-27 21:57:58]

What do you think?

[Charlie Reis, 2017-02-27 22:26:18]

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

https://codereview.chromium.org/2719133002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1677:
DCHECK(!IsWaitingForUnloadACK());
Can you double check what happens when closing a tab?  I noticed that this can
return true for RVHI::is_waiting_for_close_ack_.  If that includes the
beforeunload event for closing a tab, then my suggestion might not be safe-- a
tab can show a beforeunload dialog while closing.  If that's the case, maybe it
would be less confusing if we kept track of close (beforeunload + unload) and
navigational-unload separately.

(Maybe we treat the beforeunload and unload behavior separately already for
close, but I don't think so.)

[Avi (use Gerrit), 2017-02-27 22:30:07]

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

[commit-bot: I haz the power, 2017-02-27 22:30:41]

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

[Avi (use Gerrit), 2017-02-27 22:38:59]

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

https://codereview.chromium.org/2719133002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1677:
DCHECK(!IsWaitingForUnloadACK());
On 2017/02/27 22:26:17, Charlie Reis (slow) wrote:
> Can you double check what happens when closing a tab?  I noticed that this can
> return true for RVHI::is_waiting_for_close_ack_.  If that includes the
> beforeunload event for closing a tab, then my suggestion might not be safe-- a
> tab can show a beforeunload dialog while closing.  If that's the case, maybe
it
> would be less confusing if we kept track of close (beforeunload + unload) and
> navigational-unload separately.
> 
> (Maybe we treat the beforeunload and unload behavior separately already for
> close, but I don't think so.)

But we already keep track of beforeunload and unload separately, right?

RenderFrameHostImpl has is_waiting_for_beforeunload_ack_, which is set in
RenderFrameHostImpl::DispatchBeforeUnload. RenderViewHostImpl has
is_waiting_for_close_ack_, which is set in RenderViewHostImpl::ClosePage. I
haven't fully traced those calls yet, but RenderViewHostImpl::ClosePage sends
ViewMsg_ClosePage which turns into RenderViewImpl::OnClosePage which dispatches
the unload event, much further along than the beforeunload event.

[Charlie Reis, 2017-02-27 22:41:14]

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

https://codereview.chromium.org/2719133002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1677:
DCHECK(!IsWaitingForUnloadACK());
On 2017/02/27 22:38:58, Avi wrote:
> On 2017/02/27 22:26:17, Charlie Reis (slow) wrote:
> > Can you double check what happens when closing a tab?  I noticed that this
can
> > return true for RVHI::is_waiting_for_close_ack_.  If that includes the
> > beforeunload event for closing a tab, then my suggestion might not be safe--
a
> > tab can show a beforeunload dialog while closing.  If that's the case, maybe
> it
> > would be less confusing if we kept track of close (beforeunload + unload)
and
> > navigational-unload separately.
> > 
> > (Maybe we treat the beforeunload and unload behavior separately already for
> > close, but I don't think so.)
> 
> But we already keep track of beforeunload and unload separately, right?
> 
> RenderFrameHostImpl has is_waiting_for_beforeunload_ack_, which is set in
> RenderFrameHostImpl::DispatchBeforeUnload. RenderViewHostImpl has
> is_waiting_for_close_ack_, which is set in RenderViewHostImpl::ClosePage. I
> haven't fully traced those calls yet, but RenderViewHostImpl::ClosePage sends
> ViewMsg_ClosePage which turns into RenderViewImpl::OnClosePage which
dispatches
> the unload event, much further along than the beforeunload event.

Does the browser process send a separate beforeunload IPC when closing a tab,
though, or does it just send ViewMsg_ClosePage to run both beforeunload and
unload?

I think we might only split the two events into separate IPCs for navigations,
but I'm not sure.

[commit-bot: I haz the power, 2017-02-27 23:55:45]

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

[commit-bot: I haz the power, 2017-02-27 23:55:45]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-02-28 02:14:41]

On 2017/02/27 22:41:14, Charlie Reis (slow) wrote:
> Does the browser process send a separate beforeunload IPC when closing a tab,
> though

As far as I can tell, yes.

On my Mac, if you try to close the tab or window, you get:

1   content::RenderFrameHostImpl::DispatchBeforeUnload(bool, bool) + 55
2   chrome::UnloadController::ShouldCloseWindow() + 100
3   -[BrowserWindowController windowShouldClose:] + 66
4   -[NSWindow __close]_block_invoke + 130
5  -[NSWindow __close] + 312

Every close path that I can find appears to call
chrome::UnloadController::ShouldCloseWindow(), which makes sense. And
UnloadController::ShouldCloseWindow calls
RenderFrameHostImpl::DispatchBeforeUnload which sends FrameMsg_BeforeUnload,
different from RenderViewHostImpl::ClosePage sending ViewMsg_ClosePage.

I would be very surprised to find ViewMsg_ClosePage being sent without a
FrameMsg_BeforeUnload first. That would definitely be a user-facing, web-visible
bug, since ViewMsg_ClosePage actually closes the page.

[Avi (use Gerrit), 2017-02-28 02:20:10]

Heh. Even doing ^C from the commandline gives:

1   content::RenderFrameHostImpl::DispatchBeforeUnload(bool, bool) + 55
2   chrome::UnloadController::CallBeforeUnloadHandlers(base::Callback<void
(bool), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) + 82
3   BrowserCloseManager::TryToCloseBrowsers() + 153
4   chrome::CloseAllBrowsers() + 52
5   -[AppController tryToTerminateApplication:] + 213
6   chrome::AttemptExit() + 213
7   base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) + 288

We're serious about calling all the beforeunload handlers all the time.

[Avi (use Gerrit), 2017-02-28 02:23:48]

avi@chromium.org changed reviewers:
+ clamy@chromium.org

[Avi (use Gerrit), 2017-02-28 02:23:49]

Camille, this touches your UMA stat, so I'd like to know what you think about
this, too.

[Charlie Reis, 2017-02-28 04:59:21]

LGTM

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

https://codereview.chromium.org/2719133002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1677:
DCHECK(!IsWaitingForUnloadACK());
On 2017/02/27 22:41:14, Charlie Reis (slow) wrote:
> On 2017/02/27 22:38:58, Avi wrote:
> > On 2017/02/27 22:26:17, Charlie Reis (slow) wrote:
> > > Can you double check what happens when closing a tab?  I noticed that this
> can
> > > return true for RVHI::is_waiting_for_close_ack_.  If that includes the
> > > beforeunload event for closing a tab, then my suggestion might not be
safe--
> a
> > > tab can show a beforeunload dialog while closing.  If that's the case,
maybe
> > it
> > > would be less confusing if we kept track of close (beforeunload + unload)
> and
> > > navigational-unload separately.
> > > 
> > > (Maybe we treat the beforeunload and unload behavior separately already
for
> > > close, but I don't think so.)
> > 
> > But we already keep track of beforeunload and unload separately, right?
> > 
> > RenderFrameHostImpl has is_waiting_for_beforeunload_ack_, which is set in
> > RenderFrameHostImpl::DispatchBeforeUnload. RenderViewHostImpl has
> > is_waiting_for_close_ack_, which is set in RenderViewHostImpl::ClosePage. I
> > haven't fully traced those calls yet, but RenderViewHostImpl::ClosePage
sends
> > ViewMsg_ClosePage which turns into RenderViewImpl::OnClosePage which
> dispatches
> > the unload event, much further along than the beforeunload event.
> 
> Does the browser process send a separate beforeunload IPC when closing a tab,
> though, or does it just send ViewMsg_ClosePage to run both beforeunload and
> unload?
> 
> I think we might only split the two events into separate IPCs for navigations,
> but I'm not sure.

On 2017/02/28 02:14:41, Avi wrote:
> As far as I can tell, yes.
> 
> On my Mac, if you try to close the tab or window, you get:
> 
> 1   content::RenderFrameHostImpl::DispatchBeforeUnload(bool, bool) + 55
> 2   chrome::UnloadController::ShouldCloseWindow() + 100
> 3   -[BrowserWindowController windowShouldClose:] + 66
> 4   -[NSWindow __close]_block_invoke + 130
> 5  -[NSWindow __close] + 312
> 
> Every close path that I can find appears to call
> chrome::UnloadController::ShouldCloseWindow(), which makes sense. And
> UnloadController::ShouldCloseWindow calls
> RenderFrameHostImpl::DispatchBeforeUnload which sends FrameMsg_BeforeUnload,
> different from RenderViewHostImpl::ClosePage sending ViewMsg_ClosePage.
> 
> I would be very surprised to find ViewMsg_ClosePage being sent without a
> FrameMsg_BeforeUnload first. That would definitely be a user-facing,
web-visible
> bug, since ViewMsg_ClosePage actually closes the page.

Great!  That simplifies things.

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

https://codereview.chromium.org/2719133002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1671: if
(IsWaitingForUnloadACK()) {
Interesting.  Yes, I think this is safer, since it can handle races where the
renderer process sent up a dialog IPC just as the browser was starting to close
the tab.  We should cancel the dialog as you're doing here.

[Avi (use Gerrit), 2017-02-28 05:09:04]

avi@chromium.org changed reviewers:
+ isherman@chromium.org

[Avi (use Gerrit), 2017-02-28 05:09:05]

+Ilya for metrics

[Avi (use Gerrit), 2017-02-28 05:12:42]

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

https://codereview.chromium.org/2719133002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1671: if
(IsWaitingForUnloadACK()) {
On 2017/02/28 04:59:21, Charlie Reis (slow) wrote:
> Interesting.  Yes, I think this is safer, since it can handle races where the
> renderer process sent up a dialog IPC just as the browser was starting to
close
> the tab.  We should cancel the dialog as you're doing here.

Yep. I realized that we already were handling the passing-IPCs-in-the-night case
for swapping out, so it would be appropriate to do so for closing, as well.

[Ilya Sherman, 2017-02-28 18:11:56]

histograms.xml lgtm

[clamy, 2017-03-01 16:11:00]

Thanks! One question below.

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

https://codereview.chromium.org/2719133002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2618: success ?
RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD
If we are in the before unload dialog, shouldn't the RendererUnresponsiveType
always be RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD?

[Avi (use Gerrit), 2017-03-01 16:17:38]

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

https://codereview.chromium.org/2719133002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2618: success ?
RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD
On 2017/03/01 16:11:00, clamy wrote:
> If we are in the before unload dialog, shouldn't the RendererUnresponsiveType
> always be RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD?

This metric is yours, so I'm trying to preserve existing behavior. I hadn't
thought a ton about the semantics of those types but the more that I think about
them the more confused I get.

From what I can see in the old code, we had three types:

RENDERER_UNRESPONSIVE_DIALOG_CLOSED: If this was a "beforeunload" dialog, this
meant that the hang was after the user clicked the "stay on page" button. If
this was a dialog that ran inside of "unload" (which shouldn't happen but
whatever) this meant that the hang was after the user clicked "cancel" in an
"ok/cancel" dialog. This conflation is weird.
RENDERER_UNRESPONSIVE_BEFORE_UNLOAD: This meant that the hang was after the user
clicked the "leave page" button.
RENDERER_UNRESPONSIVE_UNLOAD: This meant that the hang was after the user
clicked "ok" in an "ok/cancel" dialog.

This CL is meant to preserve the behavior of the code. Your comment would be a
change in the case where the user clicked "stay on page" in a "beforeunload"
dialog.

[clamy, 2017-03-01 16:25:57]

Thanks! Lgtm.

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

https://codereview.chromium.org/2719133002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2618: success ?
RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD
On 2017/03/01 16:17:38, Avi wrote:
> On 2017/03/01 16:11:00, clamy wrote:
> > If we are in the before unload dialog, shouldn't the
RendererUnresponsiveType
> > always be RendererUnresponsiveType::RENDERER_UNRESPONSIVE_BEFORE_UNLOAD?
> 
> This metric is yours, so I'm trying to preserve existing behavior. I hadn't
> thought a ton about the semantics of those types but the more that I think
about
> them the more confused I get.
> 
> From what I can see in the old code, we had three types:
> 
> RENDERER_UNRESPONSIVE_DIALOG_CLOSED: If this was a "beforeunload" dialog, this
> meant that the hang was after the user clicked the "stay on page" button. If
> this was a dialog that ran inside of "unload" (which shouldn't happen but
> whatever) this meant that the hang was after the user clicked "cancel" in an
> "ok/cancel" dialog. This conflation is weird.
> RENDERER_UNRESPONSIVE_BEFORE_UNLOAD: This meant that the hang was after the
user
> clicked the "leave page" button.
> RENDERER_UNRESPONSIVE_UNLOAD: This meant that the hang was after the user
> clicked "ok" in an "ok/cancel" dialog.
> 
> This CL is meant to preserve the behavior of the code. Your comment would be a
> change in the case where the user clicked "stay on page" in a "beforeunload"
> dialog.

Well I don't quite remember the choices behind the metric, so let's keep the
behavior as is.

[Avi (use Gerrit), 2017-03-01 16:35:37]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2017-03-01 16:35:55]

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

[commit-bot: I haz the power, 2017-03-01 18:04:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488386137837400,
"parent_rev": "91dfbfcdb9793f52ffa6a685dbd94563a02f1804", "commit_rev":
"e4115eb7314d12ffa24ae0be0d192f2712693cf0"}

[commit-bot: I haz the power, 2017-03-01 18:04:48]

Description was changed from

==========
A page can't fire dialogs in unload handlers; remove code that handles that
case.

BUG=669881
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
A page can't fire dialogs in unload handlers; remove code that handles that
case.

BUG=669881
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2719133002
Cr-Commit-Position: refs/heads/master@{#453971}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4115eb7314d12ffa24ae0be0d19...
==========

[commit-bot: I haz the power, 2017-03-01 18:04:49]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e4115eb7314d12ffa24ae0be0d19...