Execute BeforeUnload on subframe browser-initiated navigations

Execute BeforeUnload on subframe browser-initiated navigations

This CL ensures we will wait on the BeforeUnload event to execute in teh
renderer while doing a browser-initiated subframe navigations. This
ensures that BeforeUnload will execute on history navigations in
subframes when PlzNavigate is enabled (as is the case in the current
architecture).

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

Review-Url: https://codereview.chromium.org/2655433002
Cr-Commit-Position: refs/heads/master@{#446325}
Committed: https://chromium.googlesource.com/chromium/src/+/3f8ab9c3ab1b4471ba67b261e73753686777f21d

[clamy, 2017-01-23 18:10:20]

Description was changed from

==========
Execute BeforeUnload on subframe browser-initiated navigations

This CL ensures we will wait on the BeforeUnload event to execute in teh
renderer while doing a browser-initiated subframe navigations. This
ensures that BeforeUnload will execute on history navigations in
subframes when PlzNavigate is enabled (as is the case in the current
architecture).

BUG=683891
==========

to

==========
Execute BeforeUnload on subframe browser-initiated navigations

This CL ensures we will wait on the BeforeUnload event to execute in teh
renderer while doing a browser-initiated subframe navigations. This
ensures that BeforeUnload will execute on history navigations in
subframes when PlzNavigate is enabled (as is the case in the current
architecture).

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

[clamy, 2017-01-23 18:10:37]

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

[commit-bot: I haz the power, 2017-01-23 18:11:06]

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-01-23 18:41:54]

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

[commit-bot: I haz the power, 2017-01-23 18:41:55]

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

[clamy, 2017-01-24 12:41:34]

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

[commit-bot: I haz the power, 2017-01-24 12:41:49]

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-01-24 13:12:58]

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

[commit-bot: I haz the power, 2017-01-24 13:12:59]

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

[clamy, 2017-01-24 15:41:14]

Description was changed from

==========
Execute BeforeUnload on subframe browser-initiated navigations

This CL ensures we will wait on the BeforeUnload event to execute in teh
renderer while doing a browser-initiated subframe navigations. This
ensures that BeforeUnload will execute on history navigations in
subframes when PlzNavigate is enabled (as is the case in the current
architecture).

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

to

==========
Execute BeforeUnload on subframe browser-initiated navigations

This CL ensures we will wait on the BeforeUnload event to execute in teh
renderer while doing a browser-initiated subframe navigations. This
ensures that BeforeUnload will execute on history navigations in
subframes when PlzNavigate is enabled (as is the case in the current
architecture).

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

[clamy, 2017-01-24 15:41:14]

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

[clamy, 2017-01-24 16:11:27]

@nasko,creis: PTAL
@nasko: following our discussion yesterday, I went with implementing the simple
version, that doesn't take into account OOPIF.

[Charlie Reis, 2017-01-24 21:42:50]

I'm generally happy with this.  One question below to make sure I understand,
though.

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

https://codereview.chromium.org/2655433002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2540: // --site-per-process
is enabled.
I missed out on the discussion-- what's still left to be done for
--site-per-process?  Maybe we can mention it in the comment.

[clamy, 2017-01-25 14:08:03]

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

[commit-bot: I haz the power, 2017-01-25 14:08:20]

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

[clamy, 2017-01-25 14:09:39]

Thanks! The issue described in the comment is that the BeforeUnload IPC will
only trigger the BeforeUnload event in the frames that are in this particular
process. So with site-per-process enabled, we still don't execute the
BeforeUnload event in out-of-process iframes (unless we happen to navigate in
one).

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

https://codereview.chromium.org/2655433002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2540: // --site-per-process
is enabled.
On 2017/01/24 21:42:50, Charlie Reis wrote:
> I missed out on the discussion-- what's still left to be done for
> --site-per-process?  Maybe we can mention it in the comment.

I've moved the comment to a more appropriate place, and explained the issue with
--site-per-process in more details.

[commit-bot: I haz the power, 2017-01-25 15:47:27]

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

[commit-bot: I haz the power, 2017-01-25 15:47:28]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-25 20:58:13]

Thanks!  LGTM.

https://codereview.chromium.org/2655433002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2655433002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:1671: // should also be dispatched to
out-of-process child frames.
Oh, I see.  Yeah, there's questions about whether the browser should tell each
frame individually, what order they run in, if it's ok to run in parallel or
needs serialization, etc.  Ok, this seems fine to leave as a TODO for now.

[clamy, 2017-01-26 10:35:36]

Thanks!

[clamy, 2017-01-26 10:35:41]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2017-01-26 10: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-01-26 10:43:05]

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

[commit-bot: I haz the power, 2017-01-26 10:43:06]

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

[clamy, 2017-01-26 12:53:10]

The CQ bit was checked by clamy@chromium.org

[clamy, 2017-01-26 12:53:10]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2655433002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-26 12:53:21]

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

[commit-bot: I haz the power, 2017-01-26 14:42:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485435190339960,
"parent_rev": "6421801905e35d778fb184e4411747f55b53f390", "commit_rev":
"3f8ab9c3ab1b4471ba67b261e73753686777f21d"}

[commit-bot: I haz the power, 2017-01-26 14:42:53]

Description was changed from

==========
Execute BeforeUnload on subframe browser-initiated navigations

This CL ensures we will wait on the BeforeUnload event to execute in teh
renderer while doing a browser-initiated subframe navigations. This
ensures that BeforeUnload will execute on history navigations in
subframes when PlzNavigate is enabled (as is the case in the current
architecture).

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

to

==========
Execute BeforeUnload on subframe browser-initiated navigations

This CL ensures we will wait on the BeforeUnload event to execute in teh
renderer while doing a browser-initiated subframe navigations. This
ensures that BeforeUnload will execute on history navigations in
subframes when PlzNavigate is enabled (as is the case in the current
architecture).

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

Review-Url: https://codereview.chromium.org/2655433002
Cr-Commit-Position: refs/heads/master@{#446325}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f8ab9c3ab1b4471ba67b261e737...
==========

[commit-bot: I haz the power, 2017-01-26 14:42:55]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3f8ab9c3ab1b4471ba67b261e737...