Closed
Bug 355195
Opened 15 years ago
Closed 14 years ago
"temporary testing assertions" that were supposed to be disabled before the release haven't been disabled
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 3 alpha7
People
(Reporter: Gavin, Assigned: Gavin)
References
()
Details
(Keywords: fixed1.8.1.1)
Attachments
(1 file, 3 obsolete files)
1.48 KB,
patch
|
mconnor
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•15 years ago
|
Flags: blocking-firefox2?
Assignee | ||
Comment 1•15 years ago
|
||
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
Assignee | ||
Comment 2•15 years ago
|
||
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•15 years ago
|
||
Per Gavin's evaluation in comment 3, marking this as a ridealong but not a blocker.
Flags: blocking-firefox2? → blocking-firefox2-
Whiteboard: [rc ridealong]
Comment 4•15 years ago
|
||
We had a user stop by #firefox tonight saying that he was seeing the assertions. :(
Flags: blocking1.8.1.1?
Updated•15 years ago
|
Attachment #241021 -
Flags: approval1.8.1.1?
Comment 5•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #241021 -
Flags: approval1.8.1.1?
Assignee | ||
Comment 6•15 years ago
|
||
I filed bug 357109 to fix the bug that that dialog explains.
Assignee | ||
Updated•15 years ago
|
Attachment #241021 -
Attachment is obsolete: true
Updated•14 years ago
|
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Assignee | ||
Comment 7•14 years ago
|
||
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)
Updated•14 years ago
|
Whiteboard: [rc ridealong] → [rc ridealong] needs r=bsmedberg
Updated•14 years ago
|
Attachment #246535 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 8•14 years ago
|
||
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?
Updated•14 years ago
|
Whiteboard: [rc ridealong] needs r=bsmedberg
Comment 9•14 years ago
|
||
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+
Assignee | ||
Comment 10•14 years ago
|
||
mozilla/toolkit/content/Makefile.in 1.5.10.1 mozilla/toolkit/content/debug.js 1.1.2.6
Updated•14 years ago
|
Flags: in-testsuite-
Comment 11•14 years ago
|
||
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•14 years ago
|
||
*** Bug 330077 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•14 years ago
|
Flags: blocking-firefox3?
Updated•14 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Updated•14 years ago
|
Target Milestone: Firefox 3 alpha1 → Firefox 3 alpha6
Comment 13•14 years ago
|
||
Should the Blocking1.8.1.1 flag be removed?
Assignee | ||
Comment 14•14 years ago
|
||
No
Comment 15•14 years ago
|
||
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.
Updated•14 years ago
|
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Assignee | ||
Comment 16•14 years ago
|
||
(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
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #246535 -
Attachment is obsolete: true
Attachment #273421 -
Flags: review?(mconnor)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patch-r?][needs review mconnor]
Comment 18•14 years ago
|
||
Comment on attachment 273421 [details] [diff] [review] use update channel pref r=mconnor
Attachment #273421 -
Flags: review?(mconnor) → review+
Updated•14 years ago
|
Whiteboard: [patch-r?][needs review mconnor] → [checkin needed]
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [checkin needed]
Assignee | ||
Comment 19•14 years ago
|
||
mozilla/toolkit/content/debug.js 1.7
You need to log in
before you can comment on or make changes to this bug.
Description
•