938687 - Failure to check the return value of PR_Close() in CloseFile() ( mozilla/dom/ipc/TabParent.cpp )

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: (Work in progress patch) checking PR_Close() return value in CloseFile() and issue warning.

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 CloseFile(), the return value of PR_Close() is not checked.
It is not possible to return the error value since the function CloseFile() is void.
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.)

So at least the error should be caught and error/warning be issued.

As for warning, I am not sure if fprintf(stderr,"ERROR: PR_Close() returned 0x%08x\n", prrc) or some such is better than NS_WARNING() which does not show the error value, and is NOOP in non-debug build. 
The proper error/warning is most useful for real-world user who experience such
errors during TB/FF operation.
So I wonder if using fprintf() is better than NS_WARNING(). Suggestions are welcome.

I am attaching a patch (against the file in comm-central.)

TIA

Comment 1

[Benjamin Smedberg]

Comment on attachment 832356 [details] [diff] [review]
(Work in progress patch) checking PR_Close() return value in CloseFile() and issue warning.

1) if ( always needs a space
2) Please add braces around the if block:

if (...) {
  NS_ERROR(...);
}

r=me with those changes

Comment 2

[ISHIKAWA, Chiaki]

Attachment: (Patch Take 2) checking PR_Close() return value in CloseFile() and issue warning: carrying over r=bsmedberg+

Thank you for your review. I am uploading the patch that addresses the issues in your comment. 
I will put in checkin-needed keyword.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/163279f146b9

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/163279f146b9