Closed
Bug 938693
Opened 11 years ago
Closed 8 years ago
Failure to check the return value of PR_Close() in ~FileBlockCache() ( mozilla/content/media/FileBlockCache.cpp)
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: ishikawa, Assigned: ishikawa)
References
(Blocks 1 open bug)
Details
(Whiteboard: [KEEP OPEN for another month or so while the clean up in Bug 936990 goes on.])
Attachments
(1 file, 1 obsolete file)
971 bytes,
patch
|
ishikawa
:
review+
|
Details | Diff | Splinter Review |
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). In ~FileBlockCache(), the return value of PR_Close() is not checked. It is not possible to return the error value since ~FileBlockCache() does not return a value. However, a failure to close a file properly usually indicates that the file system is in a serious error condition such as - transient and/or semi-permanent network error during a remote file mount (CIFS, NFS, etc.), or - malfunctioning storage device (such as worn out SD, etc.), - other system error. So at least the error should be caught and error/warning be issued. I am attaching a patch. I am temporarily is using NS_WARNING(), but I think maybe a direct fprintf(stderr,"ERROR: PR_Close() returned 0x%08x", prrc) or some such may be better since NS_WARNING() is NOOP in non-debug build, and it is not easy to print error code that may indicate a specialized error condition which is useful for debugging. So suggestion welcome. I am attaching a patch that is created against the file in comm-central. TIA
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ishikawa
Assignee | ||
Updated•11 years ago
|
Attachment #832366 -
Flags: review?(mounir)
Updated•11 years ago
|
Attachment #832366 -
Flags: review?(mounir) → review?(cpearce)
Comment 1•11 years ago
|
||
Comment on attachment 832366 [details] [diff] [review] (Work in progress patch) checking PR_Close() return value and issue warning when it failed. Review of attachment 832366 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/FileBlockCache.cpp @@ +50,5 @@ > if (mFD) { > + PRStatus prrc; > + prrc = PR_Close(mFD); > + if(prrc != PR_SUCCESS) > + NS_WARNING("PR_Close() failed."); if (prrc != PR_SUCCESS) { NS_WARNING("PR_Close() failed."); } (space between "if" and "(", and {} around single line if statements.)
Attachment #832366 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 2•11 years ago
|
||
Thank you for your review. I modified the patch as suggested.
Attachment #832366 -
Attachment is obsolete: true
Attachment #832662 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Whiteboard: [KEEP OPEN for another month or so while the clean up in Bug 936990 goes on.]
Assignee | ||
Comment 3•11 years ago
|
||
I put the checkin-needed keyword and a comment in white board saying [KEEP OPEN for another month or so while the clean up in Bug 936990 goes on.] The idea is that after fixing a dozen similar bugs, I would have better grasp of how to show the error/warning of this rare but valid error to the interactive users. NS_WARNING() is noop in non-debug build, and is not useful for a real-world user bitten by failing file system: such a user needs to learn the error first-hand quickly from a console log, etc. TIA
Comment 4•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/71fd503d1b68
Keywords: checkin-needed
Comment 5•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/71fd503d1b68
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•