938683 - Failure to check the return value of PR_Close() in WritePluginInfo ( mozilla/dom/plugins/base/nsPluginHost.cpp)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: 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 |WritePluginInfo()|, a file that is written plugin information 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).

Here if PR_Close() fails, all we can do is to return an error to the caller.
I pick up NS_ERROR_FAILURE as a generic error value to return.
(This may be refined by looking at the error code of PR_Close() although
PR_Close() does not seem to do a lot to specialize the error value by looking at errno, etc. regarding error value return.)

Also, showing that a fatal file system error is now left to the upper layer of the call tree.

But I want to show such a serious file system error to the user using the built-in system console, or dump message to the console where mozilla software is invoked.
Fr now, I use NS_WARNING(): but it is NOT GOOD in the sense, that it is basically NOOP in non-debug build. 

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. In that sense, NS_WARNING() is not satisfactory. 

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

PS: I noticed that there is Bug 532627 nsPluginHostImpl::WritePluginInfo should call PR_Sync before PR_Close

But in that bugzilla, the return values of both PR_Sync() and PR_Close() calls are not checked at all. Far from rock-solid style of programming.
The user ought to be told that the file may be corrupt due to failing close().
(BTW, the PR_Sync() seems to be absent from comm-central source tree version of the file.)

Comment 1

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Comment on attachment 832346 [details] [diff] [review]
Checking PR_Close() value to detect file system problem.

Oh I forgot. There is another call to PR_Close() against a file that was opened for reading. Usually, the failure to close a reading stream is benign in practice, but I still think
this ought to be checked and reported (by an error message) rather than
solely relying on the upper layer to show the warning/error message.

Not checking the value of such wrapper of important system call is a very bad coding style, I think.
 
TIA

PS: I put John Shoenick that was listed in "suggested reviwers" as review requestee.

Comment 2

[John Schoenick [:johns]]

Comment on attachment 832346 [details] [diff] [review]
Checking PR_Close() value to detect file system problem.

We should use MOZ_ASSERT(false, "PR_Close() failed") here since it is essentially an unhandled condition -- this will also make it fatal in the test harness.

For release builds we could report failures like this to nsIConsoleService, but we generally have no good practices here :(

Comment 3

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: Checking PR_Close() value to detect file system problem. (take 2) carrying over r=jschoenick+

Thank you for the review. I put MOZ_ASSERT() instead of NS_WARNING().
I will put checkin-needed keyword.

But I will also put in
[KEEP OPEN for another month or so while the clean up in Bug 936990 goes on.]
in white board.

The idea is that, after fixing a dozen similar bugs, I would learn more about how the error/warning should be
handled in "should not happen" case, and especially how such error should be shown to the real-world user: I will study nsIConsoleService mechanism for possible usage in this clean-8up work.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

For some reason, this makes bug 931642 happen with much higher frequency.
https://tbpl.mozilla.org/?tree=Try&rev=014f916d4a6c

Comment 5

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #4)
> For some reason, this makes bug 931642 happen with much higher frequency.
> https://tbpl.mozilla.org/?tree=Try&rev=014f916d4a6c

In comment 2,  John Schoenick stated
"We should use MOZ_ASSERT(false, "PR_Close() failed") here since it is essentially an unhandled condition -- this will also make it fatal in the test harness."

Right on. PR_Close() is failing for some reason, and we have ignored such conditions before.
That is not a healthy situation.
The change here caused the program to check for this previously unhandled condition in the
debug build test case, and so it means we have to look into the issue :-)

However, I have no idea where the error occurs: Oh, there seems to be stack dump, and so someone familiar with the code in 931642 can figure out I hope.

I will put my findings in Bug 931642 henceforth.

TIA

PS: So the patch here for seemingly innocuous case of unchecked return value of PR_Close()
can reveal strange error conditions. If that were indeed correct, good.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

We should probably try to land this now that bug 931642 is fixed.

Comment 7

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #6)
> We should probably try to land this now that bug 931642 is fixed.

What can I do to make this land?

TIA

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Asking is good :P

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a355b5e2d7e

Why are we keeping these bugs after they land on m-c, by the way? I would think that only bug 936990 would need to be open to track the ongoing work.

Comment 9

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> Asking is good :P
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8a355b5e2d7e
> 
> Why are we keeping these bugs after they land on m-c, by the way? I would
> think that only bug 936990 would need to be open to track the ongoing work.

Thanks.

I am not entirely familiar with the manner these related bugs are tracked.
In this particular instance, I am afraid that I must come back to the initial dozen bugs fixed
because
 - It is not clear what output mechanism to use to report a run-time error to the user.
   (In each bug mentioned in bug 936900 uses somewhat ad-hoc output routines.
    Surprisingly, well almost everyone who reviewed the patch has a different idea of
    using different output mechanism. : different functions to write something to console,
    to debug console, NO outut to STDOUT/STDERR as non-console application, etc.)
 - The change of output behavior between DEBUG and non-DEBUG version also seems to
   be perceived rather differently by each reviewer.

So it is likely that in the next several weeks, I may have to find a middle-ground by
digging up pros and cons of each report approach (where to output, the particular implementation to use, and at the same time paying attention to whether we want different behavior between DEBUG-build and non-DEBUG build, and a few other issues I learned.

So the question becomes, it is OK to re-visit these temporarily fixed bugs after CLOSING and RE-OPEN to
use a different output mechanism rather than using the one used in the checked-in patches?
If it is OK to RE-OPEN the bugs for such changes, yes, it is OK to close the individual bugs.
But if it is not OK and I should file a NEW BUG to change the output method for already patched bugs,
I thought it may be wise to let the initial dozen bugs to be kept open for another month so that I may be able to retrofit new insight and new patch to already patched places. (just for the sake of decreasing similar bug entries.)

TIA

Comment 10

[Ryan VanderMeulen [:RyanVM]]

The general plan is to use one bug per issue, so if the original fix landed doesn't resolve the issue, reopening is perfectly acceptable.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a355b5e2d7e