355195 - "temporary testing assertions" that were supposed to be disabled before the release haven't been disabled

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[:Gavin Sharp [email: [e-mail]]]

The NS_ASSERT dialog (like the one shown in https://bugzilla.mozilla.org/attachment.cgi?id=241016&action=view) that was added primarily for Places testing wasn't disabled before spinning RCs, which was the original intent. This means that in some failure cases we show a large ugly dialog instead of just erroring to the console.

From a quick LXR search it seems that the feeds and search code both use this function, though it's hard to tell how likely the failure cases are to occur.

Fixing this is pretty trivial, but it would require a respin.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: make the function do nothing for builds with official branding

This makes the NS_ASSERT function do nothing for builds that have --enable-official-branding set.

It would probably better to make this still show the message somehow, but I don't think dump()ing to the console is appropriate for a release build, and I'd rather not complicate this change to avoid making changes that might break existing callers.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

A quick audit of the search service callers shows that the dialog is unlikely to be triggered unless there is a truly exception circumstance (e.g. abuse of the API by an extension, programming error), and I think the same is true of the feed service callers, though I am less familiar with those.

I think the odds that this dialog can be triggered by a user action or invalid input on the web are fairly low, but I haven't ruled it out completely.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Per Gavin's evaluation in comment 3, marking this as a ridealong but not a blocker.

Comment 4

[Reed Loden [:reed] (use needinfo?)]

We had a user stop by #firefox tonight saying that he was seeing the assertions. :(

Comment 5

[Jeremy Baron]

Attachment: screenshot of the dialog this will prevent

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I filed bug 357109 to fix the bug that that dialog explains.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: 1.8 branch patch

The flag we really want for this is "release build", so "official branding" isn't really correct. For the branch, I think we should just key it off DEBUG, which is what this patch does. The branch doesn't have the patch for bug 337875, so I need to defined DEBUG manually.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 246535 [details] [diff] [review]
1.8 branch patch

Simple fix to not be obnoxious when encountering exceptional circumstances.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 246535 [details] [diff] [review]
1.8 branch patch

approved for 1.8 branch, a=dveditz for drivers

Comment 10

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/content/Makefile.in 	1.5.10.1
mozilla/toolkit/content/debug.js 	1.1.2.6

Comment 11

[Nickolay_Ponomarev]

I wonder what the plan for trunk is. I think extension developers would benefit from the full functionality of NS_ASSERT (i.e. the stack traces) even in non-debug builds. It would be nice to not introduce an additional pref for that too.

Comment 12

[Nickolay_Ponomarev]

*** Bug 330077 has been marked as a duplicate of this bug. ***

Comment 13

[David G King]

Should the Blocking1.8.1.1 flag be removed?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

No

Comment 15

[(not reading, please use [e-mail] instead)]

from joe's original checkin comment:

"NS_ASSERT() shows a dialog box containing a stack trace when it's called
with a condition that isn't true. The dialog box will be suppressed in
final builds, but in nightlies and alphas, it's useful for encouraging
bug submissions."

What about only enabling it if app.update.channel is "nightly", "beta", "default"?  (I believe "default" == "your own build", but we should double check).

You can't check if app.update.channel == "release", as partner release builds are on other channels.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> What about only enabling it if app.update.channel is "nightly", "beta",
> "default"?  (I believe "default" == "your own build", but we should double
> check).

That's a good idea, testing a patch to do that now.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Attachment: use update channel pref

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 273421 [details] [diff] [review]
use update channel pref

r=mconnor

Comment 19

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/content/debug.js 	1.7