On platforms lacking SPS profiler (Linux/PPC, BSDs, Solaris) the build is broken because only GeckoProfiler.h is exported under tools/profiler/. In file included from uriloader/prefetch/OfflineCacheUpdateChild.cpp:6: In file included from uriloader/prefetch/OfflineCacheUpdateChild.h:9: In file included from ipc/ipdl/_ipdlheaders/mozilla/docshell/POfflineCacheUpdateChild.h:14: In file included from dist/include/mozilla/ipc/MessageChannel.h:15: In file included from dist/include/mozilla/Monitor.h:10: In file included from dist/include/mozilla/CondVar.h:16: dist/include/GeckoProfiler.h:56:10: fatal error: 'ProfilerBacktrace.h' file not found #include "ProfilerBacktrace.h" ^ 1 error generated.

A new one ? oh come on :(

I guess this was caused by my recent patch for bug 1093934. The problem I hit was that the Windows build failed because for mozilla::UniquePtr<ProfilerBacktrace>, ProfilerBacktrace was only forward declared. Adding ProfilerBacktrace.h was the only way to get the Windows build on try to pass.

Why not extend bug 1145988 to include MOZILLA_XPCOMRT_API case and backout GeckoProfiler.h change of #includes?

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8fead3b81ebf due to Windows build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=8675979&repo=mozilla-inbound

Per comment 0, the original issue here is coming from GeckoProfiler.h being #included by CondVar.h. (regardless of whether profiling is enabled in our build) Perhaps CondVar.h's profiler stuff should be wrapped in "#ifdef MOZ_ENABLE_PROFILER_SPS"?

(Then we wouldn't have to remove the ProfilerBacktrace.h include from GeckoProfiler.h, and that means we wouldn't trigger the error in comment 6.)

How am I supposed to test the fix if comment 4 didn't catch Windows bustage? Trying again in hope --enable-debug build makes a difference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be40e86118c7 MSVC seems to be picky about |Maybe| unused class. UniquePtr<> non-SPS regression while exposed by bug 1093934 actually comes from bug 1129249. Let's define GeckoProfilerTracingRAII only with GeckoProfilerImpl.h which already includes ProfilerBacktrace.h. Carrying over r=rbarker from v0. Changes added in v1 need a reviewer from bug 1145988. (In reply to Daniel Holbert [:dholbert] from comment #7) > Per comment 0, the original issue here is coming from GeckoProfiler.h being > #included by CondVar.h. (regardless of whether profiling is enabled in our > build) GeckoProfiler isn't tied to --enable-profiling. And namespace pollution in SPS case under tools/profiler/ isn't something new. So, non-SPS riding on standalone XPCOM testing is actually an improvement given how often it breaks (about every month for the past 2 years). > Perhaps CondVar.h's profiler stuff should be wrapped in "#ifdef MOZ_ENABLE_PROFILER_SPS"? Stubs within GeckoProfiler.h are exactly to avoid such a strawman fix. CondVar.h isn't the only header using the profiler. https://dxr.mozilla.org/mozilla-central/search?q=GeckoProfiler.h+ext%3Ah (In reply to Daniel Holbert [:dholbert] from comment #8) > (Then we wouldn't have to remove the ProfilerBacktrace.h include from > GeckoProfiler.h, and that means we wouldn't trigger the error in comment 6.) ProfilerBacktrace.h is never exported in non-SPS case. If we're going to use SPS headers then conditionals like the following don't make sense: #if !defined(MOZ_ENABLE_PROFILER_SPS) || defined(MOZILLA_XPCOMRT_API) or #if defined(MOZ_ENABLE_PROFILER_SPS) && !defined(MOZILLA_XPCOMRT_API)

(In reply to Jan Beich from comment #9) > Created attachment 8590554 [details] [diff] [review] > fix, v1 > > How am I supposed to test the fix if comment 4 didn't catch Windows bustage? That is indeed strange. Maybe we just needed a clobber build here, for some reason? (That's one key difference between m-i vs. Try -- Try is always clobber-build, m-i is rarely clobber-build.) I don't see why we would, just spitballing... RE the updated patch: I'm happy to sign off on moving the Maybe<> variables inside the #ifdefs, if it helps things here. I'm uncomfortable with dropping the #includes, though. (Particularly the GuardObjects one; I don't see that include causing any trouble here, and we should include it since we're using stuff from it. I'm curious about the ProfilerBacktrace.h one, too.) Maybe this would all be cleaner if GeckoProfilerTracingRAII moved into its own header, which could only be included by files that use it? Then GeckoProfiler.h could legitimately drop these #includes.

Carrying over r=rbarker from v0. Changes added in this version need the same reviewer from bug 1145988. (In reply to Daniel Holbert [:dholbert] from comment #10) > I'm uncomfortable with dropping the #includes, though. (Particularly the > GuardObjects one; I don't see that include causing any trouble here, and we > should include it since we're using stuff from it. Bug 1145988 already added mozilla/GuardObjects.h. Do you want a dup? > Maybe this would all be cleaner if GeckoProfilerTracingRAII moved into its own > header, which could only be included by files that use it? Then > GeckoProfiler.h could legitimately drop these #includes. Why not GeckoProfilerImpl.h? It may make SPS header bootlegging slightly worse given the number of places GeckoProfiler.h is included in but not worse than before bug 1145988. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e5b3b7a7b96

Comment on attachment 8590593 [details] [diff] [review] fix, v2 (In reply to Jan Beich from comment #11) > Bug 1145988 already added mozilla/GuardObjects.h. Do you want a dup? I'm not quite sure what you're asking here, but it doesn't matter anymore, because: - I now see that there were actually two GuardObjects.h #includes there, and you were simply removing the second one. (not removing the include entirely) - It doesn't matter anymore, now that the guard-object-using code is moving to a different file. > > Maybe this would all be cleaner if GeckoProfilerTracingRAII moved into its own > > header [...] > Why not GeckoProfilerImpl.h? Seems reasonable to me! One nit, though: >+++ b/tools/profiler/GeckoProfilerImpl.h >+class MOZ_STACK_CLASS GeckoProfilerTracingRAII { >+public: [...] >+}; >+ > namespace mozilla { > > class MOZ_STACK_CLASS SamplerStackFrameRAII { > public: GeckoProfilerTracingRAII should go inside 'namespace mozilla' here, for consistency. This namespacing shouldn't break any of the usages, because this class is only used in RestyleTracker.cpp, inside of a 'namespace mozilla { ... }' scope. Also: This GeckoProfiler.h --> GeckoProfilerImpl.h code-moving should get sign-off from someone who actually works on profiler stuff. Tagging mstange.

(In reply to Daniel Holbert [:dholbert] from comment #12) > GeckoProfilerTracingRAII should go inside 'namespace mozilla' here, for > consistency. What about consistency with other GeckoProfiler*RAII in GeckoProfiler.h? GeckoProfiler* classes already have hints of their origin as part of the names just like mozilla_sampler_* functions. Besides, 'namespace mozilla' doesn't work as a generic way to denote SPS stuff e.g., see ProfilerBacktrace.h on which the moved GeckoProfilerTracingRAII relies.

I can't speak to GeckoProfiler coding conventions, or why GeckoProfiler has some namespaced and non-namespaced stuff. But if it's moving to GeckoProfilerImpl.h, it seems like it should match the local style in that file. Also, general mozilla style is to use "namespace mozilla"[1], so when there's ambiguity, better to err on the side of that. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

Moved GeckoProfilerTracingRAII under 'namespace mozilla' per comment 14. If :mstange disagrees one can easily swap obsolete flag. Carrying over r=rbarker from v0 and r=dholbert from v2. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6871ab383630

Oops, unrelated --enable-debug bustage in the previous Try build. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2321f13d5c2

Comment on attachment 8591303 [details] [diff] [review] fix, v2.1 Review of attachment 8591303 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine. I don't know the history of why some things are namespaced and some not.