From analysis chat with pkasting@, et al.,  today:

wolenetz: Folks, as chromium build sheriff, I'm having difficulty finding the culprit for this failure which your recent CL was part of the delta: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/win32-rel/9247  Please assist me in finding the culprit. Compile failure log summary: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8924950650681191120/+/steps/compile/0/logs/raw_io.output_failure_summary_/0

wolenetz: Ah findit is now suspecting https://crrev.com/5120c7e4466c232c0d035d88ff9e75eb90ae4e3a with 100% confidence. pkasting@, do you concur?

pkasting: On the face of it that sounds surprising.  Looking deeper

wolenetz: thanks - that components/feature_engagement/buildflags.h is new to me. It claims to be a build artifact itself (e.g. https://cs.chromium.org/chromium/src/out/win-Debug/gen/components/feature_engagement/buildflags.h?q=components/feature_engagement/buildflags.h&sq=package:chromium&dr&l=1)

pkasting: Yes.  The source directory for it doesn't seem to have been touched in a while.  Offhand I'd have suspected some kind of missing dependency that we happened to trip over due to an ordering issue

Note that my CL doesn't add or remove any #includes
browser_window.h is marked as part of the jumbo_split_static_library("ui") which has a dependency on //components/feature_engagement
So probably someone is pulling in that header without depending on ui?
Yeah chrome/browser/safe_browsing/BUILD.gn looks busted on its face to me
They're using a lot of sources without setting the right deps for those sources.  That's a timebomb.  I think it just happened to blow up on you

It looks like this has existed for some time, e.g. https://codereview.chromium.org/2973313002 a year and a half ago adds some UI headers like this

further from pkasting@: I suspect the right fix is that that safe browsing BUILD.gn needs to add a dependency on //ui in some set of conditions.

The next build, https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/win32-rel/9248, passed the compile stage, so this indeed looks like a timing/ordering issue which may recur, incurring at least overhead on chromium sheriffs (along with ephemeral tree closures).

nparker@/vakh@, please triage ASAP. Thanks!

The consequence of this will be occasional build failures if the builder orders things such that safe_browsing comes before components/feature_engagement.  My bet is that that's unlikely enough that this escaped notice.

The fix is probably reasonably easy.  I think the safe_browsing BUILD.gn likely needs a dependency on //chrome/browser/ui for any cases where the sources #include headers from there.  Not really Win-specific.

Also, for posterity, snippets from compile failure log (https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8924950650681191120/+/steps/compile/0/logs/raw_io.output_failure_summary_/0):

FAILED: obj/chrome/browser/safe_browsing/safe_browsing/chrome_cleaner_navigation_util_win.obj
..
In file included from ../../chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_navigation_util_win.cc:9:
../..\chrome/browser/ui/browser_window.h(24,10):  fatal error: 'components/feature_engagement/buildflags.h' file not found
#include "components/feature_engagement/buildflags.h"
..
FAILED: obj/chrome/browser/safe_browsing/safe_browsing/settings_reset_prompt_controller.obj
..
In file included from ../../chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc:25:
../..\chrome/browser/ui/browser_window.h(24,10):  fatal error: 'components/feature_engagement/buildflags.h' file not found
#include "components/feature_engagement/buildflags.h"
..
FAILED: obj/chrome/browser/safe_browsing/safe_browsing/settings_reset_prompt_util_win.obj
..
In file included from ../../chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_util_win.cc:25:
../..\chrome/browser/ui/browser_window.h(24,10):  fatal error: 'components/feature_engagement/buildflags.h' file not found
#include "components/feature_engagement/buildflags.h"
..
FAILED: obj/chrome/browser/safe_browsing/safe_browsing/chrome_password_protection_service.obj
..
In file included from ../../chrome/browser/safe_browsing/chrome_password_protection_service.cc:31:
In file included from ../..\chrome/browser/ui/views/frame/browser_view.h:25:
../..\chrome/browser/ui/browser_window.h(24,10):  fatal error: 'components/feature_engagement/buildflags.h' file not found
#include "components/feature_engagement/buildflags.h"

Also, jrummell@ reports that:
"gn check out/Debug/ chrome/browser/safe_browsing:safe_browsing" (on Linux) reports 247 errors, mostly "Include not allowed" and "Can't include this header from here." I would assume that gn check has been disabled for safe_browsing.
Although the actual include that failed isn't mentioned explicitly

I have a CL out to add that dep

Hmm, that didn't work:

ERROR Dependency cycle:
  //chrome/browser/ui:ui ->
  //chrome/browser/safe_browsing:safe_browsing ->
  //chrome/browser/ui:ui

So maybe we need something more specific. Looks like there are a number of explicit ui->sb dependencies, and a number of sb->ui includes.

The fact that the "check" command is erroring seems surprising to me, though I haven't run it in a long while.

vakh -- Do you happen to know if we disabled any automatic "gn check" on :safe_browsing? The docs do say "Chrome currently doesn't come close to passing a gn check pass," so maybe this is not abnormal.

Perhaps the UI-related pieces of safe_browsing need to go in chrome/browser/ui/safe_browsing/ and move to a different build target.

Most of the non-test inclusions of c/b/ui are from settings_reset_prompt/ and chrome_cleaner/. So we should certainly pull those out to separate targets.

The others are:
  chrome_password_protection_service.cc
  safe_browsing_navigation_observer.cc
  safe_browsing_controller_client.cc

Should we file separate bugs for settings_reset_prompt/ and chrome_cleaner/ so we can fix them in parallel?

Yes SG thanks.

Filed https://crbug.com/920222 and https://crbug.com/920223. The second one is assigned to me, left the first one unassigned.

Related: https://crbug.com/923645

There are relatively few //chrome/browser/ui dependencies, and it may be feasible to move these usages into //chrome/browser/ui/safe_browsing, adding an interface. However, the same problem occurs much more frequently with circular dependencies through //chrome/browser:browser. We use Profiles all over the place, and can't plausibly refactor that out. Looking at other sub-directories of //chrome/browser, it seems that we have two options:

1. Add all our source files to //chrome/browser:browser
2. Have //chrome/browser:browser, //chrome/browser/ui:ui, and //chrome/browser/extensions:extensions add //chrome/browser/safe_browsing:safe_browsing to their allow_circular_includes list.

I'd lean towards #2, since it retains some amount of encapsulation between Safe Browsing code and non-Safe Browsing code.