Closed Bug 939531 Opened 11 years ago Closed 11 years ago

Failure to the check the return value of PR_Close() in ~FileAutoCloser() (js/xpconnect/loader/mozJSComponentLoader.cpp)

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: ishikawa, Assigned: ishikawa)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

"Checking PR_Close() value to detect file system problem." This is part of an effort to make mozilla software rock-solid. (See bug 936990 (META) The return value of PR_Close() should be checked (like any other system calls). Or at least, let the software report a system call error related to file system problem as it occurs. In |~FileAutoClose()|, a file is closed at the end, but the value of PR_Close() is not checked. It should. The data that is "written" by |write| calls in previous statements may end up only in the system cache/buffer, and only when PR_Close() is called, the data is finally written to the file device (and it may not reach the device until later depending on system flush policy.) Now, if the file opened for writing is on a remote file system such as CIFS-share or NFS-share, error on close can occur due to transient or semi-permanent network error. (Or maybe the file is written to a broken device such as worn-out SD media or MMC.) I am attaching a patch. (produced against the file in comm-central). Error value to return: Here if PR_Close() fails, all we can do is to show the an error to the caller. Showing the error to the user: I want to show such a serious file system error to the user using the built-in system console, or dumping message to the console where mozilla software is invoked. Here, I use MOZ_ASSERT(). There may be a different choice here. Consider a situation where a user in the real-world is dealing with such system-related error as failing file system and requests for help in newsgroup/mailing list, developers need to know the precise error code and where the error seems to happen. I wonder if fprintf(stderr,"PR_Close() failed with 0x%08x\n", prrc) or some such is preferable. I would like to hear people's opinion. Anyway I am attaching a patch against the file in comm-central. TIA
Summary: Failure to the check the return value of PR_Close() in ~FileAutoCloser() (js/xpconnect/loader/mozJSComponentLoader.cpp)mozilla/media/mtransport/nr_socket_prsock.h) → Failure to the check the return value of PR_Close() in ~FileAutoCloser() (js/xpconnect/loader/mozJSComponentLoader.cpp)
Attachment #833462 - Flags: review?(bobbyholley+bmo)
Assignee: nobody → ishikawa
Comment on attachment 833462 [details] [diff] [review] (Work-inProgress) Checking PR_Close() return value. MOZ_ASSERT will crash debug builds, and do nothing for opt builds. This probably isn't what you want. You probably want fprintf(stderr, msg, ...) I don't really have an opinion on whether we want this in general, though. Maybe bsmedberg does.
Attachment #833462 - Flags: review?(bobbyholley+bmo) → review-
Flags: needinfo?(benjamin)
(In reply to Bobby Holley (:bholley) from comment #1) > Comment on attachment 833462 [details] [diff] [review] > (Work-inProgress) Checking PR_Close() return value. > > MOZ_ASSERT will crash debug builds, and do nothing for opt builds. This > probably isn't what you want. You probably want fprintf(stderr, msg, ...) > Thank you for pointing this out. This is not what I want. I think the user who encounters file system error that eats TB/FF data would appreciate to learn the error code printed for diagnosing the issue or for that matter realize the problem ASAP. > I don't really have an opinion on whether we want this in general, though. > Maybe bsmedberg does. Obviously there are not easy to follow guidelines since - some functions return values and the upper-layer routine can, in principle, decide to show warning/error messages, - some functions don't return values, and as of now simply ignoring error return of low-level routine (such as this function ~FileAutoCloser()), - other variants, etc. For case 2 above, I can only think of printing the error value if there is any error. I hope there aren't. But if there is unexpected error, we may want to change code to handle at least some errors more gracefully (with better propagation of errors to the upper-layer routines, or by showing error status to the user in an explicit manner.) My trying to dump the error code to console is an initial attempt. I will roll a new patch with better output characteristics, which may be fprintf() after a consideration. TIA TIA
Release builds should not print anything to stdout/stderr, as a rule. We are not a console application. Since this code is only reading files, I don't see why we'd want to warn anybody about this condition.
Flags: needinfo?(benjamin)
Thanks benjamin.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: