The default bug view has changed. See this FAQ.

debug (popup) builds (popup) shouldn't (popup) alert (popup) on (popup) asserts

RESOLVED WONTFIX

Status

()

Core
XPCOM
RESOLVED WONTFIX
9 years ago
5 years ago

People

(Reporter: Dolske, Unassigned)

Tracking

Trunk
mozilla1.9
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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.
Attachment #318327 - Flags: superreview?(benjamin)
Attachment #318327 - Flags: review?(benjamin)
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

9 years ago
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.
(Reporter)

Comment 3

9 years ago
(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?
I think we should on mozilla-central, yes. I'd want dbaron to agree, of course ;-)
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).
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.
Yeah, I guess doing the tinderboxes first is a better plan -- one that was part of the plan for 1.9, too.
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.
Attachment #318327 - Flags: superreview?(benjamin)
Attachment #318327 - Flags: review?(benjamin)
Attachment #318327 - Flags: review-
(Reporter)

Comment 9

9 years ago
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.
(Reporter)

Updated

9 years ago
Assignee: nobody → dolske
(Reporter)

Comment 10

5 years ago
Eh, I'm just going to close this. Debug build on windows are not nearly as painful as they were ~4 years ago.
Assignee: dolske → nobody
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.