1170606 - C-C T-B fixes to take care of short read (part of [META] Failure to deal with short read)

Status: NEW

(MailNews Core :: Database, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Fixes to handle short read correctly (with a few handling of Seek and Write errors thrown in.)

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.

Comment 1

[ISHIKAWA, Chiaki]

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

Comment 2

[ISHIKAWA, Chiaki]

Attachment: Fixes to handle short read correctly [take 2] (with a few handling of Seek and Write errors thrown in.)

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 3

[Magnus Melin [:mkmelin]]

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 4

[Joshua Cranmer [: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.

Comment 5

[ISHIKAWA, Chiaki]

(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

Comment 6

[Joshua Cranmer [:jcranmer]]

(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>.

Comment 7

[ISHIKAWA, Chiaki]

(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

Comment 8

[ISHIKAWA, Chiaki]

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

Comment 9

[ISHIKAWA, Chiaki]

Attachment: support function and declaration and fix for nsMsgUtil.cpp :Fixes to handle short read correctly [take 3 (with a few handling of Seek and Write errors thrown

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.

Comment 10

[ISHIKAWA, Chiaki]

Attachment: Files under mailnews/import :Fixes to handle short read correctly [take 3](with a few handling of Seek and Write errors thrown (6.77 KB, patch)

Fixes for files under mailnews/import

Comment 11

[ISHIKAWA, Chiaki]

Attachment: Files under mailnews/base :Fixes to handle short read correctly [take 3 (with a few handling of Seek and Write errors thrown (6.77 KB, patch)

Fixes for files under mailnews/base

Comment 12

[ISHIKAWA, Chiaki]

Attachment: Files under mailnews/compose:Fixes to handle short read correctly [take 3 (with a few handling of Seek and Write errors thrown (6.77 KB, patch)

fixes for files under mailnews/compose

Comment 13

[ISHIKAWA, Chiaki]

Attachment: Files under mailnews/imap :Fixes to handle short read correctly [take 3 (with a few handling of Seek and Write errors thrown (6.77 KB, patch)

fixes for files under mailnews/imap

Comment 14

[ISHIKAWA, Chiaki]

Attachment: Files under mailnews/local :Fixes to handle short read correctly [take 3] (with a few handling of Seek and Write errors thrown (6.77 KB, patch)

fixes for files under mailnews/local

Comment 15

[ISHIKAWA, Chiaki]

Attachment: Files under mailnews/mime :Fixes to handle short read correctly [take 3] (with a few handling of Seek and Write errors thrown (6.77 KB, patch)

Fixes for files under mailnews/mime

Comment 16

[ISHIKAWA, Chiaki]

Attachment: Files under OTHER directories:Fixes to handle short read correctly [take 3] (with a few handling of Seek and Write errors thrown (6.77 KB, patch)

fixes for files under other directories

Comment 17

[ISHIKAWA, Chiaki]

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

TIA

Comment 18

[ISHIKAWA, Chiaki]

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.

Comment 19

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-base.patch: support function and declaration and fix for nsMsgUtil.cpp [take 4] (with a few handling of Seek and Write errors thrown in for good measure)

Comment 20

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-IMPORT-directory.patch [take 4]

Comment 21

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-BASE-directory.patch

Comment 22

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-COMPOSE-directory-part-1.patch

Comment 23

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-IMAP-directory.patch

Comment 24

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-LOCAL-directory-REV02.patch

Comment 25

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-MIME-directory.patch

Comment 26

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-OTHER-directory.patch

Comment 27

[ISHIKAWA, Chiaki]

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

TIA

Comment 28

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-base.patch: support function and declaration and fix for nsMsgUtil.cpp [take 5] (with a few handling of Seek and Write errors thrown in for go

Bitrot, rebase, etc.

Comment 29

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-IMPORT-directory.patch [take 5]

Bitrot, rebase, etc.

Comment 30

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-BASE-directory-part-A.patch

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.

Comment 31

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-BASE-directory-part-B.patch

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

Comment 32

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-COMPOSE-directory-part-1.patch

Rebase, bitrot, etc.

Comment 33

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-IMAP-directory.patch

Bitrot, rebase, etc.

Comment 34

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-LOCAL-directory-REV02.patch

Bitrot, rebase, etc.

Comment 35

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-MIME-directory.patch

Bitrot, rebase, etc.

Comment 36

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-OTHER-directory.patch

Bitrot, rebase, etc.

Comment 37

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-base.patch: support function and declaration and fix for nsMsgUtil.cpp [take 6] (with a few handling of Seek and Write errors thrown in for go)

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

Comment 38

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-IMPORT-directory.patch [take 6]

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

Comment 39

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-BASE-directory-part-A.patch

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

Comment 40

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-BASE-directory-part-B.patch

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

Comment 41

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-COMPOSE-directory-part-1.patch

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

Comment 42

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-IMAP-directory.patch

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

Comment 43

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-LOCAL-directory-REV02.patch

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

Comment 44

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-MIME-directory.patch

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

Comment 45

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-OTHER-directory.patch

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

Comment 46

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-base.patch: support function and declaration and fix for nsMsgUtil.cpp [take 7] (with a few handling of Seek and Write errors thrown in for go)

bitrot.

Comment 47

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-BASE-directory-part-A.patch

bitrot.

Comment 48

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-BASE-directory-part-B.patch

bitrot

Comment 49

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-COMPOSE-directory-part-1.patch

Comment 50

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-IMAP-directory.patch

bitrot

Comment 51

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-IMPORT-directory.patch

bitrot

Comment 52

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-LOCAL-directory-REV03.patch

bitrot

Comment 53

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-MIME-directory.patch

bitrot

Comment 54

[ISHIKAWA, Chiaki]

Attachment: add-short-read-handling-OTHER-directory.patch

bitrot

Comment 55

[ISHIKAWA, Chiaki]

Attachment: WIP: Bug 1170606 - Handling short read on non-windows platforms. r=BenC,mkmelin

clang-format

nsMsgBrkMBoxStore.cpp reformat

Comment 56

[ISHIKAWA, Chiaki]

Many years ago, M-C tree suffered from short read, too.
I had to white list many files read by routines in M-C tree when I simulated short read and caused errors because
I did not make many changes in M-C trees.

However, yesterday, I was pleasantly surprised that many routines that read

• json files,
• js scrpt files,
• css fles,
• etc.
  do not seem to suffer from short read problems. Great.

Then do only C-C routines that read files such as folders, etc suffer from short read issues?

At least one problem remain possibly in M-C tree or JS land.
I will file a separate bug. What happens is that C-C TB
reading mozilla/devtools/client/locales/en-US/startup.properties suffers from the short read issue.

My local log shows that as soon as startup.properties became the target of short read simulation,
the following happened. The binary only reads the first 1000 octets and did not bother to see if there are remaining octets, and
quits reading (close: fd= 13). Thus, it fails to realize many keys are defined in startup.properties later in the portion of the file that was not read fully.

open64: fd=13, <</NEW-SSD/NREF-COMM-CENTRAL/mozilla/devtools/client/locales/en-US/startup.properties>>	(flags=0x0000, mode=0x00000000)
check filename: /NEW-SSD/NREF-COMM-CENTRAL/mozilla/devtools/client/locales/en-US/startup.properties
hook: matched file found
read(S): fd= 13, ..., expected=1000 -> rc=1000
close: fd= 13
console.error: "No localization found for [inspector.accesskey]"
console.error: "No localization found for [inspector.label]"
console.error: "No localization found for [inspector.panelLabel]"
console.error: "No localization found for [webConsoleCmd.accesskey]"
console.error: "No localization found for [ToolboxTabWebconsole.label]"
console.error: "No localization found for [MenuWebconsole.label]"