Assigning this to grt@ who's currently working in this space.

It sounds like the policy isn't applied to tabs during session restore.

Could you assign a target milestone for this, Georges? Should we aim for M67? Thanks.

Yes, 67 would be ideal. Updated the bug (also I don't see why this would be Windows only, so adding Mac/Linux as well).

Update: it's a classic race condition. The startup codepath checks the restored URL against the blacklist, yet the blacklist has yet to complete initialization. In slightly more detail:

The policy::URLBlacklistManager is created on the UI thread during startup the first time it is needed. This is when the NavigationThrottles are created for the first navigation (of any kind, not just the navigation to the blocked URL). The manager reads the policy prefs synchronously, then posts a task at BACKGROUND priority to build its blacklist. At this point, SessionRestore::RestoreSession continues on the UI thread, restoring all tabs in the session. At some point during restoration, the background thread builds the blacklist and bounces it back to the UI thread. While that's happening, the UI thread eventually gets to restoring the banned URL. Sure enough, it checks the URLBlacklist for the banned URL before the UI thread has handled the call to apply the newly computed blacklist. And there you go.

+joaodasilva: it looks like the blacklist manager has built the actual list on some other thread since its inception. Do you recall why the thread bounce was introduced? It seems to me that we should always gate first loads on having the blacklist ready. I can see doing the work on another thread for the sake of parallelism during startup at the cost of a little complexity.

pastarmovj: as an OWNER in components/policy, what are your thoughts?

FWIW, if the only reason for deferring the blacklist building is avoiding the startup penalty, we should definitely reconsider. This would only impact devices that have this policy set, and the admins who do set it expect to work all the time, including on session restore. That penalty should be more than acceptable in that case.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f011a45be6d1c28c4ffb5a04729d5a635d7c7ab

commit 2f011a45be6d1c28c4ffb5a04729d5a635d7c7ab
Author: Greg Thompson <grt@chromium.org>
Date: Mon Apr 30 09:44:14 2018

Initialize the URL blacklist during startup.

This allows it to be in force during session restore.

This CL also gently reduces heap usage during blacklist preparation and
moves regular blacklist prep to a non-blocking background thread.

BUG= 827173 

Change-Id: Ibabae7111b6e005ea0433567a8dff840ce2cd156
Reviewed-on: https://chromium-review.googlesource.com/1032744
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554714}
[modify] https://crrev.com/2f011a45be6d1c28c4ffb5a04729d5a635d7c7ab/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/2f011a45be6d1c28c4ffb5a04729d5a635d7c7ab/components/policy/core/browser/url_blacklist_manager.cc
[modify] https://crrev.com/2f011a45be6d1c28c4ffb5a04729d5a635d7c7ab/tools/metrics/histograms/histograms.xml

Fixed in 68.0.3416.0.

Policies are read synchronously at startup precisely to avoid bugs like this. But the thread bounce breaks the behavior for this one, and that went unnoticed for a long time :-) I don't remember the original reason for that thread behavior.

The fix looks good. Thanks!

Thanks for taking a look!

I added the URLBlacklistManager.ConstructorBuildTime metric to measure the impact of the synchronous load during startup. The 7d average on stable is sub 5ms. This seems pretty small, and only impacts startup where at least one of the two policy URL lists is set. I'll retire the metric, as it has done its job.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3706d90075cafeec69e89798ede9894bc5f8fb23

commit 3706d90075cafeec69e89798ede9894bc5f8fb23
Author: Greg Thompson <grt@chromium.org>
Date: Tue Sep 04 14:13:04 2018

Remove URLBlacklistManager.ConstructorBuildTime.

It served its purpose and is now safe to remove.

BUG= 827173 
R=pastarmovj@chromium.org,isherman@chromium.org

Change-Id: I297865c08486ace46c5a42df698cf169ab5d7f9a
Reviewed-on: https://chromium-review.googlesource.com/1203372
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588507}
[modify] https://crrev.com/3706d90075cafeec69e89798ede9894bc5f8fb23/components/policy/core/browser/url_blacklist_manager.cc
[modify] https://crrev.com/3706d90075cafeec69e89798ede9894bc5f8fb23/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3706d90075cafeec69e89798ede9894bc5f8fb23

commit 3706d90075cafeec69e89798ede9894bc5f8fb23
Author: Greg Thompson <grt@chromium.org>
Date: Tue Sep 04 14:13:04 2018

Remove URLBlacklistManager.ConstructorBuildTime.

It served its purpose and is now safe to remove.

BUG= 827173 
R=pastarmovj@chromium.org,isherman@chromium.org

Change-Id: I297865c08486ace46c5a42df698cf169ab5d7f9a
Reviewed-on: https://chromium-review.googlesource.com/1203372
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588507}
[modify] https://crrev.com/3706d90075cafeec69e89798ede9894bc5f8fb23/components/policy/core/browser/url_blacklist_manager.cc
[modify] https://crrev.com/3706d90075cafeec69e89798ede9894bc5f8fb23/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3706d90075cafeec69e89798ede9894bc5f8fb23

commit 3706d90075cafeec69e89798ede9894bc5f8fb23
Author: Greg Thompson <grt@chromium.org>
Date: Tue Sep 04 14:13:04 2018

Remove URLBlacklistManager.ConstructorBuildTime.

It served its purpose and is now safe to remove.

BUG= 827173 
R=pastarmovj@chromium.org,isherman@chromium.org

Change-Id: I297865c08486ace46c5a42df698cf169ab5d7f9a
Reviewed-on: https://chromium-review.googlesource.com/1203372
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588507}
[modify] https://crrev.com/3706d90075cafeec69e89798ede9894bc5f8fb23/components/policy/core/browser/url_blacklist_manager.cc
[modify] https://crrev.com/3706d90075cafeec69e89798ede9894bc5f8fb23/tools/metrics/histograms/histograms.xml

Verified this issue on Window 10, Windows 7 with chrome #71.0.3543.0 and observed the fix is worked as intended. Hence adding TE Verified labels.

Attaching the screen-cast for reference.

Deleted: 827173.mp4 1.5 MB

	827173.mp4 1.5 MB View Download