1320312 - Windows build failure - xul.dll fails to link due to: error LNK2019: unresolved external symbol "__declspec(dllimport) bool __cdecl MozStackWalk

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Bill Gianopoulos [:WG9s]]

Here are the logfile errors:

 6:57.17 Telemetry.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) bool __cdecl MozStackWalk(void (__cdecl*)(unsigned int,void *,void *,void *),unsigned int,unsigned int,void *,unsigned int,void *)" (__imp_?MozStackWalk@@YA_NP6AXIPAX00@ZII0I0@Z) referenced in function "public: void __thiscall `anonymous namespace'::KeyedStackCapturer::Capture(class nsACString_internal const &)" (?Capture@KeyedStackCapturer@?A0x07cb84f2@@QAEXABVnsACString_internal@@@Z)
 6:57.17
 6:57.17 xul.dll : fatal error LNK1120: 1 unresolved externals

Comment 1

[Bill Gianopoulos [:WG9s]]

and here is my .mozconfig

. $topsrcdir/browser/config/mozconfig
BUILD_OFFICIAL=1
export BUILD_OFFICIAL
WIN32_REDIST_DIR=~/mozilla/redist/x86
export WIN32_REDIST_DIR
#WIN32_CRT_SRC_DIR=~/mozilla/crtsrc
mk_add_options BUILD_OFFICIAL=1
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/fx-obj
mk_add_options RUN_AUTOCONF_LOCALLY=1
mk_add_options AUTOCONF=/usr/local/bin/autoconf-2.13
ac_add_options --enable-optimize=-O2
ac_add_options --disable-debug
ac_add_options --enable-tests
ac_add_options --disable-updater
ac_add_options --disable-crashreporter
ac_add_options --enable-jemalloc
ac_add_options --enable-eme=adobe
ac_add_options --enable-require-all-d3dc-versions
mk_add_options MOZ_MAKE_FLAGS="-j3"

Comment 2

[Bill Gianopoulos [:WG9s]]

I suspect bug 1225851.  I am trying just rebuilding in case this was just a hardware issue on my side.  If that fails, I will try backing 1225851 out.

Comment 3

[Richard Marti (:Paenglab)]

I tried the back out of bug 1225851 and it worked then.

Comment 4

[Richard Marti (:Paenglab)]

Bill, do you have tests disabled in your mozconfig? Maybe this is the reason why the mozilla builder work.

Comment 5

[Bill Gianopoulos [:WG9s]]

Ah I did post my mozconfig in comment #1.  I do not have tests disabled, however I do have crashreporter disabled.

Comment 6

[Bill Gianopoulos [:WG9s]]

OH and also updater because i do not support automatic updates of my builds.

Comment 7

[Alessio Placitelli [:Dexter]]

Mh, I think the MozStackWalk call in Telemetry.cpp [1] needs to be conditionally enabled/disabled based on the MOZ_STACKWALKING define [2], or at least that's what the comment says.

[1] - https://dxr.mozilla.org/mozilla-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/toolkit/components/telemetry/Telemetry.cpp#519
[2] - https://dxr.mozilla.org/mozilla-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/mozglue/misc/StackWalk.h#56

Comment 8

[Bill Gianopoulos [:WG9s]]

(In reply to Alessio Placitelli [:Dexter] from comment #7)
> Mh, I think the MozStackWalk call in Telemetry.cpp [1] needs to be
> conditionally enabled/disabled based on the MOZ_STACKWALKING define [2], or
> at least that's what the comment says.
> 
> [1] -
> https://dxr.mozilla.org/mozilla-central/rev/
> bad312aefb42982f492ad2cf36f4c6c3d698f4f7/toolkit/components/telemetry/
> Telemetry.cpp#519
> [2] -
> https://dxr.mozilla.org/mozilla-central/rev/
> bad312aefb42982f492ad2cf36f4c6c3d698f4f7/mozglue/misc/StackWalk.h#56

Alternatively this code needs to include what is required to make the stackwalk work and no be dependent on having crashreporter enabled.

Comment 9

[Alessio Placitelli [:Dexter]]

Georg, ni? you so it's on our radar for the next week.

Comment 10

[Bill Gianopoulos [:WG9s]]

It would seem to me that the correct fix is to have MOZ-STACKWALKING defined if either crashreporter or telemetry is enabled for the build.

Comment 11

[Bill Gianopoulos [:WG9s]]

ah so it really seems you need to have enable-profiling for this to work.  I can do that.

Comment 13

[Alessio Placitelli [:Dexter]]

Stealing the bug!

Comment 14

[Robin Steuber (they/them) [:bytesized]]

Is there a way for Windows developers to build while this is being fixed? I updated my tree and now I cannot build.

Comment 15

[:Gijs (he/him)]

(In reply to Kirk Steuber [:bytesized] from comment #14)
> Is there a way for Windows developers to build while this is being fixed? I
> updated my tree and now I cannot build.

Adding

ac_add_options --enable-profiling

to your mozconfig would be the simplest solution.

Comment 16

[Gregory Szorc [:gps]]

Building on Windows without a mozconfig is currently failing due to this bug. This state in unacceptable because it is too disruptive.

Please either land a fix for this bug ASAP or revert 620b85825ca5.

Comment 17

[Alessio Placitelli [:Dexter]]

Attachment: bug1320312.patch

The patch does this:

- Changes Telemetry.cpp to disable stack walking if MOZ_STACKWALKING is not defined.
- Adds MOZ_STACKWALKING to AppConstant so that it can be used to disable tests.
- Changes the stack capturing tests to be skipped if AppConstant.MOZ_STACKWALKING is not defined.

Comment 18

[Georg Fritzsche [:gfritzsche]]

(In reply to Gregory Szorc [:gps] from comment #16)
> Building on Windows without a mozconfig is currently failing due to this
> bug. This state in unacceptable because it is too disruptive.

If this is an important build configuration, maybe this should get CI coverage?

Comment 19

[Iaroslav Sheptykin]

Thanks for fixing the code :Dexter!

Comment 20

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8815175 [details] [diff] [review]
bug1320312.patch

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ -81,3 @@
>  
>  namespace {
> -

Unrelated change, please undo.
There are a few more of discarded empty lines below in this patch here.

@@ +395,5 @@
>  const nsClassHashtable<nsStringHashKey, HangReports::AnnotationInfo>&
>  HangReports::GetAnnotationInfo() const {
>    return mAnnotationInfo;
>  }
> +#if defined(ENABLE_STACK_CAPTURE)

Add an empty line before & after this one.

@@ -922,5 @@
>    bool GetSQLStats(JSContext *cx, JS::MutableHandle<JS::Value> ret,
>                     bool includePrivateSql);
>  
>    void ReadLateWritesStacks(nsIFile* aProfileDir);
> -

Unrelated change, please undo.

@@ +929,5 @@
>    AutoHashtable<SlowSQLEntryType> mSanitizedSQL;
>    Mutex mHashMutex;
>    HangReports mHangReports;
>    Mutex mHangReportsMutex;
> +#if defined(ENABLE_STACK_CAPTURE)

Please undo the removal of the empty lines here & below.

@@ +3141,5 @@
>                                    Move(aAnnotations));
>  }
> +#endif
> +
> +#if defined(ENABLE_STACK_CAPTURE)

This needs to behave the same as the declaration in Telemetry.h
The intention here was that Telemetry::CaptureStack() is always declared, but internally it's a no-op when ENABLE_STACK_CAPTURE is not defined (in TelemetryImpl::CaptureStack()).

It looks like we didn't cover this too well in the review before.

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: bug1320312.patch

Whoops, sorry Georg, the discarded blank lines were unintended.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8815255 [details] [diff] [review]
bug1320312.patch

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

Thanks!

Comment 23

[Alessio Placitelli [:Dexter]]

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

Comment 24

[Bill Gianopoulos [:WG9s]]

+#if defined(MOZ_STACKWALKING)
+#define ENABLE_STACK_CAPTURE
 #include "mozilla/StackWalk.h"
 #include "nsPrintfCString.h"
-#endif
+#endif // ENABLE_STACK_CAPTURE
           ^^^^^^^^^^^^^^^^^^^^

Shouldn't this comment say MOZ_STACKWALKING to match the condition on the #if?

Comment 25

[Alessio Placitelli [:Dexter]]

(In reply to Bill Gianopoulos [:WG9s] from comment #24)
> +#if defined(MOZ_STACKWALKING)
> +#define ENABLE_STACK_CAPTURE
>  #include "mozilla/StackWalk.h"
>  #include "nsPrintfCString.h"
> -#endif
> +#endif // ENABLE_STACK_CAPTURE
>            ^^^^^^^^^^^^^^^^^^^^
> 
> Shouldn't this comment say MOZ_STACKWALKING to match the condition on the
> #if?

Yup, good catch.

Comment 26

[Alessio Placitelli [:Dexter]]

Attachment: bug1320312.patch

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d6ef5ef0e2
Disable Telemetry stack capturing if stack walking is not available. r=gfritzsche

Comment 28

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15d6ef5ef0e2dfba731d4045813bf0f3828eb5b8
Bug 1320312 - Disable Telemetry stack capturing if stack walking is not available. r=gfritzsche

Comment 29

[Gregory Szorc [:gps]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #18)
> (In reply to Gregory Szorc [:gps] from comment #16)
> > Building on Windows without a mozconfig is currently failing due to this
> > bug. This state in unacceptable because it is too disruptive.
> 
> If this is an important build configuration, maybe this should get CI
> coverage?

This is a very valid point and I will follow-up on it in another bug.

While I'm here, nobody involved with this breakage should feel bad about breaking something that CI didn't catch: the fault is with CI for not testing build configurations that developers rely on.

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/15d6ef5ef0e2