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

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: 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]

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]

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]]

(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]

I think we should on mozilla-central, yes. I'd want dbaron to agree, of course ;-)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

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-4, no longer working on Mozilla)]

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]

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]]

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]]

Eh, I'm just going to close this. Debug build on windows are not nearly as painful as they were ~4 years ago.