This bugzilla entry is about > 2. Mostly C-C T-B fixes to take care of short read. in the bug 1170564 which this bug blocks. Please read the following to learn what "short read" means. The attached patch is a Work-In-Progress (and it depends on the patches that is indirectly mentioned by [1] below. That is there are many files in C-C tree to fix I/O error handling (or the lack of it) mentioned in [1] and other bugzilla entries mentioned there in. The patch uploaded here is produced ON TOP THE CHANGED FILES thusly and so it may not apply to the C-C TB tree directly. We need apply the patches one by one (I will summarize the order) once the general idea of the patch is deemed OK. TIA TIA +++ This bug was initially created as a clone of Bug #1170564 +++ After spending some time to weed out the failure to detect I/O errors from C-C TB code during message download [1], I have come to realize there are issues regarding short read. Short read refers to |read| system call returning without the full number of requested octets although the subsequent |read| calls would return the missing octets. That is, short read returns prematurely. Short read can happen during a read operation against a remotely mounted file. High-load on the remote server, or a transient network issue can cause such short read. It is not a stranger to FF developers. Network code is full of cautious programming to take care of such short read. Unfortunately, C-C TB (under linux at least) fails to handle the short read very well and as a result we see errors which I noticed during tests. So I set out to identify and fix these issues and this is a meta entry to track such errors. It turns out that performing additional writes after a short-write is automagically handled by |pt_Write|. Isn't it great? Then why not pt_Read? As I explain later in this post, we sometimes don't know how many we should read in advance. That's why. Short read simulated: I have come to learn that mounting one's profile to remote file server seems to cause strange errors. Since I don't have a CIFS server that seems to cause such short read very often under heavy I/O work load, I resort to simulation. I inject short read by preparing some functions such as |read| and |read64| that simulates short read and preload them using "export LD_PRELOAD=that-file.so" before running C-C thunderbird to find errors caused by short read. How to FIX after identifying the trouble spot: When you deal with short read issues there are a few things that we must keep in mind. At the level of |read| system call, we have no idea whether a short read ought to be followed by additional |read| or not. Then where should we make the decision? It depends on the type of |read| operation. There are two types of |read| operations. (a) # of octets to read is known in advance. The caller of |read| knows exactly that there are certain number of octets to be read in advance. E.g. (1) we check the size of JSON file by calling |stat| in advance and try to read that many octets in one |read|. [As it turns out this is a real-world problem.] (2) we try to read a "block" of a known data structure and sizeof(block) is already known in advance (at compile time, etc). A case in point is the "block" for CACHE operation. (There was an issue and it was fixed.) In these cases, we ought to perform additional |read| operations until all the expected number of octets are read (or until error condition or EOF is encountered.) in the face of short read. So we must provide a mechanism for such additional reads. In my patch, I have introduced a couple of variants of |Read| operation and |PR_Read| that performs additional read operations if we failed to read the desired # of octets in one run until all the octets are read. (b) # of octets to read is not known in advance. E.g., (1) There is a mime-encoded data in a file (as in attachment), and the program needs to read as many data as possible until the end of the data marker line is reached. (2) In general, suppose there is a data structure written in ASCII representation in a file, and also suppose the caller of |read| can know the end of the logical data structure by reading a certain textual construct that signals the end of the data. In this case, the caller of |read| needs to perform the |read| repeatedly until this end of data marker is read. Case (1) is obviously a particular case of (2). (3) There are similar variants. In these cases, we can NOT rely on the number of octets to read in advance. The size of the buffer passed to |read| is just an estimate, a ball park figure. We may have more data to read or much less data to read actually. We ought to perform additional |read| until the end of data marker is reached (or the error is signaled or EOF is reached.) Between (a) and (b), the main difference is case (a) knows the fixed number of octets to be read in advance, and case (b) does not know it. In practice, |read| calls of case (b) tend to happen inside loops and are relatively easy to spot, and no modification is necessary most of the time even if short read has to be considered. (Well there are cases where the ERROR condition was not detected at all in the current code and I tried to fix them.) The short read in case (a) needs to be handled at the spot where the required number of octets is known in the call chain. At the |read| system call level, we have no idea whether a particular short |read| ought to be followed by additional reads at all. To figure out where such problematic short read is encountered, and to learn whether the |read| is type (a) or type (b) above, I had to dump stack trace to figure out where such short read is NOT followed by necessary additional read operations. Using the stack trace, I tried to figure out where the decision to perform additional reads can be made and, if necessary, replaced the read with a version that performs such additional read operations. I think I explained enough background. There will be four bugzilla entries I intend to submit today and block this meta entriy. 1. NSS "dbm" code used for certificate and key handling. (Already reported in newsgroup/mailing list.) 2. Mostly C-C T-B fixes to take care of short read. 3. A few M-C fixes. Yes, there *ARE* places where short read is not handled in M-C! I was surprised myself. Maybe storing one's profile under remote file server was an arcane idea when mozilla code base was written years ago. But today, in corporate setup, PC is often used like a thin client and everything including your profile may be stored on a central server. 4. Case of reading JSON file from JavaScript. There is a known bug which I don't know how to fix. It is filed as a separate bugzilla. It is concerned with a reading of JSON file from JavaScript. There are a few more unresolved cases that manifest themselves during local |make mozmill| test suite operation with simulated short read injection, but I am not sure if they are real and not timing-dependent random errors that are seen before. [Yes, there are such random errors in |make mozmill|.] I will add them to known C-C T-B fixes and M-C fixes *iff* they turn out to be real bugs in the future. TIA [1] Bug 1121842 - [META] RFC: C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends.

I forgot to mention: the patch uploaded was tested by |make mozmill| with and without the simulated short read injection and the patch did not seem to add new errors (other than the usual random error that sometime show up during |make mozmill| test.) TIA

This is an update patch to match the tree. This is going to be in a series of patches. ======================================== PLANNED ORDER ======================================== * 1 A check_fileptr_change.patch: Bug 1116055: Check the unexpected change of file seek position and ask the user to report it bug 1116055 This was considered a necessary step so that we can know that if any user experiences a strange seek position change. * 2 A fix-close-step-1.patch: Bug 1122698 Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends (part 1 of a series) bug 1122698 * 3 A fix-close-part-2.patch: Bug 1134527 Add error value checking of Close() and Flush() (part-2 of a series) bug 1134527 * 4 A fix-close-part-3-write-error-check.patch: Bug 1134529: Check Write failures (part-3 of a series) bug 1134529 * 5 A removing-a-Seek.patch: C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (step 4) bug 1174500 [was mistyped as 1117450 in a few bugzilla entries. :-( ] 6 A 1116055-acelist.patch: One liner patch posted in Bug 1116055 to return a buffered stream from mbox::getNewOutputStream by aceman Bug 1176857 - C-C TB Enabling buffering for pop3 download (and possibly part of imap file operation) I have created a separate bug for this since it will be necessary to apply this patch only after other patches landed. (This is because enabling the buffering requires proper error checking of |Close()| and |Flush()|. These are only taken care of AFTER 1-5 patches above are applied.) Maybe about a month after the first patch (bug 1116055) above is applied, and other patches 2-5 have been applied, I can enable this patch. (Assuming there is NO ONE who reports strange seek position change on their computer.) 7 A add-short-read-handling.patch: Bug 1170606 - C-C T-B fixes to take care of short read Bug 1170606 This is theoretically independent of 1-6, but my local patch assumes the changes in 1-6 to be in place. (The patch may not apply cleanly without 1-6 in advance.) There are some additional shor-read issues UNDER M-C portion of the source tree. They can be applied indepdent of this series. Please feel free to take up the review or move it to somebody else based on the workload of the potential reviewers.

Comment on attachment 8625470 [details] [diff] [review] Fixes to handle short read correctly [take 2] (with a few handling of Seek and Write errors thrown in.) Review of attachment 8625470 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, but I'm not a good reviewer for this. Maybe neil, rkent or jcranmer?

Comment on attachment 8625470 [details] [diff] [review] Fixes to handle short read correctly [take 2] (with a few handling of Seek and Write errors thrown in.) Review of attachment 8625470 [details] [diff] [review]: ----------------------------------------------------------------- I will admit that I have not taken the time to test this patch locally; I have only looked at the summary diff so far. Unfortunately, I do not think this patch is quite up to the quality we expect. Heavy use of XXX comments and lots of writes to stderr give this code a feel of a temporary debugging stage. Also this patch needs to be broken up--it touches so much disparate code, and has such non-trivial effects, that I would need to digest in much smaller chunks. Having a single patch makes it hard for me to do that. ::: mailnews/base/util/nsMsgUtils.cpp @@ +75,5 @@ > #include "mozilla/Services.h" > #include "locale.h" > > +// to include NS_ERROR_INTERRUPT (just in case?) > +#include "nsError.h" There is no need to include nsError.h. It is guaranteed to be transitively included by nearly every header in the prior list. @@ +164,5 @@ > + { > + /* XXX may want to repeat read returns if errno == EINTR */ > + /* xxx I am not sure if WIN32 has errno. */ > +#if !defined(WIN32) > + if(NS_ERROR_INTERRUPT == NSRESULT_FOR_ERRNO()) Calling NSRESULT_FOR_ERRNO is a very big violation of the abstraction principle. errno is set by the last system call made, and there is no reason to presume that the ->Read call of a generic input stream is the one that made that system call. Also, based on a short perusal of Linux manpages, this loop is dangerous: if a socket times out, EINTR is returned, and this could cause an infinite loop. @@ +177,5 @@ > + } > + else > + { > + remaining -= n; > + start += n; Incrementing the start variable means that start is no longer the start of the buffer. @@ +187,5 @@ > + fflush(stdout); > + fprintf(stderr, "(debug) |input->Read| repeated in busy_beaver_read().\n"); > + } > +#endif > + } This feels like a great place to put an assertion that the amount really read should be the same as the amount requested to read. ::: mailnews/base/util/nsMsgUtils.h @@ +39,5 @@ > #define MSGS_URL "chrome://messenger/locale/messenger.properties" > > //These are utility functions that can used throughout the mailnews code > > +nsresult busy_beaver_Stream_Read(nsIInputStream *input, void *cp, uint32_t len, uint32_t *really_read); The name of this method is in flagrant violation of coding styles, and it's also a very bad name because it is completely not clear what is meant by the function name. A much better name would be along the lines of FullyReadStream. (Note also that we use CamelCase, not snake_naming in Mozilla code). In addition, the cp parameter should be a char * instead of void *, and this needs a Doxygen-style documentation comment to explain that it will either fill the buffer completely or return an error message. @@ +579,5 @@ > member = aValue; \ > return NS_OK; \ > } > > +#define MSG_REPORT_ERROR(r, msg) \ This macro should never be used--NS_WARNING would be preferable.

(In reply to Joshua Cranmer [:jcranmer] from comment #4) > Comment on attachment 8625470 [details] [diff] [review] > Fixes to handle short read correctly [take 2] (with a few handling of Seek > and Write errors thrown in.) > > Review of attachment 8625470 [details] [diff] [review]: > ----------------------------------------------------------------- > > I will admit that I have not taken the time to test this patch locally; I > have only looked at the summary diff so far. > > Unfortunately, I do not think this patch is quite up to the quality we > expect. Heavy use of XXX comments and lots of writes to stderr give this > code a feel of a temporary debugging stage. Re: xxx comment. There are a few cases where I was not really sure what the code does. But for other places, where the "FullyReadStream" is NOT needed, I left a comment so that the future code reviewer does not check whether such function is necessary. I.e., I left a mark that the |stream->Read()| on the next line has been checked and we don't need "FullyReadStream". Maybe I don't have to use "xxx" for the one line comment. Or maybe use a different moniker: "short read allowed"? > > Also this patch needs to be broken up--it touches so much disparate code, > and has such non-trivial effects, that I would need to digest in much > smaller chunks. Having a single patch makes it hard for me to do that. I could split this into directory-by-directory patches. What do you think? > ::: mailnews/base/util/nsMsgUtils.cpp > @@ +75,5 @@ > > #include "mozilla/Services.h" > > #include "locale.h" > > > > +// to include NS_ERROR_INTERRUPT (just in case?) > > +#include "nsError.h" > > There is no need to include nsError.h. It is guaranteed to be transitively > included by nearly every header in the prior list. I will remove it and see if it compiles. > @@ +164,5 @@ > > + { > > + /* XXX may want to repeat read returns if errno == EINTR */ > > + /* xxx I am not sure if WIN32 has errno. */ > > +#if !defined(WIN32) > > + if(NS_ERROR_INTERRUPT == NSRESULT_FOR_ERRNO()) > > Calling NSRESULT_FOR_ERRNO is a very big violation of the abstraction > principle. errno is set by the last system call made, and there is no reason > to presume that the ->Read call of a generic input stream is the one that > made that system call. I should probably figure out how to preserve the errno set by I/O system call and use that here, but > Also, based on a short perusal of Linux manpages, this loop is dangerous: if > a socket times out, EINTR is returned, and this could cause an infinite loop. This is a great observation. I have not have experienced a problem here, but in a change to M-C side of the code, I *DID* see a strange infinite loop. I suspect it is a network-time out issue. (Any chance if you recall the issue number of linux magazine handy?) Beside the dependency issue, I will remove this retry. (Even without this retry, the chance of successful full read will go up with this patch significantly. Also, we can/should log suspicious error. [the topic comes up later.]) > @@ +177,5 @@ > > + } > > + else > > + { > > + remaining -= n; > > + start += n; > > Incrementing the start variable means that start is no longer the start of > the buffer. Retry will be removed. > @@ +187,5 @@ > > + fflush(stdout); > > + fprintf(stderr, "(debug) |input->Read| repeated in busy_beaver_read().\n"); > > + } > > +#endif > > + } > > This feels like a great place to put an assertion that the amount really > read should be the same as the amount requested to read. Indeed it is a good idea. I will do so. > ::: mailnews/base/util/nsMsgUtils.h > @@ +39,5 @@ > > #define MSGS_URL "chrome://messenger/locale/messenger.properties" > > > > //These are utility functions that can used throughout the mailnews code > > > > +nsresult busy_beaver_Stream_Read(nsIInputStream *input, void *cp, uint32_t len, uint32_t *really_read); > > The name of this method is in flagrant violation of coding styles, and it's > also a very bad name because it is completely not clear what is meant by the > function name. A much better name would be along the lines of > FullyReadStream. (Note also that we use CamelCase, not snake_naming in > Mozilla code). > > In addition, the cp parameter should be a char * instead of void *, and this > needs a Doxygen-style documentation comment to explain that it will either > fill the buffer completely or return an error message. I will change the function parameter and add the comment as suggested. > @@ +579,5 @@ > > member = aValue; \ > > return NS_OK; \ > > } > > > > +#define MSG_REPORT_ERROR(r, msg) \ > > This macro should never be used--NS_WARNING would be preferable. I will re-think the error macro usage. For the final build, I want only the real serious error reported in error console for users to know that something serious has happened on their computer and they can use the information when report any strange thunderbird behavior. The logged inforamtion should be useful developers. For the debug build, I want some additional warning/information level warning (clearly marked as such info/warn, etc.) in the error console to learn that some safeguard in case of possible error I/O operations DO work, etc. I will begin by splitting the patches first. Please let me know if directory-by-directory split would be fine. TIA

(In reply to ISHIKAWA, Chiaki from comment #5) > I could split this into directory-by-directory patches. > What do you think? Yes, that would be fine. > (Any chance if you recall the issue number of linux magazine handy?) man 7 signal, search for EINTR. Also can be found, e.g., <http://man7.org/linux/man-pages/man7/signal.7.html>.

(In reply to Joshua Cranmer [:jcranmer] from comment #6) > (In reply to ISHIKAWA, Chiaki from comment #5) > > I could split this into directory-by-directory patches. > > What do you think? > > Yes, that would be fine. Will do. > > > (Any chance if you recall the issue number of linux magazine handy?) > > man 7 signal, search for EINTR. Also can be found, e.g., > <http://man7.org/linux/man-pages/man7/signal.7.html>. I will look into it. Socket angle has never occurred to me. TIA

Hi, I had a serious problem with my hardware since this summer which has finally been fixed in the last couple of weeks. My development PC seems to be working without major issue for the last couple of weeks. So I will look into this and other bug reports which I filed and was about to fix. TIA

This is the first of a series of patches that are the division of original monolithic patch into ones that addresses file under a certain directory only.

Fixes for files under mailnews/import

Fixes for files under mailnews/base

fixes for files under mailnews/compose

fixes for files under mailnews/imap

fixes for files under mailnews/local

Fixes for files under mailnews/mime

fixes for files under other directories

These patches have to come after the patches to enable buffering. I will update those patches before requesting review. TIA

This was posted to Bug 1116055 comment 107 https://bugzilla.mozilla.org/show_bug.cgi?id=1116055#c107 Excerpts from the post that are relevant to this bugzilla. ======================================== Updated List of patches and rough road map. ======================================== The following is the updated list of patches in Feb 2016 (with exact bugzilla #, etc.) regarding the enabling of buffering to make message writing faster in C-C TB and related necessary error checking of low-level I/O routines. On top of that, a measure to handle short-read that may be introduced by read against remote file system is introduced. ... ======================================== PLANNED ORDER of landing patches ======================================== bug 1116055 -> bug 1242030 (consolidation of 1122698, 1134527, 1134529, 1174500) -> bug 1242042 -> bug 1176857 -> bug 1242046 -> bug 1242050 -> bug 1170606 ======================================== An overview of patches in bugzilla entries. ---------------------------------------- [In February 2016, I fixed the errors observed ONLY UNDER WINDOWS when we use Maildir format for storage. Thus I needed to tweak my patch set somewhat and added a patch to take care of this issue and so I updated this write-up.] ... [11] Bug 1170606 add-short-read-handling.patch: Bug 1170606 - C-C T-B fixes to take care of short read This patch set *could* be theoretically independent of [1]-[9], and [10]. But my patch application order assumes the changes in [1]-[9] to have landed first. (The patches in bug 1170606 may not apply cleanly without [1]-[9] in advance. The files and lines touched overlap significantly. ) Also, note that some missing I/O error checks, i.e., missing from the early patches in [1]-[9] above, have been included in this patch set during more than 12 months of testing since I started working on the general issue of ineffective output buffering that slows down C-C TB. Simply stated, the current code sucks so badly regarding the failure to check the return code of I/O and low-level routines that creating a neat set of patches is IMPOSSIBLE. You read a dozen lines of code and realize it fails to test return values of some low-level functions. This happens almost always when I check certain part of files in detail:-( This situation is pathetic, but has to be dealt with to have a "rock solid mail client (tm)" :-) [But seriously, after analyzing the code in detail for the last 12 months, my hope is very distant now.] The patch in bug 1170606 has been split into smaller patch chunks that address files in one directory or two at a time. add-short-read-handling-base.patch add-short-read-handling-IMPORT-directory.patch add-short-read-handling-BASE-directory.patch add-short-read-handling-COMPOSE-directory-part-1.patch add-short-read-handling-IMAP-directory.patch add-short-read-handling-LOCAL-directory.patch add-short-read-handling-MIME-directory.patch add-short-read-handling-OTHER-directory.patch There are some additional short-read issues UNDER M-C portion of the source tree. They can be applied independent of this series and I am not mentioning them here. *** additional comment: I made an incorrect short-read patch for M-C which rendered OSX binary useless, and it was backed out. Bug 1170646 - A few M-C fixes to handle short read in Cache code ( from [META] Failure to deal with short read) I have no idea why linux version did not suffer from a similar problem. But even under OSX, only a version of SDK seemed to suffer from the issue and not another version. I may revisit it in a few weeks now that OSX debug build succeeds on try-comm-server (back then I could not, and did not assume OSX version of PR_Read() behaved slightly differently from linux's.) My understanding of nsIInputStream Read() semantics at least is better now. Read() returns 0=NS_OK when it encounters EOF without reading an octet whereas typically read() returns EOF under POSIX when it tries to read it. There may be another subtle semantic issue regarding PR_Read() that was handled in bug 1170646, and I may have introduced a subtle mix-up here, or that the OSX SDK have been updated. Here again my confusion and subsequent debugging were not wasted. I found a few interesting things. There seems to be a few places where EOF processing is not quite correct. At least inefficient.

I have uploaded updated patches. I have to modify the patches to take care of the bitrot caused by changes in comment 18. TIA

Bitrot, rebase, etc.

Bitrot, rebase, etc.

Bitrot, rebase, etc. add-short-read-handling-BASE-directory.patch was split into part-A and part-B to make it easy for some corruption issues to be investigated. This is part-A.

This is part-B of split add-short-read-handling-BASE-diretory.patch.

Rebase, bitrot, etc.

Bitrot, rebase, etc.

Bitrot, rebase, etc.

Bitrot, rebase, etc.

Bitrot, rebase, etc.

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.

bitrot.

bitrot.

bitrot

bitrot

bitrot

bitrot

bitrot

bitrot