Closed
Bug 950856
Opened 11 years ago
Closed 11 years ago
Fail the build if you attempt to use NS_StackWalk on Windows where it won't work
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
Details
Attachments
(1 file, 2 obsolete files)
6.19 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ehsan
Assignee | ||
Updated•11 years ago
|
Attachment #8348279 -
Flags: review?(nfroyd)
Attachment #8348279 -
Flags: review?(bgirard)
![]() |
||
Comment 2•11 years ago
|
||
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+
Comment 3•11 years ago
|
||
I disagree. NS_Stackwalk should return its best guess even if that's nothing. It shouldn't crash.
Assignee | ||
Comment 4•11 years ago
|
||
(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)
Assignee | ||
Comment 5•11 years ago
|
||
(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 6•11 years ago
|
||
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 7•11 years ago
|
||
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+
Updated•11 years ago
|
Flags: needinfo?(bgirard)
Assignee | ||
Comment 8•11 years ago
|
||
Comment 9•11 years ago
|
||
Backed out for Windows B2G bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea2f91a2994
https://tbpl.mozilla.org/php/getParsedLog.php?id=32174614&tree=Mozilla-Inbound
Assignee | ||
Comment 10•11 years ago
|
||
Well, this patch "worked" for the only case I failed to catch! :-)
Assignee | ||
Comment 11•11 years ago
|
||
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
Assignee | ||
Comment 12•11 years ago
|
||
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 13•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
(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
![]() |
||
Comment 15•11 years ago
|
||
(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...)
Comment 16•11 years ago
|
||
(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?
Assignee | ||
Comment 17•11 years ago
|
||
I don't have any strong preferences. Filed bug 952281 so that we can discuss it there.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•