1145988 - dist/include/GeckoProfiler.h:239:5: error: use of undeclared identifier 'MOZ_GUARD_OBJECT_NOTIFIER_INIT'

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Jan Beich]

On platforms lacking SPS profiler (e.g. Linux/PPC, BSDs, Solaris) the build is broken likely due to header bootlegging. Injecting #error into mfbt/GuardObjects.h may explain how the header ends up being included there with SPS.

  In file included from netwerk/base/Unified_cpp_netwerk_base2.cpp:2:
  In file included from netwerk/base/nsInputStreamPump.cpp:8:
  In file included from netwerk/base/nsInputStreamPump.h:14:
  In file included from dist/include/mozilla/ReentrantMonitor.h:13:
  dist/include/GeckoProfiler.h:235:28: error: expected ')'
                             MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
                             ^
  dist/include/GeckoProfiler.h:233:27: note: to match this '('
    GeckoProfilerTracingRAII(const char* aCategory, const char* aInfo,
                            ^
  dist/include/GeckoProfiler.h:248:3: error: unknown type name
        'MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER'
    MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
    ^
  dist/include/GeckoProfiler.h:249:3: error: expected member name or ';' after
        declaration specifiers
    const char* mCategory;
    ^
  dist/include/GeckoProfiler.h:236:7: error: member initializer 'mCategory' does
        not name a non-static data member or base class
      : mCategory(aCategory)
        ^~~~~~~~~~~~~~~~~~~~
  dist/include/GeckoProfiler.h:239:5: error: use of undeclared identifier
        'MOZ_GUARD_OBJECT_NOTIFIER_INIT'
      MOZ_GUARD_OBJECT_NOTIFIER_INIT;
      ^
  dist/include/GeckoProfiler.h:240:22: error: use of undeclared identifier
        'mCategory'; did you mean 'aCategory'?
      profiler_tracing(mCategory, mInfo, aBacktrace.release(), TRACING_INTERVA...
                       ^~~~~~~~~
                       aCategory
  dist/include/GeckoProfiler.h:233:40: note: 'aCategory' declared here
    GeckoProfilerTracingRAII(const char* aCategory, const char* aInfo,
                                         ^
  dist/include/GeckoProfiler.h:244:22: error: use of undeclared identifier
        'mCategory'
      profiler_tracing(mCategory, mInfo, TRACING_INTERVAL_END);
                       ^
  7 errors generated.

and only GeckoProfiler.h under tools/profiler/ is exported without SPS

  In file included from layout/generic/Unified_cpp_layout_generic0.cpp:47:
  In file included from layout/generic/StickyScrollContainer.cpp:16:
  layout/generic/../base/RestyleTracker.h:19:10: fatal error:
        'ProfilerBacktrace.h' file not found
  #include "ProfilerBacktrace.h"
           ^
  1 error generated.

but the header is required to avoid the following

  dist/include/mozilla/UniquePtr.h:488:19: error: invalid application of
        'sizeof' to an incomplete type 'ProfilerBacktrace'
      static_assert(sizeof(T) > 0, "T must be complete");
                    ^~~~~~~~~
  dist/include/mozilla/UniquePtr.h:308:7: note: in instantiation of member
        function 'mozilla::DefaultDelete<ProfilerBacktrace>::operator()' requested
        here
        getDeleter()(old);
        ^
  layout/generic/../base/RestyleTracker.h:399:22: note:
        in instantiation of member function 'mozilla::UniquePtr<ProfilerBacktrace,
        mozilla::DefaultDelete<ProfilerBacktrace> >::reset' requested here
        rd->mBacktrace.reset(profiler_get_backtrace());
                       ^
  dist/include/GeckoProfiler.h:101:7: note: forward declaration of
        'ProfilerBacktrace'
  class ProfilerBacktrace;
        ^
  1 error generated.

Comment 1

[Jan Beich]

Attachment: fix, v0

Not sure how much to comment out or provide a stub for |class ProfilerBacktrace|.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc359be5655

Comment 2

[Chiajung Hung [:chiajung]]

Comment on attachment 8581152 [details] [diff] [review]
fix, v0

Review of attachment 8581152 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 4

[Landry Breuil (:gaston)]

Comment on attachment 8581152 [details] [diff] [review]
fix, v0

Adding a different reviewer so that this hopefully lands..

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8581152 [details] [diff] [review]
fix, v0

Review of attachment 8581152 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/RestyleTracker.h
@@ +19,5 @@
>  #include "GeckoProfiler.h"
>  
> +#ifdef MOZ_ENABLE_PROFILER_SPS
> +#include "ProfilerBacktrace.h"
> +#endif

Can this just be a forward-declaration of "class ProfilerBacktrace"?

(I think it can? If so, I wouldn't bother with ifdefs around the forward-decl; and the #include can probably move to RestyleTracker.cpp, if we even need it there (we probably do).)

@@ +402,1 @@
>        rd->mBacktrace.reset(profiler_get_backtrace());

The #ifdef/#endif should move out one curly-brace level -- it should be outside the profiler_feature_active() call.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8581152 [details] [diff] [review]
fix, v0

(In reply to Daniel Holbert [:dholbert] from comment #5)
> (I think it can? If so, I wouldn't bother with ifdefs around the
> forward-decl; and the #include can probably move to RestyleTracker.cpp, if
> we even need it there (we probably do).)

Locally, I can build with just the forward-decl -- no need to move the #include to RestyleTracker.cpp -- which makes sense because I think we just pass & receive ProfilerBacktrace pointers to & from profiler functions. We never actually do anything directly with the ProfilerBacktrace.

I'm willing to believe that I'm just getting saved by unified builds and/or some other attribute of my build environment, though.

r+ with both parts of comment 5 addressed (the first part ideally with just a forward-dec; if you need the #include, r=me still stands, but I'm curious where it ends up being necessary.)

Comment 7

[Jan Beich]

Attachment: fix, v1

(In reply to Daniel Holbert [:dholbert] from comment #5)
> > +#ifdef MOZ_ENABLE_PROFILER_SPS
> > +#include "ProfilerBacktrace.h"
> > +#endif
> 
> Can this just be a forward-declaration of "class ProfilerBacktrace"?

Let me quote the last error in comment 0:

  dist/include/mozilla/UniquePtr.h:488:19: error: invalid application of
        'sizeof' to an incomplete type 'ProfilerBacktrace'
      static_assert(sizeof(T) > 0, "T must be complete");
                    ^~~~~~~~~
  dist/include/mozilla/UniquePtr.h:308:7: note: in instantiation of member
        function 'mozilla::DefaultDelete<ProfilerBacktrace>::operator()' requested
        here
        getDeleter()(old);
        ^
  layout/generic/../base/RestyleTracker.h:399:22: note:
        in instantiation of member function 'mozilla::UniquePtr<ProfilerBacktrace,
        mozilla::DefaultDelete<ProfilerBacktrace> >::reset' requested here
        rd->mBacktrace.reset(profiler_get_backtrace());
                       ^
  dist/include/GeckoProfiler.h:101:7: note: forward declaration of
        'ProfilerBacktrace'
  class ProfilerBacktrace;
        ^
  1 error generated.

> (I think it can? If so, I wouldn't bother with ifdefs around the
> forward-decl; and the #include can probably move to RestyleTracker.cpp, if
> we even need it there (we probably do).)

In comment 1 by stub I meant to make smth like the following generic
and put under |#ifndef MOZ_ENABLE_PROFILER_SPS| in GeckoProfiler.h.

  #ifdef MOZ_ENABLE_PROFILER_SPS
      UniquePtr<ProfilerBacktrace> mBacktrace;
  #else
      UniquePtr<NULL> mBacktrace;
  #endif

> @@ +402,1 @@
> >        rd->mBacktrace.reset(profiler_get_backtrace());
> 
> The #ifdef/#endif should move out one curly-brace level -- it should be
> outside the profiler_feature_active() call.

Done in v1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3db5d66a9e65

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Locally, I can build with just the forward-decl -- no need to move the
> #include to RestyleTracker.cpp -- which makes sense because I think we just
> pass & receive ProfilerBacktrace pointers to & from profiler functions. We
> never actually do anything directly with the ProfilerBacktrace.

Please, do a proper non-SPS build by modifying configure.in.
SPS bootlegs ProfilerBacktrace.h via GeckoProfiler.h.

Here's an example:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e47266fc83a3

  ../../dist/include/mozilla/UniquePtr.h:489:16: error: invalid application of 'sizeof' to incomplete type 'ProfilerBacktrace'
  ../../dist/include/mozilla/UniquePtr.h:489:5: error: possible problem detected in invocation of delete operator: [-Werror]
  ../../dist/include/mozilla/UniquePtr.h:486:8: error: 'aPtr' has incomplete type [-Werror]
  ../../dist/include/GeckoProfiler.h:101:7: error: forward declaration of 'class ProfilerBacktrace' [-Werror]
  gmake[5]: *** [Unified_cpp_layout_generic0.o] Error 1
  gmake[5]: *** Waiting for unfinished jobs....
  ../../dist/include/mozilla/UniquePtr.h:489:16: error: invalid application of 'sizeof' to incomplete type 'ProfilerBacktrace'
  ../../dist/include/mozilla/UniquePtr.h:489:5: error: possible problem detected in invocation of delete operator: [-Werror]
  ../../dist/include/mozilla/UniquePtr.h:486:8: error: 'aPtr' has incomplete type [-Werror]
  ../../dist/include/GeckoProfiler.h:101:7: error: forward declaration of 'class ProfilerBacktrace' [-Werror]
  gmake[5]: *** [Unified_cpp_layout_generic1.o] Error 1
  ../../dist/include/mozilla/UniquePtr.h:489:16: error: invalid application of 'sizeof' to incomplete type 'ProfilerBacktrace'
  ../../dist/include/mozilla/UniquePtr.h:489:5: error: possible problem detected in invocation of delete operator: [-Werror]
  ../../dist/include/mozilla/UniquePtr.h:486:8: error: 'aPtr' has incomplete type [-Werror]
  ../../dist/include/GeckoProfiler.h:101:7: error: forward declaration of 'class ProfilerBacktrace' [-Werror]
  gmake[5]: *** [Unified_cpp_layout_generic2.o] Error 1
  ../../dist/include/mozilla/UniquePtr.h:489:16: error: invalid application of 'sizeof' to incomplete type 'ProfilerBacktrace'
  ../../dist/include/mozilla/UniquePtr.h:489:5: error: possible problem detected in invocation of delete operator: [-Werror]
  ../../dist/include/mozilla/UniquePtr.h:486:8: error: 'aPtr' has incomplete type [-Werror]
  ../../dist/include/GeckoProfiler.h:101:7: error: forward declaration of 'class ProfilerBacktrace' [-Werror]
  gmake[5]: *** [Unified_cpp_layout_tables0.o] Error 1

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8586863 [details] [diff] [review]
fix, v1

Gotcha. I hadn't realized UniquePtr<Foo> requires Foo to be defined. (nsRefPtr / nsAutoPtr do not have that requirement, IIRC)

Thanks for catching & fixing the bustage!

r=me

Comment 9

[Landry Breuil (:gaston)]

m-i's closed, setting checkin-needed...

Comment 10

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4de1ed6e851d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4de1ed6e851d

Comment 12

[Jan Beich]

Comment on attachment 8586863 [details] [diff] [review]
fix, v1

Approval Request Comment
[Feature/regressing bug #]: bug 1129249 regression
[User impact if declined]: broken build on non-SPS platforms
[Describe test coverage new/current, TreeHerder]: m-c, m-i, Try in comment 7
[Risks and why]: Low: due to typos either broken build or new code from bug 1129249 being accidentally disabled for SPS platforms
[String/UUID change made/needed]: None

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8586863 [details] [diff] [review]
fix, v1

Approving this for 39 since it's a recent regression and looks like a low-risk fix.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a6b36e4eac9