Closed Bug 950856 Opened 6 years ago Closed 6 years ago

Fail the build if you attempt to use NS_StackWalk on Windows where it won't work

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file, 2 obsolete files)

I'm sick and tired of having this footgun blow other people's feet every time that somebody new starts to use NS_StackWalk.  The latest incident was bug 948621.  Let's just fix this once and for all!
Assignee: nobody → ehsan
Attachment #8348279 - Flags: review?(nfroyd)
Attachment #8348279 - Flags: review?(bgirard)
Comment on attachment 8348279 [details] [diff] [review]
Fail the build if you attempt to use NS_StackWalk on Windows where it won't work; r=froydnj

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

r=me, but you need a build person to sign off on the moz.build changes, I think.

::: memory/replace/dmd/DMD.cpp
@@ -16,5 @@
>  
>  #ifdef XP_WIN
> -#if defined(MOZ_OPTIMIZE) && !defined(MOZ_PROFILING)
> -#error "Optimized, DMD-enabled builds on Windows must be built with --enable-profiling"
> -#endif

Let's keep this bit so builds fail with a comprehensible error message rather than odd link errors.
Attachment #8348279 - Flags: review?(nfroyd)
Attachment #8348279 - Flags: review?(mh+mozilla)
Attachment #8348279 - Flags: review+
I disagree. NS_Stackwalk should return its best guess even if that's nothing. It shouldn't crash.
(In reply to Benoit Girard (:BenWa) from comment #3)
> I disagree. NS_Stackwalk should return its best guess even if that's
> nothing. It shouldn't crash.

I chatted with BenWa in person about this a bit but he had to go attend a meeting.

Here is what I'm trying to do.  So far, every single new consumer of NS_StackWalk has used it without realizing this weird limitation for Windows.  And I mean, every single new consumer.  My goal here is to make this fail *at compile time* because there is no conceivable case where you can use NS_StackWalk and get what you expect out of it on Windows without frame pointers.  And fixing it is not an option because the issue is that the underlying Windows API silently returns garbage.

If you're proposing that we should instead return an error or document this in the header, then that suggests that this won't fail at compile time, and then we might as well WONTFIX this bug and keep chasing new consumers of NS_StackWalk like I've been doing throughout the years. :(
Flags: needinfo?(bgirard)
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 8348279 [details] [diff] [review]
> Fail the build if you attempt to use NS_StackWalk on Windows where it won't
> work; r=froydnj
> 
> Review of attachment 8348279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but you need a build person to sign off on the moz.build changes, I
> think.

Sorry, I forgot to mention, I got an IRC review from gps of some sorts.

> ::: memory/replace/dmd/DMD.cpp
> @@ -16,5 @@
> >  
> >  #ifdef XP_WIN
> > -#if defined(MOZ_OPTIMIZE) && !defined(MOZ_PROFILING)
> > -#error "Optimized, DMD-enabled builds on Windows must be built with --enable-profiling"
> > -#endif
> 
> Let's keep this bit so builds fail with a comprehensible error message
> rather than odd link errors.

Sure, I have no problem with that!
Comment on attachment 8348279 [details] [diff] [review]
Fail the build if you attempt to use NS_StackWalk on Windows where it won't work; r=froydnj

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> (In reply to Nathan Froyd (:froydnj) from comment #2)
> > Comment on attachment 8348279 [details] [diff] [review]
> > Fail the build if you attempt to use NS_StackWalk on Windows where it won't
> > work; r=froydnj
> > 
> > Review of attachment 8348279 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me, but you need a build person to sign off on the moz.build changes, I
> > think.
> 
> Sorry, I forgot to mention, I got an IRC review from gps of some sorts.

OK, great, let's nix glandium's review, then.  Now to convince Benoit!
Attachment #8348279 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8348279 [details] [diff] [review]
Fail the build if you attempt to use NS_StackWalk on Windows where it won't work; r=froydnj

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

I talked to ehsan, IMO we should return an empty list and an error code but this solution has its advantages.
Attachment #8348279 - Flags: review?(bgirard) → review+
Flags: needinfo?(bgirard)
Well, this patch "worked" for the only case I failed to catch!  :-)
Attached patch Patch (v2) (obsolete) — Splinter Review
The thing that I forgot is that our builders use --enable-debug and --enable-optimize together, and my patch did not check that.  I don't think that change is worth another review, but here's the patch.  I'll push it if it's green on try.

https://tbpl.mozilla.org/?tree=Try&rev=331bd1f1594c
Attachment #8348279 - Attachment is obsolete: true
Attached patch Patch (v3)Splinter Review
This has changed enough to warrant another review I think

https://tbpl.mozilla.org/?tree=Try&rev=23f0280fb4aa
Attachment #8349841 - Attachment is obsolete: true
Attachment #8350111 - Flags: review?(nfroyd)
Comment on attachment 8350111 [details] [diff] [review]
Patch (v3)

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

I think this is fine; can you file a followup bug for consolidating the |!XP_WIN or (!MOZ_OPTIMIZE or ...)| condition into the build system somewhere (MOZ_CAN_WALK_THE_STACK?) so we can just use that in these three places (and DMD, and any place people start using NS_StackWalk)?
Attachment #8350111 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from comment #13)
> I think this is fine; can you file a followup bug for consolidating the
> |!XP_WIN or (!MOZ_OPTIMIZE or ...)| condition into the build system
> somewhere (MOZ_CAN_WALK_THE_STACK?) so we can just use that in these three
> places (and DMD, and any place people start using NS_StackWalk)?

The reason I did not do that is that is that I don't want that -D to be passed in to every source file that we compile.

https://hg.mozilla.org/integration/mozilla-inbound/rev/57eb08af0794
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> (In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from
> comment #13)
> > I think this is fine; can you file a followup bug for consolidating the
> > |!XP_WIN or (!MOZ_OPTIMIZE or ...)| condition into the build system
> > somewhere (MOZ_CAN_WALK_THE_STACK?) so we can just use that in these three
> > places (and DMD, and any place people start using NS_StackWalk)?
> 
> The reason I did not do that is that is that I don't want that -D to be
> passed in to every source file that we compile.

That's fair, I suppose, but certainly we could have the build system set a flag that we can support stack walking, and then individual moz.build files could examine it and set an appropriate define?  I don't know how that sits with the build system folks, but such a solution seems pretty reasonable.  (I think the logic in DMD.cpp is now out of date, for instance...)
(In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from comment #15)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> > (In reply to Nathan Froyd (:froydnj) (AFKish through 3 January 2014) from
> > comment #13)
> > > I think this is fine; can you file a followup bug for consolidating the
> > > |!XP_WIN or (!MOZ_OPTIMIZE or ...)| condition into the build system
> > > somewhere (MOZ_CAN_WALK_THE_STACK?) so we can just use that in these three
> > > places (and DMD, and any place people start using NS_StackWalk)?
> > 
> > The reason I did not do that is that is that I don't want that -D to be
> > passed in to every source file that we compile.
> 
> That's fair, I suppose, but certainly we could have the build system set a
> flag that we can support stack walking, and then individual moz.build files
> could examine it and set an appropriate define?  I don't know how that sits
> with the build system folks, but such a solution seems pretty reasonable. 
> (I think the logic in DMD.cpp is now out of date, for instance...)

We know all of these conditions at configure time right? so why can't this just go in mozilla-config.h?
I don't have any strong preferences.  Filed bug 952281 so that we can discuss it there.
https://hg.mozilla.org/mozilla-central/rev/57eb08af0794
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.