Last Comment Bug 431289 - debug (popup) builds (popup) shouldn't (popup) alert (popup) on (popup) asserts
: debug (popup) builds (popup) shouldn't (popup) alert (popup) on (popup) asserts
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 2 votes (vote)
: mozilla1.9
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-28 23:49 PDT by Justin Dolske [:Dolske]
Modified: 2012-04-20 22:16 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (945 bytes, patch)
2008-04-28 23:49 PDT, Justin Dolske [:Dolske]
benjamin: review-
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2008-04-28 23:49:20 PDT
Created attachment 318327 [details] [diff] [review]
Patch v.1

This irritates me every time I set up a fresh build environment on Windows.

1) Do a debug build, run it
2) Get stuck in popup alert hell because of asserts
3) Spend 10 minutes trying to remember how to disable them

Most people seem to be annoyed this, and have disabled it in their build environment.

It would be better to just warn on the console by default (like other platforms). And if someone really wants to have the current behavior, they can set XPCOM_DEBUG_BREAK to "trap" or "break". It's one less hoop for new developers to jump through, and a small pain for veterans.
Comment 1 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2008-04-29 08:06:23 PDT
The only patch I'm going to accept is one that makes assertions more painful, not less: the default should be that assertions trap/crash.
Comment 2 Jesse Ruderman 2008-04-29 09:39:11 PDT
In the distant future, when Firefox doesn't assert all the time, sure.  But for the last five years, the only effect of the default has been to annoy and punish people who develop on Windows.
Comment 3 Justin Dolske [:Dolske] 2008-04-29 11:18:38 PDT
(In reply to comment #1)
> The only patch I'm going to accept is one that makes assertions more painful,
> not less: the default should be that assertions trap/crash.

I'm all for that, but I think the current behavior combined with the present state of assertions being common just results in people who use debug builds disabling the feature entirely. So when we do eventually get to a state where assertions are rare (and worthy of fatal/annoying behavior, we're stuck in a state where folks still have it disabled). Debug builds on Windows are basically unusable with the way things are.

Devil's Advocate: Would you take a patch today to change the default on all platforms to crash on assert? Is not, why?
Comment 4 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2008-04-29 11:19:36 PDT
I think we should on mozilla-central, yes. I'd want dbaron to agree, of course ;-)
Comment 5 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-04-29 11:26:37 PDT
I'd be ok with changing the default, at least as long as we still have the ability to disable the option (which I suspect people will need to do for a bit).
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-04-29 11:56:21 PDT
Making assertions fatal now is just going to force everyone to override the setting. It just adds another hoop people will have to jump through before they can do useful development. So, no thanks.

IMHO we should start by driving assertions to zero for all tests on all tinderboxes. Then make assertions fatal on all tests on all tinderboxes. *Then* flip the switch to make them fatal for all developers.

In the meantime I think we should relax things on Windows because right now the assertion alerts are just pain. My launch scripts disable them and I bet everyone else would if they knew how.
Comment 7 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-04-29 12:02:29 PDT
Yeah, I guess doing the tinderboxes first is a better plan -- one that was part of the plan for 1.9, too.
Comment 8 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2008-08-14 06:53:53 PDT
Comment on attachment 318327 [details] [diff] [review]
Patch v.1

I still don't think this is the right course of action. If you are running in a debugger, we do want assertions to trap. If you want to simply disable the debug dialog for assertions, that might make sense.
Comment 9 Justin Dolske [:Dolske] 2008-08-15 14:06:20 PDT
Yeah, I'd be happy with just removing the "click ok to continue" dialog. Looks like it's just the WinMessageBox call (same file)? Will investigate.
Comment 10 Justin Dolske [:Dolske] 2012-04-20 22:16:39 PDT
Eh, I'm just going to close this. Debug build on windows are not nearly as painful as they were ~4 years ago.

Note You need to log in before you can comment on or make changes to this bug.