Closed Bug 355195 Opened 13 years ago Closed 13 years ago

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

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 3 alpha7

People

(Reporter: Gavin, Assigned: Gavin)

References

()

Details

(Keywords: fixed1.8.1.1)

Attachments

(1 file, 3 obsolete files)

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.
Flags: blocking-firefox2?
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.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
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.
Per Gavin's evaluation in comment 3, marking this as a ridealong but not a blocker.
Flags: blocking-firefox2? → blocking-firefox2-
Whiteboard: [rc ridealong]
We had a user stop by #firefox tonight saying that he was seeing the assertions. :(
Flags: blocking1.8.1.1?
Attachment #241021 - Flags: approval1.8.1.1?
Attachment #241021 - Flags: approval1.8.1.1?
I filed bug 357109 to fix the bug that that dialog explains.
Attachment #241021 - Attachment is obsolete: true
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Attached patch 1.8 branch patch (obsolete) — Splinter Review
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.
Attachment #242608 - Attachment is obsolete: true
Attachment #246535 - Flags: review?(benjamin)
Whiteboard: [rc ridealong] → [rc ridealong] needs r=bsmedberg
Attachment #246535 - Flags: review?(benjamin) → review+
Comment on attachment 246535 [details] [diff] [review]
1.8 branch patch

Simple fix to not be obnoxious when encountering exceptional circumstances.
Attachment #246535 - Flags: approval1.8.1.1?
Whiteboard: [rc ridealong] needs r=bsmedberg
Comment on attachment 246535 [details] [diff] [review]
1.8 branch patch

approved for 1.8 branch, a=dveditz for drivers
Attachment #246535 - Flags: approval1.8.1.1? → approval1.8.1.1+
mozilla/toolkit/content/Makefile.in 	1.5.10.1
mozilla/toolkit/content/debug.js 	1.1.2.6
Keywords: fixed1.8.1.1
Target Milestone: Firefox 2 → Firefox 3 alpha1
Version: 2.0 Branch → Trunk
Flags: in-testsuite-
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.
*** Bug 330077 has been marked as a duplicate of this bug. ***
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 alpha1 → Firefox 3 alpha6
Should the Blocking1.8.1.1 flag be removed?
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.

Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
(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.
Priority: -- → P1
Attachment #246535 - Attachment is obsolete: true
Attachment #273421 - Flags: review?(mconnor)
Whiteboard: [patch-r?][needs review mconnor]
Comment on attachment 273421 [details] [diff] [review]
use update channel pref

r=mconnor
Attachment #273421 - Flags: review?(mconnor) → review+
Whiteboard: [patch-r?][needs review mconnor] → [checkin needed]
Keywords: checkin-needed
Whiteboard: [checkin needed]
mozilla/toolkit/content/debug.js 	1.7
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.