Don't show dialogs from swapped out frames.

Don't show dialogs from swapped out frames.

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

Committed: https://crrev.com/1e870b1fa5121d217751feb61d5aa30386a0a0d2
Cr-Commit-Position: refs/heads/master@{#421580}

[Avi (use Gerrit), 2016-09-23 23:46:22]

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

[Avi (use Gerrit), 2016-09-23 23:46:23]

Description was changed from

==========
Don't show dialogs from swapped out frames.

BUG=634108
TEST=as in bug
==========

to

==========
Don't show dialogs from swapped out frames.

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

[commit-bot: I haz the power, 2016-09-23 23:46:57]

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, 2016-09-24 00:35:36]

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

[commit-bot: I haz the power, 2016-09-24 00:35:37]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-09-24 01:09:22]

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

[Avi (use Gerrit), 2016-09-24 01:09:23]

I'm still not entirely clear why the "suppress further dialogs" IPC isn't doing
what we want it to do.

[Avi (use Gerrit), 2016-09-24 01:10:41]

On 2016/09/24 01:09:23, Avi wrote:
> I'm still not entirely clear why the "suppress further dialogs" IPC isn't
doing
> what we want it to do.

Actually, you mentioned being sure to respond, so let me do that, and dig
further...

[Avi (use Gerrit), 2016-09-24 20:27:09]

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

[commit-bot: I haz the power, 2016-09-24 20:27:18]

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), 2016-09-24 20:31:12]

ptal

[commit-bot: I haz the power, 2016-09-24 21:30:07]

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

[commit-bot: I haz the power, 2016-09-24 21:30:08]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-27 00:06:58]

Thanks!  This fix looks good.

I was trying to figure out why SuppressFurtherDialogs isn't working, and I think
I figured it out-- normally we rely on the cross-process navigation to get to
CommitPending and SwapOutOldFrame, which tells the renderer not to send any more
dialogs, before later dismissing the dialog in DidNavigateAnyFramePostCommit. 
However, in this bug the same-process alert.html dismisses the dialog first
(without going into RFHM::CommitPending).  Later, the cross-process navigation
commits and we should get SuppressFurtherDialogs, but it doesn't matter since
the renderer already has another dialog IPC on the way up.

Your fix seems sensible, and I'm not sure there's anything better we can do for
the same-process-then-cross-process case.

I would like to include a test if possible-- maybe one that simulates a
same-process navigation right as the cross-process commit arrives?  Nasko just
wrote a test quite similar to that today:
https://codereview.chromium.org/2377453002/.  (Let me know if it proves
infeasible and we can proceed with the fix.)

Thanks!

[Charlie Reis, 2016-09-27 00:41:18]

On 2016/09/27 00:06:58, Charlie Reis (slow) wrote:
> Thanks!  This fix looks good.
> 
> I was trying to figure out why SuppressFurtherDialogs isn't working, and I
think
> I figured it out-- normally we rely on the cross-process navigation to get to
> CommitPending and SwapOutOldFrame, which tells the renderer not to send any
more
> dialogs, before later dismissing the dialog in DidNavigateAnyFramePostCommit. 
> However, in this bug the same-process alert.html dismisses the dialog first
> (without going into RFHM::CommitPending).  Later, the cross-process navigation
> commits and we should get SuppressFurtherDialogs, but it doesn't matter since
> the renderer already has another dialog IPC on the way up.
> 
> Your fix seems sensible, and I'm not sure there's anything better we can do
for
> the same-process-then-cross-process case.
> 
> I would like to include a test if possible-- maybe one that simulates a
> same-process navigation right as the cross-process commit arrives?  Nasko just
> wrote a test quite similar to that today:
> https://codereview.chromium.org/2377453002/.  (Let me know if it proves
> infeasible and we can proceed with the fix.)
> 
> Thanks!

And actually, we could even just dismiss the dialog when the commit arrives,
before it's processed (similar to how Nasko kicks off a back navigation).  That
should show the same bug.

[Avi (use Gerrit), 2016-09-27 17:27:08]

On 2016/09/27 00:41:18, Charlie Reis (slow) wrote:
> On 2016/09/27 00:06:58, Charlie Reis (slow) wrote:
> > Thanks!  This fix looks good.
> > 
> > I was trying to figure out why SuppressFurtherDialogs isn't working, and I
> think
> > I figured it out-- normally we rely on the cross-process navigation to get
to
> > CommitPending and SwapOutOldFrame, which tells the renderer not to send any
> more
> > dialogs, before later dismissing the dialog in
DidNavigateAnyFramePostCommit. 
> > However, in this bug the same-process alert.html dismisses the dialog first
> > (without going into RFHM::CommitPending).  Later, the cross-process
navigation
> > commits and we should get SuppressFurtherDialogs, but it doesn't matter
since
> > the renderer already has another dialog IPC on the way up.
> > 
> > Your fix seems sensible, and I'm not sure there's anything better we can do
> for
> > the same-process-then-cross-process case.
> > 
> > I would like to include a test if possible-- maybe one that simulates a
> > same-process navigation right as the cross-process commit arrives?  Nasko
just
> > wrote a test quite similar to that today:
> > https://codereview.chromium.org/2377453002/.  (Let me know if it proves
> > infeasible and we can proceed with the fix.)
> > 
> > Thanks!
> 
> And actually, we could even just dismiss the dialog when the commit arrives,
> before it's processed (similar to how Nasko kicks off a back navigation). 
That
> should show the same bug.

I'm sorry, but I'm not following you. In the browsertest we set up one page to
pop a dialog when leaving, we navigate to another page, we intercept creation of
the new render process so we can install a message filter, the filter catches
the commit of the new page... and what happens?

The problem is ensuring the IPC timing; we need to ensure that the dialog IPC
arrives after the commit one. That would be trivial in a unittest, but I'm not
seeing how to get the browsertest to behave like that.

[nasko, 2016-09-27 18:25:53]

On 2016/09/27 17:27:08, Avi wrote:
> On 2016/09/27 00:41:18, Charlie Reis (slow) wrote:
> > On 2016/09/27 00:06:58, Charlie Reis (slow) wrote:
> > > Thanks!  This fix looks good.
> > > 
> > > I was trying to figure out why SuppressFurtherDialogs isn't working, and I
> > think
> > > I figured it out-- normally we rely on the cross-process navigation to get
> to
> > > CommitPending and SwapOutOldFrame, which tells the renderer not to send
any
> > more
> > > dialogs, before later dismissing the dialog in
> DidNavigateAnyFramePostCommit. 
> > > However, in this bug the same-process alert.html dismisses the dialog
first
> > > (without going into RFHM::CommitPending).  Later, the cross-process
> navigation
> > > commits and we should get SuppressFurtherDialogs, but it doesn't matter
> since
> > > the renderer already has another dialog IPC on the way up.
> > > 
> > > Your fix seems sensible, and I'm not sure there's anything better we can
do
> > for
> > > the same-process-then-cross-process case.
> > > 
> > > I would like to include a test if possible-- maybe one that simulates a
> > > same-process navigation right as the cross-process commit arrives?  Nasko
> just
> > > wrote a test quite similar to that today:
> > > https://codereview.chromium.org/2377453002/.  (Let me know if it proves
> > > infeasible and we can proceed with the fix.)
> > > 
> > > Thanks!
> > 
> > And actually, we could even just dismiss the dialog when the commit arrives,
> > before it's processed (similar to how Nasko kicks off a back navigation). 
> That
> > should show the same bug.
> 
> I'm sorry, but I'm not following you. In the browsertest we set up one page to
> pop a dialog when leaving, we navigate to another page, we intercept creation
of
> the new render process so we can install a message filter, the filter catches
> the commit of the new page... and what happens?
> 
> The problem is ensuring the IPC timing; we need to ensure that the dialog IPC
> arrives after the commit one. That would be trivial in a unittest, but I'm not
> seeing how to get the browsertest to behave like that.

What if you have the filter object keep the dialog IPC around, wait to see the
commit, then dispatch the dialog IPC once the commit IPC is dispatched?

[Charlie Reis, 2016-09-27 18:26:23]

On 2016/09/27 17:27:08, Avi wrote:
> On 2016/09/27 00:41:18, Charlie Reis (slow) wrote:
> > On 2016/09/27 00:06:58, Charlie Reis (slow) wrote:
> > > Thanks!  This fix looks good.
> > > 
> > > I was trying to figure out why SuppressFurtherDialogs isn't working, and I
> > think
> > > I figured it out-- normally we rely on the cross-process navigation to get
> to
> > > CommitPending and SwapOutOldFrame, which tells the renderer not to send
any
> > more
> > > dialogs, before later dismissing the dialog in
> DidNavigateAnyFramePostCommit. 
> > > However, in this bug the same-process alert.html dismisses the dialog
first
> > > (without going into RFHM::CommitPending).  Later, the cross-process
> navigation
> > > commits and we should get SuppressFurtherDialogs, but it doesn't matter
> since
> > > the renderer already has another dialog IPC on the way up.
> > > 
> > > Your fix seems sensible, and I'm not sure there's anything better we can
do
> > for
> > > the same-process-then-cross-process case.
> > > 
> > > I would like to include a test if possible-- maybe one that simulates a
> > > same-process navigation right as the cross-process commit arrives?  Nasko
> just
> > > wrote a test quite similar to that today:
> > > https://codereview.chromium.org/2377453002/.  (Let me know if it proves
> > > infeasible and we can proceed with the fix.)
> > > 
> > > Thanks!
> > 
> > And actually, we could even just dismiss the dialog when the commit arrives,
> > before it's processed (similar to how Nasko kicks off a back navigation). 
> That
> > should show the same bug.
> 
> I'm sorry, but I'm not following you. In the browsertest we set up one page to
> pop a dialog when leaving, we navigate to another page, we intercept creation
of
> the new render process so we can install a message filter, the filter catches
> the commit of the new page... and what happens?
> 
> The problem is ensuring the IPC timing; we need to ensure that the dialog IPC
> arrives after the commit one. That would be trivial in a unittest, but I'm not
> seeing how to get the browsertest to behave like that.

That's pretty close-- the test page would have a script that shows two dialogs
back to back (e.g., "alert(1); alert(2);").  Then something similar to Nasko's
GoBackAndCommitFilter would intercept the cross-process commit IPC, dismiss
alert(1), and allow the commit to proceed.

This is guaranteed to exercise the race we want-- the browser process hasn't
sent a SuppressFurtherDialogs IPC to the renderer at the time we dismiss
alert(1), so the renderer process will send alert(2) to the browser process as
soon as it hears alert(1) is dismissed.  Meanwhile, the browser process
immediately commits the cross-process navigation after dimissing alert(1), so by
the time it hears alert(2), it's already showing the new RFH.

Sound good?

[Avi (use Gerrit), 2016-09-27 19:52:08]

On 2016/09/27 18:26:23, Charlie Reis (slow) wrote:
> On 2016/09/27 17:27:08, Avi wrote:
> > On 2016/09/27 00:41:18, Charlie Reis (slow) wrote:
> > > On 2016/09/27 00:06:58, Charlie Reis (slow) wrote:
> > > > Thanks!  This fix looks good.
> > > > 
> > > > I was trying to figure out why SuppressFurtherDialogs isn't working, and
I
> > > think
> > > > I figured it out-- normally we rely on the cross-process navigation to
get
> > to
> > > > CommitPending and SwapOutOldFrame, which tells the renderer not to send
> any
> > > more
> > > > dialogs, before later dismissing the dialog in
> > DidNavigateAnyFramePostCommit. 
> > > > However, in this bug the same-process alert.html dismisses the dialog
> first
> > > > (without going into RFHM::CommitPending).  Later, the cross-process
> > navigation
> > > > commits and we should get SuppressFurtherDialogs, but it doesn't matter
> > since
> > > > the renderer already has another dialog IPC on the way up.
> > > > 
> > > > Your fix seems sensible, and I'm not sure there's anything better we can
> do
> > > for
> > > > the same-process-then-cross-process case.
> > > > 
> > > > I would like to include a test if possible-- maybe one that simulates a
> > > > same-process navigation right as the cross-process commit arrives? 
Nasko
> > just
> > > > wrote a test quite similar to that today:
> > > > https://codereview.chromium.org/2377453002/.  (Let me know if it proves
> > > > infeasible and we can proceed with the fix.)
> > > > 
> > > > Thanks!
> > > 
> > > And actually, we could even just dismiss the dialog when the commit
arrives,
> > > before it's processed (similar to how Nasko kicks off a back navigation). 
> > That
> > > should show the same bug.
> > 
> > I'm sorry, but I'm not following you. In the browsertest we set up one page
to
> > pop a dialog when leaving, we navigate to another page, we intercept
creation
> of
> > the new render process so we can install a message filter, the filter
catches
> > the commit of the new page... and what happens?
> > 
> > The problem is ensuring the IPC timing; we need to ensure that the dialog
IPC
> > arrives after the commit one. That would be trivial in a unittest, but I'm
not
> > seeing how to get the browsertest to behave like that.
> 
> That's pretty close-- the test page would have a script that shows two dialogs
> back to back (e.g., "alert(1); alert(2);").  Then something similar to Nasko's
> GoBackAndCommitFilter would intercept the cross-process commit IPC, dismiss
> alert(1), and allow the commit to proceed.
> 
> This is guaranteed to exercise the race we want-- the browser process hasn't
> sent a SuppressFurtherDialogs IPC to the renderer at the time we dismiss
> alert(1), so the renderer process will send alert(2) to the browser process as
> soon as it hears alert(1) is dismissed.  Meanwhile, the browser process
> immediately commits the cross-process navigation after dimissing alert(1), so
by
> the time it hears alert(2), it's already showing the new RFH.
> 
> Sound good?

I'm trying to simulate the second race, which is more problematic:
1. Renderer 2 navigates
2. Renderer 1 pops a dialog
3. Dialog request comes in right after the swapout

The problem that I'm seeing (with my sequence and with your proposal) is that
message filters are per-RPH. For me, it's:
1. Filter renderer 1
2. Intercept the dialog request
3. Navigate renderer 2
4. Free the IPC from renderer 1... but by that point the RPH is torn down.

I must admit I'm not good at the details here, so I'm probably missing
something.

Nasko, you said: "What if you have the filter object keep the dialog IPC around,
wait to see the commit, then dispatch the dialog IPC once the commit IPC is
dispatched?"

Hmmm... Can I have a filter object that both lives on renderer 1's RPH (so it
can mess around with the dialog IPC) and is a WCO so it can see the commit of
the second renderer? Lemme try that...

[Charlie Reis, 2016-09-27 20:00:50]

On 2016/09/27 19:52:08, Avi wrote:
> On 2016/09/27 18:26:23, Charlie Reis (slow) wrote:
> > On 2016/09/27 17:27:08, Avi wrote:
> > > On 2016/09/27 00:41:18, Charlie Reis (slow) wrote:
> > > > On 2016/09/27 00:06:58, Charlie Reis (slow) wrote:
> > > > > Thanks!  This fix looks good.
> > > > > 
> > > > > I was trying to figure out why SuppressFurtherDialogs isn't working,
and
> I
> > > > think
> > > > > I figured it out-- normally we rely on the cross-process navigation to
> get
> > > to
> > > > > CommitPending and SwapOutOldFrame, which tells the renderer not to
send
> > any
> > > > more
> > > > > dialogs, before later dismissing the dialog in
> > > DidNavigateAnyFramePostCommit. 
> > > > > However, in this bug the same-process alert.html dismisses the dialog
> > first
> > > > > (without going into RFHM::CommitPending).  Later, the cross-process
> > > navigation
> > > > > commits and we should get SuppressFurtherDialogs, but it doesn't
matter
> > > since
> > > > > the renderer already has another dialog IPC on the way up.
> > > > > 
> > > > > Your fix seems sensible, and I'm not sure there's anything better we
can
> > do
> > > > for
> > > > > the same-process-then-cross-process case.
> > > > > 
> > > > > I would like to include a test if possible-- maybe one that simulates
a
> > > > > same-process navigation right as the cross-process commit arrives? 
> Nasko
> > > just
> > > > > wrote a test quite similar to that today:
> > > > > https://codereview.chromium.org/2377453002/.  (Let me know if it
proves
> > > > > infeasible and we can proceed with the fix.)
> > > > > 
> > > > > Thanks!
> > > > 
> > > > And actually, we could even just dismiss the dialog when the commit
> arrives,
> > > > before it's processed (similar to how Nasko kicks off a back
navigation). 
> > > That
> > > > should show the same bug.
> > > 
> > > I'm sorry, but I'm not following you. In the browsertest we set up one
page
> to
> > > pop a dialog when leaving, we navigate to another page, we intercept
> creation
> > of
> > > the new render process so we can install a message filter, the filter
> catches
> > > the commit of the new page... and what happens?
> > > 
> > > The problem is ensuring the IPC timing; we need to ensure that the dialog
> IPC
> > > arrives after the commit one. That would be trivial in a unittest, but I'm
> not
> > > seeing how to get the browsertest to behave like that.
> > 
> > That's pretty close-- the test page would have a script that shows two
dialogs
> > back to back (e.g., "alert(1); alert(2);").  Then something similar to
Nasko's
> > GoBackAndCommitFilter would intercept the cross-process commit IPC, dismiss
> > alert(1), and allow the commit to proceed.
> > 
> > This is guaranteed to exercise the race we want-- the browser process hasn't
> > sent a SuppressFurtherDialogs IPC to the renderer at the time we dismiss
> > alert(1), so the renderer process will send alert(2) to the browser process
as
> > soon as it hears alert(1) is dismissed.  Meanwhile, the browser process
> > immediately commits the cross-process navigation after dimissing alert(1),
so
> by
> > the time it hears alert(2), it's already showing the new RFH.
> > 
> > Sound good?
> 
> I'm trying to simulate the second race, which is more problematic:
> 1. Renderer 2 navigates
> 2. Renderer 1 pops a dialog
> 3. Dialog request comes in right after the swapout
> 
> The problem that I'm seeing (with my sequence and with your proposal) is that
> message filters are per-RPH. For me, it's:
> 1. Filter renderer 1
> 2. Intercept the dialog request
> 3. Navigate renderer 2
> 4. Free the IPC from renderer 1... but by that point the RPH is torn down.
> 
> I must admit I'm not good at the details here, so I'm probably missing
> something.
> 
> Nasko, you said: "What if you have the filter object keep the dialog IPC
around,
> wait to see the commit, then dispatch the dialog IPC once the commit IPC is
> dispatched?"
> 
> Hmmm... Can I have a filter object that both lives on renderer 1's RPH (so it
> can mess around with the dialog IPC) and is a WCO so it can see the commit of
> the second renderer? Lemme try that...

Sorry, maybe I wasn't clear.  You shouldn't need to do any filtering on the
dialog IPCs, or for RPH 1 at all.

Prereq: Run a script in renderer 1 that shows back-to-back dialogs, and start a
cross process navigation to renderer 2.  We want alert(1) visible when the
commit arrives.  (Is it possible to programmatically start the navigation after
the dialog is showing?)

The filter I was referring to would be on RPH 2, watching for the commit from
renderer 2.  When the commit arrives, we call CancelActiveAndPendingDialogs() on
the WebContents and then let the commit proceed.  Renderer 1 would automatically
send its next dialog IPC.

I suppose we would need to find a way for the test to wait for that dialog IPC,
so we can verify no new dialog gets shown?  I'm guessing that's just a matter of
waiting for process 1 to exit.

(Sorry, I'm not sure which part you're stuck on.  For reference, I'm not sure I
understand Nasko's suggestion, so we may be talking about different things.)  :)

[Avi (use Gerrit), 2016-09-27 20:05:13]

On 2016/09/27 20:00:50, Charlie Reis (slow) wrote:
> When the commit arrives, we call CancelActiveAndPendingDialogs() on
> the WebContents and then let the commit proceed.

The WebContents already calls CancelActiveAndPendingDialogs on the commit, and I
worry about interfering.

> (Sorry, I'm not sure which part you're stuck on.  For reference, I'm not sure
I
> understand Nasko's suggestion, so we may be talking about different things.) 
:)

In http://crbug.com/634108 there is a race which I talk about in comment 10, but
adjusting the logging created a worse race that I talk about in comment 15. The
first race involves the bad renderer spewing dialogs and the second one ends up
persisting, while the worse race ends up with even the first dialog persisting.
I'm going to repro that worse race in the browser test, because it's simpler and
because the same issue is coming up.

Let's suspend discussion here and I'll bring actual issues rather than
ponderings of "how can this work"...

[Avi (use Gerrit), 2016-09-27 21:04:19]

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

[commit-bot: I haz the power, 2016-09-27 21:04:45]

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), 2016-09-27 21:05:14]

ptal

[commit-bot: I haz the power, 2016-09-27 21:31:31]

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

[commit-bot: I haz the power, 2016-09-27 21:31:32]

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...)

[Charlie Reis, 2016-09-27 22:18:17]

Sure, that approach looks like it should work, too.  LGTM with nits.

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

https://codereview.chromium.org/2366693006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6161:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Looks like this is failing on the bots?

https://codereview.chromium.org/2366693006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6185:
*(volatile int*)(nullptr) = 0xDEAD;
NOTREACHED or CHECK(false)?  Or are those not as effective?

[nasko, 2016-09-27 22:24:22]

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

[nasko, 2016-09-27 22:24:22]

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

https://codereview.chromium.org/2366693006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6161:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2016/09/27 22:18:17, Charlie Reis wrote:
> Looks like this is failing on the bots?

Hmm, this is a bit problematic since this object is both BrowserMessageFilter
and WebContentsObserver, both of which have OnMessageReceived. I'd expect this
will be called on both threads, even though it is even more weird to think which
version of the method that is inheriting : ).

[Avi (use Gerrit), 2016-09-27 23:00:43]

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

https://codereview.chromium.org/2366693006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6161:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2016/09/27 22:24:22, nasko (slow) wrote:
> On 2016/09/27 22:18:17, Charlie Reis wrote:
> > Looks like this is failing on the bots?
> 
> Hmm, this is a bit problematic since this object is both BrowserMessageFilter
> and WebContentsObserver, both of which have OnMessageReceived. I'd expect this
> will be called on both threads, even though it is even more weird to think
which
> version of the method that is inheriting : ).

Will fix in the next version.

https://codereview.chromium.org/2366693006/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6185:
*(volatile int*)(nullptr) = 0xDEAD;
On 2016/09/27 22:18:17, Charlie Reis wrote:
> NOTREACHED or CHECK(false)?  Or are those not as effective?

I had DCHECK(false) which didn't do anything. I'll try CHECK(false).

[Avi (use Gerrit), 2016-09-28 15:18:08]

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

[commit-bot: I haz the power, 2016-09-28 15:18:21]

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), 2016-09-28 15:54:01]

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

[commit-bot: I haz the power, 2016-09-28 15:54:13]

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, 2016-09-28 16:44:36]

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

[commit-bot: I haz the power, 2016-09-28 16:44:37]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-09-28 16:54:30]

All fixed; please take a look.

[Charlie Reis, 2016-09-28 17:14:16]

Thanks!  LGTM.

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

https://codereview.chromium.org/2366693006/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6181: class
: public WebContentsObserver {
Wow, I don't know that I've seen anonymous inner classes in C++ before.  Neat. 
:)

https://codereview.chromium.org/2366693006/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6185: using
WebContentsObserver::Observe;
Is this needed?  Seems like we should be able to resolve the Observe call above,
but I could be wrong.

[Avi (use Gerrit), 2016-09-28 17:46:16]

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

https://codereview.chromium.org/2366693006/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6181: class
: public WebContentsObserver {
On 2016/09/28 17:14:16, Charlie Reis wrote:
> Wow, I don't know that I've seen anonymous inner classes in C++ before.  Neat.

> :)

Thanks! I'm not trying to be clever; this seems to be the simplest way of doing
it.

It seems to be hinted at in the C++ style guide, but it certainly not
prohibited.

https://codereview.chromium.org/2366693006/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:6185: using
WebContentsObserver::Observe;
On 2016/09/28 17:14:16, Charlie Reis wrote:
> Is this needed?  Seems like we should be able to resolve the Observe call
above,
> but I could be wrong.

No, it's protected, and even though (I think) there's implicit friendship,
friends don't get to call protected functions.

[Avi (use Gerrit), 2016-09-28 18:19:55]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2016-09-28 18:20:18]

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, 2016-09-28 18:28:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-28 18:31:13]

Description was changed from

==========
Don't show dialogs from swapped out frames.

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

to

==========
Don't show dialogs from swapped out frames.

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

Committed: https://crrev.com/1e870b1fa5121d217751feb61d5aa30386a0a0d2
Cr-Commit-Position: refs/heads/master@{#421580}
==========

[commit-bot: I haz the power, 2016-09-28 18:31:14]

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