Avoid showing the supervised user block interstitial more than once

Avoid showing the supervised user block interstitial more than once

With asynchronous checks, it can happen that multiple checks return that
the current page should be blocked. When trying to show an interstitial
page over another existing one, the old interstitial page automatically
is closed, which for the case of the supervised user interstitial would
close the whole tab if there is no navigation to go back to. Add a flag
in SupervisedUserNavigationObserver that stores whether an interstitial
is currently showing, and clear it when the interstitial is closed.

Because the SupervisedUserNavigationObserver is now used to store state,
it is required in order to show the interstitial, so the navigation will
now fail if the SupervisedUserNavigationObserver for a WebContents is
missing.

Also, remove the case where the interstitial immediately proceeds,
because the URL filter is always checked on the UI thread.

BUG=715981
Review-Url: https://codereview.chromium.org/2845053002
Cr-Commit-Position: refs/heads/master@{#468591}
Committed: https://chromium.googlesource.com/chromium/src/+/3483d15c1e3cec604c66663f19654a271bfe77ec

[Bernhard Bauer, 2017-04-27 11:26:32]

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

[commit-bot: I haz the power, 2017-04-27 11:26:46]

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-04-27 11:37:54]

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

[commit-bot: I haz the power, 2017-04-27 11:37:55]

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

[Bernhard Bauer, 2017-04-27 13:51:57]

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

[commit-bot: I haz the power, 2017-04-27 13:52: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-04-27 14:40:53]

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

[commit-bot: I haz the power, 2017-04-27 14:40:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bernhard Bauer, 2017-04-28 14:21:29]

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

[commit-bot: I haz the power, 2017-04-28 14:22: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-04-28 15:57:36]

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

[commit-bot: I haz the power, 2017-04-28 15:57:37]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-04-28 16:06:41]

Description was changed from

==========
Avoid showing the block interstitial for supervised users more than once

With asynchronous checks, it can happen that multiple checks return that
the current page should be blocked. When trying to show an interstitial
page over another existing one, the old interstitial page automatically
is closed, which for the case of the supervised user interstitial would
close the whole tab if there is no navigation to go back to.

BUG=715981
==========

to

==========
Avoid showing the block interstitial for supervised users more than once

With asynchronous checks, it can happen that multiple checks return that
the current page should be blocked. When trying to show an interstitial
page over another existing one, the old interstitial page automatically
is closed, which for the case of the supervised user interstitial would
close the whole tab if there is no navigation to go back to.

Because the SupervisedUserNavigationObserver is now used to store state,
it's required in order to show the interstitial, so the navigation will
now fail if the SupervisedUserNavigationObserver for a WebContents
is missing.

Also, remove the case where the interstitial immediately proceeds,
because the URL filter is always checked on the UI thread.

BUG=715981
==========

[Bernhard Bauer, 2017-04-28 16:15:06]

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

[Bernhard Bauer, 2017-04-28 16:15:32]

Description was changed from

==========
Avoid showing the block interstitial for supervised users more than once

With asynchronous checks, it can happen that multiple checks return that
the current page should be blocked. When trying to show an interstitial
page over another existing one, the old interstitial page automatically
is closed, which for the case of the supervised user interstitial would
close the whole tab if there is no navigation to go back to.

Because the SupervisedUserNavigationObserver is now used to store state,
it's required in order to show the interstitial, so the navigation will
now fail if the SupervisedUserNavigationObserver for a WebContents
is missing.

Also, remove the case where the interstitial immediately proceeds,
because the URL filter is always checked on the UI thread.

BUG=715981
==========

to

==========
Avoid showing the supervised user block interstitial more than once

With asynchronous checks, it can happen that multiple checks return that
the current page should be blocked. When trying to show an interstitial
page over another existing one, the old interstitial page automatically
is closed, which for the case of the supervised user interstitial would
close the whole tab if there is no navigation to go back to.

Because the SupervisedUserNavigationObserver is now used to store state,
it's required in order to show the interstitial, so the navigation will
now fail if the SupervisedUserNavigationObserver for a WebContents
is missing.

Also, remove the case where the interstitial immediately proceeds,
because the URL filter is always checked on the UI thread.

BUG=715981
==========

[commit-bot: I haz the power, 2017-04-28 16:15:32]

Dry run: CQ is trying da patch.

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

[Bernhard Bauer, 2017-04-28 16:17:27]

Description was changed from

==========
Avoid showing the supervised user block interstitial more than once

With asynchronous checks, it can happen that multiple checks return that
the current page should be blocked. When trying to show an interstitial
page over another existing one, the old interstitial page automatically
is closed, which for the case of the supervised user interstitial would
close the whole tab if there is no navigation to go back to.

Because the SupervisedUserNavigationObserver is now used to store state,
it's required in order to show the interstitial, so the navigation will
now fail if the SupervisedUserNavigationObserver for a WebContents
is missing.

Also, remove the case where the interstitial immediately proceeds,
because the URL filter is always checked on the UI thread.

BUG=715981
==========

to

==========
Avoid showing the supervised user block interstitial more than once

With asynchronous checks, it can happen that multiple checks return that
the current page should be blocked. When trying to show an interstitial
page over another existing one, the old interstitial page automatically
is closed, which for the case of the supervised user interstitial would
close the whole tab if there is no navigation to go back to. Add a flag
in SupervisedUserNavigationObserver that stores whether an interstitial
is currently showing, and clear it when the interstitial is closed.

Because the SupervisedUserNavigationObserver is now used to store state,
it is required in order to show the interstitial, so the navigation will
now fail if the SupervisedUserNavigationObserver for a WebContents is
missing.

Also, remove the case where the interstitial immediately proceeds,
because the URL filter is always checked on the UI thread.

BUG=715981
==========

[Bernhard Bauer, 2017-04-28 16:17:49]

bauerb@chromium.org changed reviewers:
+ treib@chromium.org

[Bernhard Bauer, 2017-04-28 16:17:49]

Please review :)

[commit-bot: I haz the power, 2017-04-28 17:01:44]

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

[commit-bot: I haz the power, 2017-04-28 17:01:45]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-05-02 08:24:47]

lgtm

Meta-comment: All this interstitial code feels way too intricate and fragile; we
keep having these edge cases... :-/

https://codereview.chromium.org/2845053002/diff/60001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_browsertest.cc (right):

https://codereview.chromium.org/2845053002/diff/60001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_browsertest.cc:236: // Navigates
to a new tab, then blocks the current page (which makes the
nit: Maybe I'm biased, but to me "Navigates to a new tab" sounds like it opens
an NTP. Maybe "Navigates to some page in a new tab, then blocks that page ..."?

https://codereview.chromium.org/2845053002/diff/60001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_interstitial.cc (right):

https://codereview.chromium.org/2845053002/diff/60001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_interstitial.cc:122:
interstitial->Init();
Should Init() now be merged into the ctor?

[Bernhard Bauer, 2017-05-02 09:02:54]

On 2017/05/02 08:24:47, Marc Treib wrote:
> lgtm
> 
> Meta-comment: All this interstitial code feels way too intricate and fragile;
we
> keep having these edge cases... :-/

Yeah... part of it is that interstitials just are weird, and then we're teasing
out edge cases by having two completely different ways of showing the
interstitial, the latter of which was added much later and got less manual
testing coverage, so it takes longer to discover all the edge cases.

https://codereview.chromium.org/2845053002/diff/60001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_browsertest.cc (right):

https://codereview.chromium.org/2845053002/diff/60001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_browsertest.cc:236: // Navigates
to a new tab, then blocks the current page (which makes the
On 2017/05/02 08:24:46, Marc Treib wrote:
> nit: Maybe I'm biased, but to me "Navigates to a new tab" sounds like it opens
> an NTP. Maybe "Navigates to some page in a new tab, then blocks that page
..."?

Done.

https://codereview.chromium.org/2845053002/diff/60001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_interstitial.cc (right):

https://codereview.chromium.org/2845053002/diff/60001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_interstitial.cc:122:
interstitial->Init();
On 2017/05/02 08:24:47, Marc Treib wrote:
> Should Init() now be merged into the ctor?

I could do that, but then the constructor would do quite a lot of things,
including handing off a pointer to itself to the interstitial page (which then
owns it, so it could decide to immediately destroy the delegate!), and that all
seems a bit brittle...

[Bernhard Bauer, 2017-05-02 09:04:15]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2017-05-02 09:04:16]

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

[commit-bot: I haz the power, 2017-05-02 09:04:28]

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-05-02 09:54:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493715855608690,
"parent_rev": "5c9d124ac2131f4765f86902ff4cf711ed96c37e", "commit_rev":
"3483d15c1e3cec604c66663f19654a271bfe77ec"}

[commit-bot: I haz the power, 2017-05-02 09:54:42]

Description was changed from

==========
Avoid showing the supervised user block interstitial more than once

With asynchronous checks, it can happen that multiple checks return that
the current page should be blocked. When trying to show an interstitial
page over another existing one, the old interstitial page automatically
is closed, which for the case of the supervised user interstitial would
close the whole tab if there is no navigation to go back to. Add a flag
in SupervisedUserNavigationObserver that stores whether an interstitial
is currently showing, and clear it when the interstitial is closed.

Because the SupervisedUserNavigationObserver is now used to store state,
it is required in order to show the interstitial, so the navigation will
now fail if the SupervisedUserNavigationObserver for a WebContents is
missing.

Also, remove the case where the interstitial immediately proceeds,
because the URL filter is always checked on the UI thread.

BUG=715981
==========

to

==========
Avoid showing the supervised user block interstitial more than once

With asynchronous checks, it can happen that multiple checks return that
the current page should be blocked. When trying to show an interstitial
page over another existing one, the old interstitial page automatically
is closed, which for the case of the supervised user interstitial would
close the whole tab if there is no navigation to go back to. Add a flag
in SupervisedUserNavigationObserver that stores whether an interstitial
is currently showing, and clear it when the interstitial is closed.

Because the SupervisedUserNavigationObserver is now used to store state,
it is required in order to show the interstitial, so the navigation will
now fail if the SupervisedUserNavigationObserver for a WebContents is
missing.

Also, remove the case where the interstitial immediately proceeds,
because the URL filter is always checked on the UI thread.

BUG=715981

Review-Url: https://codereview.chromium.org/2845053002
Cr-Commit-Position: refs/heads/master@{#468591}
Committed:
https://chromium.googlesource.com/chromium/src/+/3483d15c1e3cec604c66663f1965...
==========

[commit-bot: I haz the power, 2017-05-02 09:54:43]

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