As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 612093 - PR_ASSERT should not be silent on Windows
: PR_ASSERT should not be silent on Windows
Status: RESOLVED FIXED
:
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: other
: x86 Windows 7
: -- normal (vote)
: 4.8.7
Assigned To: Jesse Ruderman
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-14 00:52 PST by Jesse Ruderman
Modified: 2010-11-16 12:04 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (723 bytes, patch)
2010-11-14 00:52 PST, Jesse Ruderman
no flags Details | Diff | Splinter Review
patch v2 (647 bytes, patch)
2010-11-15 23:30 PST, Jesse Ruderman
wtc: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2010-11-14 00:52:34 PST
Created attachment 490448 [details] [diff] [review]
patch

PR_ASSERT's silence confuses my fuzzer into thinking that Firefox exited suddenly for no reason at all.  (In case you're curious, I'm hitting bug 345094.)

wtc suggested this fix long ago (bug 183952 comment 5) and timeless objected for a reason I do not understand.

See bug 561672 for why I included an fflush.
Comment 1 User image Nelson Bolyard (seldom reads bugmail) 2010-11-14 12:35:47 PST
Jesse, On MSWindows, programs built to be "Windows" programs (as opposed to 
those built to be "console" programs, e.g. Unix-like filters) close stdin,
stdout and stderr soon after startup.  All Mozilla's programs on MSWindows
are Windows programs.  Consequently, the write and flush you propose to do 
to stderr will be writes to closed file descriptors on windows, when used 
in "Windows" (not "console") type programs.  Your proposed change would work
OK in (say) NSS and NSPR command line test programs, all of which are 
"console" programs, but would do nothing (or worse) in Mozilla products on
MSWindows.

In light of that fact, do you really want to go ahead with this change?
Comment 2 User image Jesse Ruderman 2010-11-14 13:46:36 PST
If I run a debug build of Firefox from the command line, I see plenty of stdout and stderr output.  I tested the patch and it fixed a bug on Windows 7.  So I don't know what you're talking about.
Comment 3 User image Nelson Bolyard (seldom reads bugmail) 2010-11-14 13:54:09 PST
I personally had the experience of adding lots of debug printf to stderr, 
only to see it come to naught on Windows.  But that was years ago.  I'm 
assuming it's still true, but maybe this has changed in the last 8 years.  
It was certainly true for a long time.
Comment 4 User image timeless 2010-11-15 01:16:21 PST
jesse: in this case I object to you not pushing on in my bug. 

nelson: debug builds are console apps which is why jesse's "solution" would "fix" his version of the problem. And why it wouldn't help anyone else.

*** This bug has been marked as a duplicate of bug 183952 ***
Comment 5 User image Jesse Ruderman 2010-11-15 11:41:50 PST
The current patch in bug 183952 does not look like it fixes this bug.
Comment 6 User image Wan-Teh Chang 2010-11-15 13:48:54 PST
Comment on attachment 490448 [details] [diff] [review]
patch

You can just remove this ifdef:

>+#if defined(XP_UNIX) || defined(XP_OS2) || defined(XP_BEOS) || defined(WIN32)

as it covers all the platforms.

>     fprintf(stderr, "Assertion failure: %s, at %s:%d\n", s, file, ln);
>+    fflush(stderr);

I believe we didn't add an fflush() because stderr is not buffered.
Comment 7 User image Jesse Ruderman 2010-11-15 16:32:38 PST
I needed fflush in JS_Assert (bug 561672), but maybe I don't need it here.
Comment 8 User image Jesse Ruderman 2010-11-15 23:28:16 PST
The fflush is needed here.  I tested with automation.py and a testcase from bug 345094 on Windows 7.  Without the fflush, the assertion message didn't come through.
Comment 9 User image Jesse Ruderman 2010-11-15 23:30:42 PST
Created attachment 490823 [details] [diff] [review]
patch v2

This version of the patch removes the ifdef.
Comment 10 User image Wan-Teh Chang 2010-11-16 12:04:05 PST
Comment on attachment 490823 [details] [diff] [review]
patch v2

r=wtc.

Patch checked in on the NSPR trunk (NSPR 4.8.7).

Checking in prlog.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v  <--  prlog.c
new revision: 3.53; previous revision: 3.52
done

Note You need to log in before you can comment on or make changes to this bug.