Fix JavaScript alerts from frames with oopif on, after a cross-process click.

Fix JavaScript alerts from frames with oopif on, after a cross-process click.

BUG=453893, 593482
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/6b8dbb34625fc40457dcffc75ae0409176d843af
Cr-Commit-Position: refs/heads/master@{#386158}

[Avi (use Gerrit), 2016-04-01 20:47:09]

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

[commit-bot: I haz the power, 2016-04-01 20:47:32]

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

[commit-bot: I haz the power, 2016-04-01 20:50:26]

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

[commit-bot: I haz the power, 2016-04-01 20:50:27]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Avi (use Gerrit), 2016-04-01 21:08:10]

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

[commit-bot: I haz the power, 2016-04-01 21:09:16]

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

[commit-bot: I haz the power, 2016-04-01 21:46:01]

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

[commit-bot: I haz the power, 2016-04-01 21:46:02]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Avi (use Gerrit), 2016-04-02 17:45:15]

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

[commit-bot: I haz the power, 2016-04-02 17:45:28]

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

[commit-bot: I haz the power, 2016-04-02 19:10:47]

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

[commit-bot: I haz the power, 2016-04-02 19:10:48]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-04-03 23:59:25]

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

[Avi (use Gerrit), 2016-04-03 23:59:26]

Charlie, because you introduced this in https://codereview.chromium.org/50223002
. That also modified BrowserTest.CrossProcessNavCancelsDialogs to test this, and
that still passes.

Nasko for IPC.

[Avi (use Gerrit), 2016-04-04 01:47:18]

On 2016/04/03 23:59:26, Avi wrote:
> Charlie, because you introduced this in
https://codereview.chromium.org/50223002
> . That also modified BrowserTest.CrossProcessNavCancelsDialogs to test this,
and
> that still passes.
> 
> Nasko for IPC.

Actually, hold off on this. First, PageGroupLoadDeferrer doesn't even exist any
more (It's ScopedPageLoadDeferrer as of
https://codereview.chromium.org/172513002, two years ago) and because I didn't
realize that the test for this is disabled.

The test actually fails; I need to think about this one harder.

[nasko, 2016-04-04 22:00:16]

Just one question. Otherwise it looks fine.

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

https://codereview.chromium.org/1849013004/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:1581: suppress_dialogs_until_swap_out_ =
false;
Is it safe? This RenderFrame is about to be booted and deleted, does it make
sense to allow it to show dialogs?

[Avi (use Gerrit), 2016-04-05 01:31:18]

Nasko,

As I noted, please hold off on this. I don't fully understand this code, and am
still working on it.

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

https://codereview.chromium.org/1849013004/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:1581: suppress_dialogs_until_swap_out_ =
false;
On 2016/04/04 22:00:16, nasko (slow) wrote:
> Is it safe? This RenderFrame is about to be booted and deleted, does it make
> sense to allow it to show dialogs?

This particular switch is to make sure that a frame can't prevent itself from
being switched out by excessive alerts. It was introduced in
https://codereview.chromium.org/50223002 but I don't fully understand it yet.

[Avi (use Gerrit), 2016-04-06 22:20:44]

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

https://codereview.chromium.org/1849013004/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:1581: suppress_dialogs_until_swap_out_ =
false;
On 2016/04/04 22:00:16, nasko (very slow Apr 8-18) wrote:
> Is it safe? This RenderFrame is about to be booted and deleted, does it make
> sense to allow it to show dialogs?

Is being swapped out a guarantee that the frame is going to be deleted?

[nasko, 2016-04-06 22:28:30]

On 2016/04/06 22:20:44, Avi wrote:
>
https://codereview.chromium.org/1849013004/diff/40001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1849013004/diff/40001/content/renderer/render...
> content/renderer/render_frame_impl.cc:1581: suppress_dialogs_until_swap_out_ =
> false;
> On 2016/04/04 22:00:16, nasko (very slow Apr 8-18) wrote:
> > Is it safe? This RenderFrame is about to be booted and deleted, does it make
> > sense to allow it to show dialogs?
> 
> Is being swapped out a guarantee that the frame is going to be deleted?

Yes : ). We no longer have swapped out RenderFrames. WebFrame::swap() can fail,
but I think even in that case RenderFrame is deleted. dcheng@ can confirm the
last part.

[dcheng, 2016-04-07 01:54:47]

On 2016/04/06 at 22:28:30, nasko wrote:
> On 2016/04/06 22:20:44, Avi wrote:
> >
https://codereview.chromium.org/1849013004/diff/40001/content/renderer/render...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
https://codereview.chromium.org/1849013004/diff/40001/content/renderer/render...
> > content/renderer/render_frame_impl.cc:1581: suppress_dialogs_until_swap_out_
=
> > false;
> > On 2016/04/04 22:00:16, nasko (very slow Apr 8-18) wrote:
> > > Is it safe? This RenderFrame is about to be booted and deleted, does it
make
> > > sense to allow it to show dialogs?
> > 
> > Is being swapped out a guarantee that the frame is going to be deleted?
> 
> Yes : ). We no longer have swapped out RenderFrames. WebFrame::swap() can
fail, but I think even in that case RenderFrame is deleted. dcheng@ can confirm
the last part.

Yes, calling old_frame->swap(new_frame) guarantees that |old_frame| will be
detached. I'll add a comment to clarify this.

[Avi (use Gerrit), 2016-04-07 01:58:40]

On 2016/04/07 01:54:47, dcheng wrote:
> On 2016/04/06 at 22:28:30, nasko wrote:
> > On 2016/04/06 22:20:44, Avi wrote:
> > >
>
https://codereview.chromium.org/1849013004/diff/40001/content/renderer/render...
> > > File content/renderer/render_frame_impl.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1849013004/diff/40001/content/renderer/render...
> > > content/renderer/render_frame_impl.cc:1581:
suppress_dialogs_until_swap_out_
> =
> > > false;
> > > On 2016/04/04 22:00:16, nasko (very slow Apr 8-18) wrote:
> > > > Is it safe? This RenderFrame is about to be booted and deleted, does it
> make
> > > > sense to allow it to show dialogs?
> > > 
> > > Is being swapped out a guarantee that the frame is going to be deleted?
> > 
> > Yes : ). We no longer have swapped out RenderFrames. WebFrame::swap() can
> fail, but I think even in that case RenderFrame is deleted. dcheng@ can
confirm
> the last part.
> 
> Yes, calling old_frame->swap(new_frame) guarantees that |old_frame| will be
> detached. I'll add a comment to clarify this.

Oh, nice! I'll simplify the code tomorrow.

[Avi (use Gerrit), 2016-04-07 14:40:50]

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

[commit-bot: I haz the power, 2016-04-07 14:41:06]

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

[Avi (use Gerrit), 2016-04-07 17:40:57]

Nasko, Charlie, PTAL.

[commit-bot: I haz the power, 2016-04-07 17:46:05]

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

[commit-bot: I haz the power, 2016-04-07 17:46:06]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-04-07 23:03:12]

Description was changed from

==========
Fix JavaScript alerts from frames with oopif on, after a cross-process click.

BUG=453893
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix JavaScript alerts from frames with oopif on, after a cross-process click.

BUG=453893,593482
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-04-08 17:27:39]

Nice.  LGTM!

There's a chance that this logic might be technically unnecessary (in terms of
ScopedPageLoadDeferrer) now that we don't navigate to swappedout:// as part of
SwapOut anymore, but I'm guessing we want to keep it anyway-- we shouldn't have
dialogs popping up as a frame is swapping out, since the new frame has already
committed at that point in time.  Even if the browser process ignored the
requests, it could confuse the renderer process to have a nested message loop
for the dialog, and that would affect other frames in the process.

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

https://codereview.chromium.org/1849013004/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:564: // separately so that
the PageGroupLoadDeferrers of any current dialogs are no
nit: ScopedPageLoadDeferrer (throughout the CL)

https://codereview.chromium.org/1849013004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1849013004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:990: // Navigate
the top frame cross-platform; ensure that dialogs work.
nit: cross-site

(I don't think we support cross-platform links yet.)  :)

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

https://codereview.chromium.org/1849013004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:1580:
render_view_->suppress_dialogs_until_swap_out_ = false;
Yes, I agree with removing this.  This RenderFrameImpl will go away after this.

[nasko, 2016-04-08 17:33:23]

LGTM with a nit.

https://codereview.chromium.org/1849013004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1849013004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:990: // Navigate
the top frame cross-platform; ensure that dialogs work.
nit: s/cross-platform/cross-site/?

[Avi (use Gerrit), 2016-04-08 17:44:01]

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

https://codereview.chromium.org/1849013004/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:564: // separately so that
the PageGroupLoadDeferrers of any current dialogs are no
On 2016/04/08 17:27:39, Charlie Reis wrote:
> nit: ScopedPageLoadDeferrer (throughout the CL)

Done.

https://codereview.chromium.org/1849013004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1849013004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:990: // Navigate
the top frame cross-platform; ensure that dialogs work.
On 2016/04/08 17:33:23, nasko (very slow Apr 8-18) wrote:
> nit: s/cross-platform/cross-site/?

Brain-o.

[Avi (use Gerrit), 2016-04-08 17:45:05]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-04-08 17:45:06]

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

[commit-bot: I haz the power, 2016-04-08 17:45:37]

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

[commit-bot: I haz the power, 2016-04-08 18:59:46]

Description was changed from

==========
Fix JavaScript alerts from frames with oopif on, after a cross-process click.

BUG=453893,593482
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix JavaScript alerts from frames with oopif on, after a cross-process click.

BUG=453893,593482
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-04-08 18:59:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-08 19:01:18]

Description was changed from

==========
Fix JavaScript alerts from frames with oopif on, after a cross-process click.

BUG=453893,593482
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix JavaScript alerts from frames with oopif on, after a cross-process click.

BUG=453893,593482
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/6b8dbb34625fc40457dcffc75ae0409176d843af
Cr-Commit-Position: refs/heads/master@{#386158}
==========

[commit-bot: I haz the power, 2016-04-08 19:01:20]

Patchset 5 (id:??) landed as
https://crrev.com/6b8dbb34625fc40457dcffc75ae0409176d843af
Cr-Commit-Position: refs/heads/master@{#386158}