Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500

NEW
Assigned to

Status

MailNews Core
Networking
--
major
a year ago
19 days ago

People

(Reporter: ISHIKAWA, Chiaki, Assigned: ISHIKAWA, Chiaki)

Tracking

(Blocks: 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 63 obsolete attachments)

6.38 KB, patch
Details | Diff | Splinter Review
11.76 KB, patch
Details | Diff | Splinter Review
5.95 KB, patch
Details | Diff | Splinter Review
31.39 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
Created attachment 8711165 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

I am loading merged patches from the bug ug 1122698, bug 1134527, bug 1134529, bug  1174500 
The reason for consolidation is that the patch as a whole is much easier to 
read when consolidated.

however, I have split the resulting patch into a few smaller chunks that address files under only certain directories.

More rational and timeline will be posted shortly.


The first patch is to address the following issue:
To enhance error processing, I had to change the semantics of a couple
of functions:
 - |discardNewMessage| does not close aOutputStream.
    (Clarification/Rationale in Bug 1121842, 1122698)
 - |finishdNewMessage| does not close aOutputStream.
   (Clarification/Rationale in Bug 1121842, 1122698)

This required the changes to files under mailnews/import directory as well.
The first patch in the following list addresses this need.

 A Nulling-filestream-after-close-under-IMPORT-directory.patch: Patches for...
 A fix-close-MERGED-base-dir.patch: Bug 1122698 Fixing unchecked Close, unc...
 A fix-close-MERGED-imap-dir.patch: Fixing unchecked Close, unchecked Flush...
 A fix-close-MERGED-local-dir.patch: Fixing unchecked Close, unchecked Flus...
 and  another patch that is explained later:
 A enable-buffering-1st-step.patch (explained later in a different bugzilla.)

TIA
(Assignee)

Updated

a year ago
(Assignee)

Comment 1

a year ago
Created attachment 8711166 [details] [diff] [review]
fix-close-MERGED-base-dir.patch

fix for files under mailnews/base directory.
(Assignee)

Updated

a year ago
Assignee: nobody → ishikawa
(Assignee)

Comment 2

a year ago
Created attachment 8711167 [details] [diff] [review]
fix-close-MERGED-imap-dir.patch

fixes to files under mailnews/imap directory
(Assignee)

Comment 3

a year ago
Created attachment 8711168 [details] [diff] [review]
fix-close-MERGED-local-dir.patch

fixes to files under mailnews/local directory
(Assignee)

Comment 4

a year ago
I will post the rationale and roadmap of the patches shortly.
TIA
(Assignee)

Updated

a year ago
Summary: Conslidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500 → Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500
(Assignee)

Updated

a year ago
Component: Untriaged → Networking
Product: Thunderbird → MailNews Core
(Assignee)

Comment 5

a year ago
A full explanation of the patches and road map is in bug 116055 comment 101
https://bugzilla.mozilla.org/show_bug.cgi?id=1116055#c101

I am excerpting the relevant part from post regarding the consolidation.


... [omission.]...

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

... [omitted] ....


[2] Bug 1242030 - Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500

    The |Seek| before writing each line was introduced, I think, when
    function to enable buffering inadvertently resets file position to
    the beginning.  This may be a natural behavior of such functions,
    but I think it was not documented well and people were caught by
    surprise.   I suspect that the excessive use of |Seek| was an
    attempt to put a band-aid over this issue, and when the attempt was
    made to enable buffering, a subtle issue of rewinding the file
    position seemed to cause file corruption. (I experienced such
    corruption myself, but thanks to the patch in [1], I could learn
    the resetting of file position occurred, which was traced to a
    function to enable buffering, and so I could fix the issue.)

  I have merged the four patches from bug 1122698, 1134527, 1134529,
  1174500 mentioned below, into one patch set in bug 1242030,
  consisting of small patches to files under a directory each. Each
  patch is smaller than the whole gigantic consolidated patch since it
  now only address changes under a particular directory or two.

  The reason for consolidation is that patch as a whole is much easier
  to understand this way. (Error checks are uniformly visible in the
  consolidated patch whereas in the former separate patches, low-level
  I/O functions whose return values were not checked in the earlier
  patch are checked later. This makes the reader of the earlier patch
  sometimes wonder why we don't check the error status of a particular
  low-level I/O function when in fact such a function is checked in a
  later patch. Consolidated patch avoids this issue.

  To enhance error processing, I had to change the semantics of a
  couple of functions:

 - |discardNewMessage| does not close aOutputStream.
    (Clarification/Rationale in Bug 1121842, 1122698)
 - |finishdNewMessage| does not close aOutputStream.
   (Clarification/Rationale in Bug 1121842, 1122698)

 This required the changes to files under mailnews/import directory, too.
 The first patch in the following list addresses this need.

 Consolidated patch set in bug Bug 1242030 are split into the following
 smaller patches.

 Nulling-filestream-after-close-under-IMPORT-directory.patch
 fix-close-MERGED-base-dir.patch
 fix-close-MERGED-imap-dir.patch
 fix-close-MERGED-local-dir.patch
 and  another patch that is explained later:
 enable-buffering-1st-step.patch (explained later in a different
 bugzilla, Bug 1242042.)

[3] (Now merged in Bug 1242030)
Bug 1122698
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)

[4] (Now merged in Bug 1242030)
Bug 1134527
fix-close-part-2.patch: Bug 1134527 Add error value checking of Close() and Flush() (part-2 of a series)

[5] (Now merged  in Bug 1242030)
Bug 1134529
fix-close-part-3-write-error-check.patch: Bug 1134529: Check Write failures (part-3 of a series)

[6] (Now merged  in Bug 1242030, but then the essential part of
bug 1174500 [was mistyped as 1117450 in a few bugzilla entries. :-( ]
removing a |Seek| was spun off as a separate patch: Bug 1242046 )
removing-a-Seek.patch: C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (step 4)

 ... [omitted] ...

TIA
(Assignee)

Updated

a year ago
Attachment #8711165 - Flags: review?(neil)
(Assignee)

Updated

a year ago
Attachment #8711166 - Flags: review?
(Assignee)

Updated

a year ago
Attachment #8711166 - Flags: review? → review?(neil)
(Assignee)

Updated

a year ago
Attachment #8711167 - Flags: review?(neil)
(Assignee)

Updated

a year ago
Attachment #8711168 - Flags: review?(neil)
(Assignee)

Comment 6

a year ago
Created attachment 8721165 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

Part of updated patch set (Feb 18)
Attachment #8711165 - Attachment is obsolete: true
Attachment #8711165 - Flags: review?(neil)
(Assignee)

Comment 7

a year ago
Created attachment 8721166 [details] [diff] [review]
fix-close-MERGED-base-dir.patch
Attachment #8711166 - Attachment is obsolete: true
Attachment #8711166 - Flags: review?(neil)
(Assignee)

Comment 8

a year ago
Created attachment 8721167 [details] [diff] [review]
fix-close-MERGED-imap-dir.patch
Attachment #8711167 - Attachment is obsolete: true
Attachment #8711167 - Flags: review?(neil)
(Assignee)

Comment 9

a year ago
Created attachment 8721168 [details] [diff] [review]
fix-close-MERGED-local-dir-Rev02.patch
Attachment #8711168 - Attachment is obsolete: true
Attachment #8711168 - Flags: review?(neil)
(Assignee)

Comment 10

a year ago
Created attachment 8721169 [details] [diff] [review]
fix-close-MERGED-add-closedflag-IMPORT-directory.patch

This is a new addition on Feb 18. See the next comment for full details.
(Assignee)

Comment 11

a year ago
A full explanation of the updated patches and road map is in bug 116055 comment 107
https://bugzilla.mozilla.org/show_bug.cgi?id=1116055#c107

I am excerpting a relevant part for this bugzilla entry.


========================================
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.]

 ...

[2] Bug 1242030 - Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500

    The |Seek| before writing each line was introduced, I think, when
    function to enable buffering inadvertently resets file position to
    the beginning.  This may be a natural behavior of such functions,
    but I think it was not documented well and people were caught by
    surprise.   I suspect that the excessive use of |Seek| was an
    attempt to put a band-aid over this issue, and when the attempt was
    made to enable buffering, a subtle issue of rewinding the file
    position seemed to cause file corruption. (I experienced such
    corruption myself, but thanks to the patch in [1], I could learn
    the resetting of file position occurred, which was traced to a
    function to enable buffering, and so I could fix the issue.)

  I have merged the four patches from bug 1122698, 1134527, 1134529,
  1174500 mentioned below, into one patch set in bug 1242030,
  consisting of small patches to files under a directory each. Each
  patch is smaller than the whole gigantic consolidated patch since it
  now only address changes under a particular directory or two.

  The reason for consolidation is that patch as a whole is much easier
  to understand this way. (Error checks are uniformly visible in the
  consolidated patch whereas in the former separate patches, low-level
  I/O functions whose return values were not checked in the earlier
  patch are checked later. This makes the reader of the earlier patch
  sometimes wonder why we don't check the error status of a particular
  low-level I/O function when, in fact, such a function is checked in a
  later patch. Consolidated patch avoids this issue.

  To enhance error recovery processing, I had to change the semantics
  of a couple of functions:

 - |discardNewMessage| does not close aOutputStream.
    (Clarification/Rationale in Bug 1121842, 1122698)
 - |finishdNewMessage| does not close aOutputStream.
   (Clarification/Rationale in Bug 1121842, 1122698)

   (Feb 2016) EXCEPT for the version of DiscardNewMessage and
   FinishNewMessage for Maildir storage format.

The original analysis and reasoning is given in Bug 1121842 in
a verbose comment a la stream of consciousness monologue.

This change required the modifications to files under mailnews/import
directory, too.  The first patch in the following list addresses this
need.:  Nulling-filestream-after-close-under-IMPORT-directory.patch

*** begin Feb 2016 Feb ****

Additional comment about this design

But I had to change one thing in Feb 2016 to fix errors only observed
under Windows on try-comm-central.

(Feb 2016) I said, we don't close file stream 
EXCEPT for the version of DiscardNewMessage and
FinishNewMessage for Maildir storage format.
DiscardNewMessage and FinishNewMessage() DO close the stream in that
case.
This has something to do with renaming/removing a file cannot be done
under Windows if an open file descriptor exists for that file.
This was the cause of the failures of WINDOWS BUILD on try-comm-central.
(Under linux or OSX, it is possible to do so.)
My patch set was created originally under linux.

However, my desire, explained in bug 1121842, to keep handling the
Close() of the file stream to the newly downloaded/copied/moved
message file (or mbox, etc.) done on the caller-side of the
Finish/DiscardNewMessage() so that yet-to-be-devised better
error-handling could be done in the logically higher-level of routines
necessitated adding a third argument to the pair of functions to
return the indicator whether these functions closed the passed file
stream (as the 1st argument) before returning.  By looking at the
indicator, the callers can decide whether to close the filestream and
check the error code on their own.

(With the change, Windows DEBUG build on try-comm-central ran without a hitch!)

The addition of third argument to Finish/DiscardNewMessage()
necessitated an additional patch to modify the code under
mailnews/import to import messages from OTHER e-mail clients such as
OE5, etc.

**** end Feb 2016 addition.

 Consolidated patch set in bug Bug 1242030 are split into the following
 smaller patches.

 Nulling-filestream-after-close-under-IMPORT-directory.patch
 fix-close-MERGED-base-dir.patch
 fix-close-MERGED-imap-dir.patch
 fix-close-MERGED-local-dir.patch
 (The next one is an addition in Feb 2016)
*fix-close-MERGED-add-closedflag-IMPORT-directory.patch
 and  another patch that is explained later:
 enable-buffering-1st-step.patch (explained later in a different
 bugzilla, Bug 1242042.)

[3] (Now merged in Bug 1242030)
Bug 1122698
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)

[4] (Now merged in Bug 1242030)
Bug 1134527
fix-close-part-2.patch: Bug 1134527 Add error value checking of Close() and Flush() (part-2 of a series)

[5] (Now merged  in Bug 1242030)
Bug 1134529
fix-close-part-3-write-error-check.patch: Bug 1134529: Check Write failures (part-3 of a series)

[6] (Now merged  in Bug 1242030, but then the essential part of
bug 1174500 [was mistyped as 1117450 in a few bugzilla entries. :-( ]
removing a |Seek| was spun off as a separate patch: Bug 1242046 )
removing-a-Seek.patch: C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (step 4)

[6.5] (New in Feb 2016)
An addition to 1242030 in Feb.
fix-close-MERGED-add-closedflag-IMPORT-directory.patch

This patch takes care of the modification of files under
mailnews/import directory to take care of the addition of the third
argument to |FinishNewMessage| and |DiscardNewMessage|.
 ...
(Assignee)

Comment 12

11 months ago
Created attachment 8761282 [details] [diff] [review]
fix-close-MERGED-add-closedflag-IMPORT-directory.patch (fix bit rot on Jun 4, 2016)

This is a replacement to fix bit rot of  fix-close-MERGED-add-closedflag-IMPORT-directory.patch that was uploaded previously.
(There have been a Becky import addition, and this patch address the necessary patch in that source code.)

This patch is part of the series of patch to enable buffered I/O (mostly interesting is the output part).

These patches ought to be applied in this order.

Patches marked with "*"  are new updated uploads to cope with bit rot. (June 9th, 2016)

*[1] Bug 1116055 
check_fileptr_change.patch: Bug 1116055: Check the unexpected change of file seek position and ask the user to report it

[2] Bug 1242030 - Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500

 Consolidated patch set in bug Bug 1242030 are split into the following
 smaller patches.

 Nulling-filestream-after-close-under-IMPORT-directory.patch
 fix-close-MERGED-base-dir.patch
 fix-close-MERGED-imap-dir.patch
 fix-close-MERGED-local-dir.patch
*fix-close-MERGED-add-closedflag-IMPORT-directory.patch

[7] (New in 2016 ) Bug 1242042 - Enabling buffering for file stream to write message for C-C TB
A enable-buffering-1st-step.patch: Enabling buffering first step.

[8] Bug 1176857
1116055-acelist.patch: One liner patch posted in Bug 1116055 to return a buffered stream from mbox::getNewOutputStream by aceman

[9] (NEW in 2016)
Bug 1242046:  	Removing unnecessary |Seek| that caused the C-C TB to operate slowly in terms of I/O 
  removing-a-Seek-rev02.patch: removing a Seek that is performance
  pig!  This was extracted into a new patch from the patches above [5]-[8].

[10] New in 2016
1242050 - C-C TB: use a default output stream buffer size of 16KB instead of 4KB 
buffer-by-16K-by-default.patch: output buffer by 16KB by default

The patches have been tested locally under linux (with |make mozmill|, etc)
and on c-c try server.

Please check the following tryserver job for testing result, etc.
There are no new errors aside from other errors noticed by others.

See for example:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2a082a9b933239bb49f22bab3f8bf6b445326127

Aside from the know existing errors, there are no new errors introduced.

TIA
Attachment #8721169 - Attachment is obsolete: true
(In reply to ISHIKAWA, Chiaki from comment #12)
> 
> See for example:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=2a082a9b933239bb49f22bab3f8bf6b445326127

Chiaki, given the probability that patches likely will take some weeks to get reviewed, how comfortable would you feel with me offering this try build to a few properly coached technical testers who are *imap* only, so that we can get some feedback?
Flags: needinfo?(ishikawa)
(Assignee)

Comment 14

11 months ago
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #13)
> (In reply to ISHIKAWA, Chiaki from comment #12)
> > 
> > See for example:
> > https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> > central&revision=2a082a9b933239bb49f22bab3f8bf6b445326127
> 
> Chiaki, given the probability that patches likely will take some weeks to
> get reviewed, how comfortable would you feel with me offering this try build
> to a few properly coached technical testers who are *imap* only, so that we
> can get some feedback?

That would be great.
I am quie confident that this does not introduce any new bugs although
it may not solve some known issues (aside from the use of larger buffers and
the confirmation of the legacy error handling's working with the patch!)

For that matter, I am not sure who the reviewers ought to be.

Thank you for the help!
Flags: needinfo?(ishikawa)

Comment 15

9 months ago
Comment on attachment 8721165 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

>+      // XXX : I wonder what happens if reusable stream is returned
>+      // in the following GetNewMsgOutputStream.
>+      // In the next while loop,
>+      // wouln't the previous outStream with open file be simply
>+      // overwritten and we leak file descriptor and such?
>+      // It looks that if outStream is not nullptr
>+      // new File stream is NOT created and assigned. Old file stream is used.
getter_Addrefs actually clears its variable first, so you don't actually pass the previous outStream in. Instead what happens is that GetNewMsgOutputStream saves the old file stream internally. If it's still open, then it just returns it again, otherwise it opens a new stream.

>-      if (!reusable)
>+      if (!reusable) {
>         outStream->Close();
>+        outStream = nullptr;
>+      }
>     }
>-    if (outStream)
>+    if (outStream) {
>       outStream->Close();
>+      outStream = nullptr;
>+    }
[A non-resuable stream is already closed at this point, so it doesn't matter that we're trying to close it again, although there are some cases such as m_dstOutputStream below where it does get set to null, so the consistency would be good for clarity.]

>+  // XXX: I believe m_dstOutputStream should be closed here.
>+  // Otherwise, a reusable stream won't be closed at all. Maybe we
>+  // leak file descriptor.
No, it gets closed in cMbxScanner::CleanUp.
(Assignee)

Comment 16

8 months ago
(In reply to neil@parkwaycc.co.uk from comment #15)
> Comment on attachment 8721165 [details] [diff] [review]
> Nulling-filestream-after-close-under-IMPORT-directory.patch
> 

Neil, thank you for your comment.
I have been on a summer holiday week: I just came back home yesterday.
I will try to incorporate your comment into comments in my patch set.

TIA
(Assignee)

Comment 17

7 months ago
Created attachment 8790812 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

Trying to accommodate Neal's comment in comment 15.
Attachment #8721165 - Attachment is obsolete: true
Attachment #8790812 - Flags: review?(neil)
(Assignee)

Comment 18

7 months ago
Created attachment 8790814 [details] [diff] [review]
fix-close-MERGED-base-dir.patch

Fixing the recent bitrot due to changes of NS_WARN_IF_FALSE to NS_WARNING_ASSERTION.
(Some of the use of NS_WARN_IF_FALSE may not be proper to change to NS_WARNING_ASSERTION since I want the error/warning message printed even normal distributed build, but for now, getting the build done is utmost priority.)
Attachment #8721166 - Attachment is obsolete: true
(Assignee)

Comment 19

7 months ago
Created attachment 8790816 [details] [diff] [review]
fix-close-MERGED-imap-dir.patch

Fix bitrot. See previous comment.
Attachment #8721167 - Attachment is obsolete: true
(Assignee)

Comment 20

7 months ago
Created attachment 8790820 [details] [diff] [review]
fix-close-MERGED-local-dir-Rev02.patch

Fix bitrot. See previous comment.
Attachment #8721168 - Attachment is obsolete: true
(Assignee)

Comment 21

7 months ago
Created attachment 8790824 [details] [diff] [review]
fix-close-MERGED-add-closedflag-IMPORT-directory.patch

fix bitrot. See previous comment.
Attachment #8761282 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1122698
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1134527
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1134529
(Assignee)

Comment 25

7 months ago
Created attachment 8796871 [details] [diff] [review]
fix-close-MERGED-base-dir.patch

Fixed bitrot and a few other merge issues since Sept 21.
Attachment #8790814 - Attachment is obsolete: true
(Assignee)

Comment 26

7 months ago
Created attachment 8796872 [details] [diff] [review]
fix-close-MERGED-imap-dir.patch

Fixed bitrot and a merge issues since Sep 21.
Attachment #8790816 - Attachment is obsolete: true
(Assignee)

Comment 27

7 months ago
Created attachment 8796873 [details] [diff] [review]
fix-close-MERGED-local-dir-Rev02.patch

Fixed bitrot and merge issue since Sep 13.
Attachment #8790820 - Attachment is obsolete: true
(Assignee)

Comment 28

7 months ago
Created attachment 8796874 [details] [diff] [review]
fix-close-MERGED-add-closedflag-IMPORT-directory.patch

Fixed bitrot and a merge issue since Sep 13.
Attachment #8790824 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Duplicate of this bug: 1174500
(Assignee)

Updated

7 months ago
Depends on: 1116055
(Assignee)

Updated

7 months ago
Blocks: 1242042
(Assignee)

Updated

7 months ago
Duplicate of this bug: 939549
There are many references to maildir. Should this bug block re-enabling of maildir?

Also, it would be great to drive this in in the next couple weeks before we lose the window of version 52. Are there other potential reviewers for the patches where Neil is not referenced?
Flags: needinfo?(acelists)
Also, thinking out loud, I'm not sure we'd want approvals for the import patches to block progress on the dependent bugs. 
(unless not having them presents a significant danger)
(Assignee)

Comment 33

7 months ago
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #32)
> Also, thinking out loud, I'm not sure we'd want approvals for the import
> patches to block progress on the dependent bugs. 
> (unless not having them presents a significant danger)

There are a few places where function signature changed and need to be accommodated.
(Assignee)

Comment 34

7 months ago
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #31)
> There are many references to maildir. Should this bug block re-enabling of
> maildir?
> 
> Also, it would be great to drive this in in the next couple weeks before we
> lose the window of version 52. Are there other potential reviewers for the
> patches where Neil is not referenced?

I am wondering who the reviewers would be. If you can suggest some and assign as reviewers, I would appreciate it.
(Assignee)

Updated

7 months ago
Depends on: 939548
(Assignee)

Comment 35

7 months ago
I missed a patch that was created last year due to a hard disk problem last summer.
So the order of patch application is

bug 1116055
 => bug 939548 (this was missed until this weekend)
  -> bug 1242030 (consolidation of 1122698, 1134527, 1134529, 1174500)
    -> bug 1242042 
       -> bug 1176857
         -> bug 1242046
           -> bug 1242050
             -> bug 1170606
(Assignee)

Comment 36

7 months ago
Created attachment 8798545 [details] [diff] [review]
fix-close-MERGED-base-dir.patch

Fixed bitrot: #if DEBUG -> #ifdef DEBUG, NS_WARNING_ASSERTION -> open coding since I want an behavior that is both in effect during debug and optimized build.
Attachment #8796871 - Attachment is obsolete: true
(Assignee)

Comment 37

7 months ago
Created attachment 8798559 [details] [diff] [review]
fix-close-MERGED-imap-dir.patch

Bitrot. One line change, etc.
Attachment #8796872 - Attachment is obsolete: true
(Assignee)

Comment 38

7 months ago
Created attachment 8798560 [details] [diff] [review]
fix-close-MERGED-local-dir-Rev02.patch

Bitrot, rebase, etc.
Attachment #8796873 - Attachment is obsolete: true
(Assignee)

Comment 39

7 months ago
Created attachment 8798562 [details] [diff] [review]
fix-close-MERGED-add-closedflag-IMPORT-directory.patch

Bitrot, rebase, etc.
(Assignee)

Updated

7 months ago
Attachment #8796874 - Attachment is obsolete: true

Comment 40

6 months ago
Actually it is not great to split patches according to directories, unless it is a global change (like refactoring some constructs (e.g. removing Iterator()), or adding checking of return values from Close() all over the place).

Maybe at least the changes needed for DiscardNewMessage could have been in a single patch so we could see what that actually does across the tree.

But it is probably too late to rearrange the patches around after all the work you did already. And maybe some reviewer asked for the consolidation in the past.

What I am more concerned with is that you add a ton of rv1...rv4 variables and print out debugging messages if there are errors. Those variable names are unusual in the current tree. But you do that to actually NOT handle the errors returned. The function is not exited and some other rv from other operation is returned. So what is the point of this? The I/O errors are not handled, only printed out in messages that only developers will maybe see. I thought you wanted to handle the errors to prevent data corruption, which you say may be made worse by more buffering.
Flags: needinfo?(acelists)
Version: unspecified → Trunk
(Assignee)

Comment 41

6 months ago
(In reply to :aceman from comment #40)
> Actually it is not great to split patches according to directories, unless
> it is a global change (like refactoring some constructs (e.g. removing
> Iterator()), or adding checking of return values from Close() all over the
> place).
> 
> Maybe at least the changes needed for DiscardNewMessage could have been in a
> single patch so we could see what that actually does across the tree.
> 

Let me think about it. If the reproducing the patches in this fashion is not difficult I may do so over this weekend.

> But it is probably too late to rearrange the patches around after all the
> work you did already. And maybe some reviewer asked for the consolidation in
> the past.
> 
> What I am more concerned with is that you add a ton of rv1...rv4 variables
> and print out debugging messages if there are errors. Those variable names
> are unusual in the current tree. But you do that to actually NOT handle the
> errors returned. The function is not exited and some other rv from other
> operation is returned. So what is the point of this? The I/O errors are not
> handled, only printed out in messages that only developers will maybe see. I
> thought you wanted to handle the errors to prevent data corruption, which
> you say may be made worse by more buffering.

You noticed a very good point.
Let me explain here again.

First off, I am not claiming my patches are perfect, but
these were necessary to recover from download errors when I simulate the dowload errors by
unplugging network cable to simulate errors when my Mail store was on remote file system.

Basically the reason where the error return value of low-level I/O function was checked
but not serious action is taken is as follows.
[In a nutshell, there is no clear-cut agreed upon error recovery scheme in place, nor TB does not seem to be designed and coded with such error recovery in mind in the first place as far as I could tell [except for the crude transaction-like handling of message download], simply I DON'T KNOW WHAT TO DO with error return values in many place in this legacy code base.]

Let me explain:
During my debug efforts to make sure that error return delayed by buffering from |Write| to |Close|
is handled at least minimally sanely, I tested the mail download errors.
Then I noticed this
 - all I could effectively test was the mail downloading failures,
   for that, there was a very crude transaction-mechanism that uses DiscardNewMessage 
  when there is an error
   (which unfortunately didn't  work before until proper I/O error checks were 
   introduced by my patches). So error returns related to this processing.
   *ARE* checked and used for such error-recovery actions.
- there *are* other I/O errors that I tried to check (I mean I inserted the "rv =" and
  dumped the value there. But, since there is no overall well-described error recovery strategy, 
  all I could do was to note that there was
an error and print the error value (and this helped me to notice the issue with Maildir under Windows, which would have been impossible without such dumps.). 

My intention was to place the dumps in "ifdef DEBUG", "#endif" so that such dumps are seen in DEBUG build. (But many serious errors are also dumped in non-DEBUG build, too, so that someone using TB would notice). *If* and *when* such an error recovery scheme is devised and agreed upon, the checks (rv = ...)
will be now utilized to guide such error recover actions.

In the meantime, the dump in DEBUG build should help us to 
detect where errors occur, and may even ask particular users who suffer errors due to flakey network connection to remote file servers (and this actually is more important in the patch series in 
bug 1170606 to handle short read) to learn what errors occur in the field, indeed.

So my short answer is for many cases, I have no idea what error recovery actions should be taken, frankly speaking [just understanding how some copy operations done asynchronously is beyond this casual user of TB, and devising error recovery strategy is more so], but for future development efforts, I marked where the error return should be checked so that people can work on such future improvement easily.

Once the error recovery, etc. is well defined, or some quick advices are given, I will use the error return values to guide some suggested actions.

In the meantime, the dump would help any erratic behavior due to I/O errors in the field (assuming people test their environment using DEBUG version once they realize their environment is causing problems..)

As I think hard about it, there are probably more users who get bitten by the issue addressed by patches in the next pending bug 1170606, but reaching that bugzilla is still a long way to go.

Again, my patches are not complete, and I wished I could wrap up the unhandled cases, but simply I could not, for now. 

> I thought you
> wanted to handle the errors to prevent data corruption, which you say may be
> made worse by more buffering.

This part is addressed by my patches as far as the major error modes that I could induce by
artificial network errors against CIFS/SAMBA, NFS network systems are concerned. 
I am confident, this alone will save some users in the field (and without my patches, the crude transaction handling does NOT work at all was what I found out. In this sense alone, the patches are worthwhile.)

So it is a small improvement. We have to improve more in the future if there is enough man-power, 
but for now, it is a humble start.

I am hoping that the next major patch series in bugzilla bug 1170606 would interest some corporate users, who need to use remote network file system to store their profile including Mail store there [linux's I/O does not handle low-level error retry and short read/write whereas Windows's do. So linux TB has many problems there.],
to throw in some resources for TB development, but maybe I am having a pipe dream.
[And I also noticed that we need to fix M-C tree as well. Ugh...]


TIA

Chiaki
(Assignee)

Comment 42

6 months ago
I realize that some description about the patch content in comment 11 and comment 12 needs updating due to
the latest other landed patches, bitrot, and my better understanding of the code. I will try to update that
in the next few days *IIF* I have the time.

TIA
(Assignee)

Comment 43

6 months ago
I have been updating the patches to address the bitrot. I have found a couple of
incorrect merge/consolidation while I am doing it. That is good.
I would post them Sunday evening after running through try-comm-central.

While I was doing the updating of the patches, I realized there was a very pressing need for using
rv2, rv3, rv4, etc., at least different error variables, instead of just using single rv.

Namely, the legacy code used |rv| for error processing. Some times it uses |rv| in a contorted manner which I dislike very much and I think rkent dislikes it, too.

  rv = someFunction().
  if (NS_SUCCEEDED(rv)) {
     .... rv  = other_function();
  }
  ...
  if (NS_SUCCEEDED(rv)) {
    ... rv = still_other_function(); ...
  }

  return rv;

What |rv| holds at the end is anyone's guess. We can only learn it at runtime.
Also, trying to modify the code by introduciing other error checks
is made difficult due to the above rigid use of single variable |rv| for conditional
expression. 
As soon as we assign some new error return value to |rv|, we break the logic of
legacy code.
But I needed to introduce the checks for many new hither-to unchecked error returns.
So I needed to come up with new variables to hold the newly checked  return values.
Yes, it is unfortunate that I did use generic unimaginative names. (But so is |rv|.) 
At least, by not touching |rv|, where I checked and dumped error return values only [hoping that we would come to handle the proper error recovery in the future], at least the legacy error handling based on
|rv| (or the lack of it) is not disturbed.

That is,


  rv = someFunction().
  if (NS_SUCCEEDED(rv)) {
     .... rv  = other_function();
  }
  ...
  // By using rv2 below, I don't disturb the legacy error processing (or the lack of it)
  // based on rv.
  nsresult rv2 = formally_unchecked_IO_operation();
  if (NS_FAILED(rv2)) {
    do something, including return rv2 if necessary.
  } 
  ...
  if (NS_SUCCEEDED(rv)) {
    ... rv = still_other_function(); ...
  }

  return rv;

[Of course, you will find that some parts DO mix and match the |rv|, |rv1|, etc. for proper [that looked to me proper] error opration].

Also, some I/O errors occur independent of each other in an file-level operation
(sometimes they are related and occur in cascade). 
For good error reporting, we need to record each of these errors into different variables,
and may decide to report the error based on a predefined rule of priority.

A case in point is a function called CopyToNative().
My memory is hazy, but either this function or its friend was responsible for failing to report the error of moving/copying a message to another folder. That was a bad dataloss error.

While debugging the problem, I noticed the function was responsible for not reporting the error,
and there are a few possible error cases, and so I introduced different variables to record each of the error (well, back then, I had the composure to come up with aptly named variables instead of |rv1|, |rv2|, etc.), and used the variables to report the errors by favoring some errors over the others.

See https://dxr.mozilla.org/comm-central/source/mozilla/xpcom/io/nsLocalFileUnix.cpp#778
especially the code starting around 887 until the function returns error value at the end.

I know the use of |rv1|, |rv2|, |rv3|, etc. sucks and ugly, but there were reasons for them.
(And I didn't feel the necessity to come up with fancy variable names just for error reporting in a code base that lacks proper error checks in so many places.)

TIA
(Assignee)

Comment 44

6 months ago
Created attachment 8807617 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.
Attachment #8790812 - Attachment is obsolete: true
Attachment #8790812 - Flags: review?(neil)
Attachment #8807617 - Flags: review?(neil)
(Assignee)

Comment 45

6 months ago
Created attachment 8807618 [details] [diff] [review]
bug 1242030-c fix-close-MERGED-imap-dir.patch

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.
Attachment #8798559 - Attachment is obsolete: true
(Assignee)

Comment 46

6 months ago
Created attachment 8807620 [details] [diff] [review]
bug 1242030-b  fix-close-MERGED-base-dir.patch

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.
Attachment #8798545 - Attachment is obsolete: true
(Assignee)

Comment 47

6 months ago
Created attachment 8807621 [details] [diff] [review]
bug 1242030-d fix-close-MERGED-local-dir-Rev02.patch

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.
Attachment #8798560 - Attachment is obsolete: true
(Assignee)

Comment 48

6 months ago
Created attachment 8807622 [details] [diff] [review]
fix-close-MERGED-add-closedflag-IMPORT-directory.patch

Fixed bitrot and modified some comments so that they are more "appropriate" in C-C source tree.
Attachment #8798562 - Attachment is obsolete: true

Comment 49

6 months ago
If you're waiting for Neil, you can wait until Christmas 2029 ;-)
(Assignee)

Comment 50

6 months ago
(In reply to Jorg K (GMT+1) from comment #49)
> If you're waiting for Neil, you can wait until Christmas 2029 ;-)

Will you take up the review, Jorg, then?

I need someone's input. 

I don't have a strong opinion of coding style. etc.
All I want is to put in the essential patches for enabling buffered-write.
It doesn't make sense to overwhelm the local OS or remote file server with 75-octet writes over and over again for 5MB pdf attachment. (Each December we have a large technology exhibition, and to prepare the handout to the visitors, I need to overlook the drafts for the handout, and about 1000 e-mails with attachments fly in November alone. Failure to use buffered-write *IS* noticeable when you need to save so many relatively large attachments.)

The patches have been in my local queue for a very long time now., and I want to get rid of them soon because it interferes with working on new bugs I find.

TIA

Comment 51

6 months ago
(In reply to ISHIKAWA, Chiaki from comment #50)
> Will you take up the review, Jorg, then?
Yes, I'll do the first pass, then over to Aceman ;-)
(Assignee)

Comment 52

6 months ago
(In reply to Jorg K (GMT+1) from comment #51)
> (In reply to ISHIKAWA, Chiaki from comment #50)
> > Will you take up the review, Jorg, then?
> Yes, I'll do the first pass, then over to Aceman ;-)

Thanks a lot!
My day job gets in the way due to some big meetings and a conference in the next 6-7 weeks, but I will try.

Comment 53

6 months ago
Comment on attachment 8807617 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

That about the other four patches in the bug? We do one by one?
Attachment #8807617 - Flags: review?(neil) → review?(jorgk)

Comment 54

6 months ago
Comment on attachment 8807617 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

Review of attachment 8807617 [details] [diff] [review]:
-----------------------------------------------------------------

Please address the nits below.

::: mailnews/import/applemail/src/nsAppleMailImport.cpp
@@ +564,5 @@
> +      // internally. If it's still open, then it just returns it
> +      // again, otherwise it opens a new stream.
> +      // In a nutshell, if the old outStream was still open, a new
> +      // File stream is NOT created and assigned. Old file stream is
> +      // used instead.

Please remove comment. This is not the place to capture this information.

@@ +586,2 @@
>          outStream->Close();
> +        outStream = nullptr;

I'm OK with this and all the ones that follow.

@@ +591,5 @@
> +    // A non-resuable stream is already closed at this point, so it
> +    // doesn't matter that we're trying to close it again, although
> +    // there are some cases such as m_dstOutputStream below where it
> +    // does get set to null, so the consistency would be good for
> +    // clarity. That is why outStream is set to nullptr above and below.

Remove comment.

::: mailnews/import/oexpress/nsOE5File.cpp
@@ +321,5 @@
> +    // it's still open, then it just returns it again, otherwise it
> +    // opens a new stream.
> +    // In a nutshell, if the old outStream was still open, a new File
> +    // stream is NOT created and assigned. Old file stream is used
> +    // instead.

Remove comment.

::: mailnews/import/oexpress/nsOEMailbox.cpp
@@ +273,5 @@
> +  if (m_mbxFileInputStream) {
> +    rv = m_mbxFileInputStream->Close();
> +    if (NS_FAILED(rv)) {
> +      IMPORT_LOG1("CleanUp: m_mbxFileInputStream failed: 0x%08x\n", (unsigned) rv);
> +    }

We don't need to log this.

@@ +280,5 @@
> +  if (m_dstOutputStream)  {
> +    rv = m_dstOutputStream->Close();
> +    if (NS_FAILED(rv)) {
> +      IMPORT_LOG1("CleanUp: m_dstOutputStream failed: 0x%08x\n", (unsigned) rv);
> +    }

And this one either.

@@ +353,5 @@
>      m_dstOutputStream->Close();
>      m_dstOutputStream = nullptr;
>    }
> +
> +  // cf. reusable m_dstOutputStream does not have to be closed here.

cf. meaning?

::: mailnews/import/outlook/src/nsOutlookMail.cpp
@@ +442,2 @@
>      outputStream->Close();
> +    outputStream = nullptr; // code uniformity. 

Remove comment and trailing space.
(In reply to Jorg K (GMT+1) from comment #53)
> Comment on attachment 8807617 [details] [diff] [review]
> Nulling-filestream-after-close-under-IMPORT-directory.patch
> 
> [W]hat about the other four patches in the bug? We do one by one?

Indeed. I suggest we treat the larger, non-import patches with higher importance than the import patches.
(Assignee)

Comment 56

6 months ago
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #55)
> (In reply to Jorg K (GMT+1) from comment #53)
> > Comment on attachment 8807617 [details] [diff] [review]
> > Nulling-filestream-after-close-under-IMPORT-directory.patch
> > 
> > [W]hat about the other four patches in the bug? We do one by one?
> 
> Indeed. I suggest we treat the larger, non-import patches with higher
> importance than the import patches.

They have to be checked in parallel, sort of, since the changes need to be applied in one bunch.

I can't do a lot until Thursday, but the comments in Comment 54 are noted.

TIA

Comment 57

6 months ago
Comment on attachment 8807617 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

Ready when you are.
Attachment #8807617 - Flags: review?(jorgk)
(Assignee)

Comment 58

6 months ago
Created attachment 8809861 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

(In reply to Jorg K (GMT+1) from comment #54)
> Comment on attachment 8807617 [details] [diff] [review]
> Nulling-filestream-after-close-under-IMPORT-directory.patch
> 
> Review of attachment 8807617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please address the nits below.
> 
> ::: mailnews/import/applemail/src/nsAppleMailImport.cpp
> @@ +564,5 @@
> > +      // internally. If it's still open, then it just returns it
> > +      // again, otherwise it opens a new stream.
> > +      // In a nutshell, if the old outStream was still open, a new
> > +      // File stream is NOT created and assigned. Old file stream is
> > +      // used instead.
> 
> Please remove comment. This is not the place to capture this information.
> 

DONE

> @@ +586,2 @@
> >          outStream->Close();
> > +        outStream = nullptr;
> 
> I'm OK with this and all the ones that follow.

Thansk.

 
> @@ +591,5 @@
> > +    // A non-resuable stream is already closed at this point, so it
> > +    // doesn't matter that we're trying to close it again, although
> > +    // there are some cases such as m_dstOutputStream below where it
> > +    // does get set to null, so the consistency would be good for
> > +    // clarity. That is why outStream is set to nullptr above and below.
> 
> Remove comment.

DONE

> 
> ::: mailnews/import/oexpress/nsOE5File.cpp
> @@ +321,5 @@
> > +    // it's still open, then it just returns it again, otherwise it
> > +    // opens a new stream.
> > +    // In a nutshell, if the old outStream was still open, a new File
> > +    // stream is NOT created and assigned. Old file stream is used
> > +    // instead.
> 
> Remove comment.
> 

DONE


> ::: mailnews/import/oexpress/nsOEMailbox.cpp
> @@ +273,5 @@
> > +  if (m_mbxFileInputStream) {
> > +    rv = m_mbxFileInputStream->Close();
> > +    if (NS_FAILED(rv)) {
> > +      IMPORT_LOG1("CleanUp: m_mbxFileInputStream failed: 0x%08x\n", (unsigned) rv);
> > +    }
> 
> We don't need to log this.


Removed.

> 
> @@ +280,5 @@
> > +  if (m_dstOutputStream)  {
> > +    rv = m_dstOutputStream->Close();
> > +    if (NS_FAILED(rv)) {
> > +      IMPORT_LOG1("CleanUp: m_dstOutputStream failed: 0x%08x\n", (unsigned) rv);
> > +    }
> 
> And this one either.

Removed, but I left an XXX TODO comment.

> 
> @@ +353,5 @@
> >      m_dstOutputStream->Close();
> >      m_dstOutputStream = nullptr;
> >    }
> > +
> > +  // cf. reusable m_dstOutputStream does not have to be closed here.
> 
> cf. meaning?
Originally I had thought that I need to create
an else-clause, and close the reuable m_dstOutputStream there.
  else {
    close m_dstOutputStream
  }
 
However, it was pointed out that we don't have to close the reusable one here since it is closed in Cleanup.
Either I put 
   else {
     // no action since reusable m_dstOutputStream is closed in CleanUp.
   }
explicitly, or modify the comment?

> 
> ::: mailnews/import/outlook/src/nsOutlookMail.cpp
> @@ +442,2 @@
> >      outputStream->Close();
> > +    outputStream = nullptr; // code uniformity. 
> 
> Remove comment and trailing space.

DONE

TIA
Attachment #8807617 - Attachment is obsolete: true
Attachment #8809861 - Flags: review?(jorgk)
(Assignee)

Comment 59

6 months ago
Created attachment 8809863 [details] [diff] [review]
bug 1242030-e fix-close-MERGED-add-closedflag-IMPORT-directory.patch

Handle bitrot caused by the change in Nulling-filestream-after-close-under-IMPORT-directory.patch.

Once  8809861: Nulling-filestream-after-close-under-IMPORT-directory.patch is checked in, all other fours including this one need to be checked simultaneously.

Should I set the review flag to Jorg myself?
Attachment #8807622 - Attachment is obsolete: true
Flags: needinfo?(vseerror)
Flags: needinfo?(jorgk)
(Assignee)

Comment 60

6 months ago
It might be easier to merge the patch in 
fix-close-MERGED-add-closedflag-IMPORT-directory.patch
and  
Nulling-filestream-after-close-under-IMPORT-directory.patch 
then it will be a single patch along with three other patches that must go in at the same time.
(We can save the uploading of modified fix-close-MERGED-add-closedflag-IMPORT-directory.patch
due to the bitrot caused by the change in 
Nulling-filestream-after-close-under-IMPORT-directory.patch )

The split of 
Nulling-filestream-after-close-under-IMPORT-directory.patch
and 
fix-close-MERGED-add-closedflag-IMPORT-directory.patch
is purely historical. 
But if 
Nulling-filestream-after-close-under-IMPORT-directory.patch 
can go in soon, it is OK.

TIA
Comment on attachment 8809863 [details] [diff] [review]
bug 1242030-e fix-close-MERGED-add-closedflag-IMPORT-directory.patch

> Should I set the review flag to Jorg myself?

In future, yes. AIUI Jorg has graciously agreed to review your patches and the two of you (with aceman) are doing a great job.
Flags: needinfo?(vseerror)
Flags: needinfo?(jorgk)
Attachment #8809863 - Flags: review?(jorgk)

Comment 62

6 months ago
Comment on attachment 8809861 [details] [diff] [review]
Nulling-filestream-after-close-under-IMPORT-directory.patch

Review of attachment 8809861 [details] [diff] [review]:
-----------------------------------------------------------------

You didn't do what you said you'd be doing ;-(

::: mailnews/import/applemail/src/nsAppleMailImport.cpp
@@ +559,5 @@
> +      // outStream with open file is not overwritten and we do NOT
> +      // leak file descriptor and such.  Details: getter_Addrefs
> +      // actually clears its variable first, so you don't actually
> +      // pass the previous outStream in. Instead GetNewMsgOutputStream
> +      // saves the old file stream internally.

I think we agreed to remove the comment since this is not the place to capture this information.

Besides, there are two implementations of this, mbox and maildir, and I'm not sure that the comment is actually correct.

::: mailnews/import/oexpress/nsOE5File.cpp
@@ +317,5 @@
> +    // file descriptor and such.
> +    // Details: getter_Addrefs actually clears its variable first, so
> +    // you don't actually pass the previous outStream in. Instead
> +    // GetNewMsgOutputStream saves the old file stream internally.
> +

remove.

::: mailnews/import/oexpress/nsOEMailbox.cpp
@@ +275,5 @@
> +    m_mbxFileInputStream = nullptr;
> +  }
> +  if (m_dstOutputStream)  {
> +    rv = m_dstOutputStream->Close();
> +    // XXX TODO we should check write error

Remove. One comment above it enough.

@@ +350,5 @@
>    }
> +
> +  // cf. reusable m_dstOutputStream does not have to be closed here.
> +  // It gets closed in CMbxScanner::CleanUp.
> +

The comment is fine if you remove the empty line.
I asked what "cf." means, but you haven't answered. So I'm asking again. I think it would be best not to use abbreviations which are uncommon. See:
https://en.wikipedia.org/wiki/Cf.
Attachment #8809861 - Flags: review?(jorgk)

Comment 63

6 months ago
Comment on attachment 8809863 [details] [diff] [review]
bug 1242030-e fix-close-MERGED-add-closedflag-IMPORT-directory.patch

Review of attachment 8809863 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/import/applemail/src/nsAppleMailImport.cpp
@@ +572,5 @@
>  
>        // add the data to the mbox stream
>        if (NS_SUCCEEDED(nsEmlxHelperUtils::AddEmlxMessageToStream(currentEntry, outStream))) {
>          mProgress++;
> +        msgStore->FinishNewMessage(outStream, msgHdr, &closed);

I'm not sure how this would compile since the function takes two parameters only.

Do you intend to modify this function? Not in this patch though.
Attachment #8809863 - Flags: review?(jorgk)
(Assignee)

Comment 64

5 months ago
Created attachment 8810174 [details] [diff] [review]
bug 1242030-a Nulling-filestream-after-close-under-IMPORT-directory.patch

Modified patch.

(In reply to Jorg K (GMT+1) from comment #62)
> Comment on attachment 8809861 [details] [diff] [review]
> Nulling-filestream-after-close-under-IMPORT-directory.patch
> 
> Review of attachment 8809861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You didn't do what you said you'd be doing ;-(
> 
> ::: mailnews/import/applemail/src/nsAppleMailImport.cpp
> @@ +559,5 @@
> > +      // outStream with open file is not overwritten and we do NOT
> > +      // leak file descriptor and such.  Details: getter_Addrefs
> > +      // actually clears its variable first, so you don't actually
> > +      // pass the previous outStream in. Instead GetNewMsgOutputStream
> > +      // saves the old file stream internally.
> 
> I think we agreed to remove the comment since this is not the place to
> capture this information.
> 

Sorry, misunderstanding on my part.
I thought you want the removal of the latter half of the comment
paragraph.

Previous comment:
>> ::: mailnews/import/applemail/src/nsAppleMailImport.cpp
>> @@ +564,5 @@
>> > +      // internally. If it's still open, then it just returns it
>> > +      // again, otherwise it opens a new stream.
>> > +      // In a nutshell, if the old outStream was still open, a new
>> > +      // File stream is NOT created and assigned. Old file stream is
>> > +      // used instead.
>> 
>> Please remove comment. This is not the place to capture this information.

All right, I will remove the comment entirely.
But where do you think would be the appropriate space for the comment
which I gleaned from a comment from someone in the know. 
That was helpful in understanding that
the code in question does not leak file descriptor.

> Besides, there are two implementations of this, mbox and maildir, and I'm
> not sure that the comment is actually correct.

Maybe my original comment was not quite clear enough, but so was the
comments I got.
Basically,
 mbox: returns reuseable stream.
 imap: it seems to return reuseable stream.
 maildir: does not return reuseable stream.

So there is a subtle issue with resuseable vs non-resuseable stream,
but
From an old comment:
+    // : A non-resuable stream is already closed at this point, so it
+    // doesn't matter that we're trying to close it again, although
+    // there are some cases such as m_dstOutputStream below where it
+    // does get set to null, so the consistency would be good for
+    // clarity. That is why outStream is set to nullptr above and below.

I thought there was some additional comment somewhere about this
subtle issue later added based on additional comments in the bugzilla.
(Not sure where I put the comment, though...)

I am afraid, though, if we need to explain this with such a verbose
comment to a NEWCOMER who needs to maintain the code, either the original
design was broken or we did not leave enough documentation (I think
the latter definitely adds to the difficulty of maintenance, and I
feel we should put all the relevant comments we could glean from the
seasoned hands during the discussion for long-term maintenance
reasons. Yeah, such addition can't be uniforma and anecdotal, but what
else can we do with the limited man-power?)

> ::: mailnews/import/oexpress/nsOE5File.cpp
> @@ +317,5 @@
> > +    // file descriptor and such.
> > +    // Details: getter_Addrefs actually clears its variable first, so
> > +    // you don't actually pass the previous outStream in. Instead
> > +    // GetNewMsgOutputStream saves the old file stream internally.
> > +
> 
> remove.

I removed the whole comment including the lines preceding lines.

> 
> ::: mailnews/import/oexpress/nsOEMailbox.cpp
> @@ +275,5 @@
> > +    m_mbxFileInputStream = nullptr;
> > +  }
> > +  if (m_dstOutputStream)  {
> > +    rv = m_dstOutputStream->Close();
> > +    // XXX TODO we should check write error
> 
> Remove. One comment above it enough.

DONE. 
 
> @@ +350,5 @@
> >    }
> > +
> > +  // cf. reusable m_dstOutputStream does not have to be closed here.
> > +  // It gets closed in CMbxScanner::CleanUp.
> > +
> 
> The comment is fine if you remove the empty line.
> I asked what "cf." means, but you haven't answered. So I'm asking again. I
> think it would be best not to use abbreviations which are uncommon. See:
> https://en.wikipedia.org/wiki/Cf.

I now I see you asked what "cf." means".
I think I originally used cf. in the sense of the following, but
not entirely sure of how to include the line information a la mxr (now
dxr) and failed to put that location information following cf.

>From Wikipedia, the free encyclopedia

>The abbreviation cf. (short for the Latin: confer, meaning
> "compare")[1] is used in writing to refer the reader to other
> material to make a comparison with the topic being discussed. It is
> used to form a contrast, for example: "Abbott (2010) found supportive
> results in her memory experiment, unlike those of previous work
> (cf. Zeller & Williams, 2007)."[2] Many usage guides recommend
> against using "cf." in place of "see also" to indicate sources of
> additional examples or supporting evidence.[3][4]c.

Back when I was a grad student many years ago, not many usage guides
discouraged the use of "cf.". So the time changes :-)
Anyway, since now I realize CMbxScanner::CleanUp is in the SAME file,
I changed the comment lines to
  // reusable m_dstOutputStream does not have to be closed here.
  // It gets closed in CMbxScanner::CleanUp in this file.

TIA
Attachment #8809861 - Attachment is obsolete: true
Attachment #8810174 - Flags: review?(jorgk)
(Assignee)

Comment 65

5 months ago
(In reply to Jorg K (GMT+1) from comment #63)
> Comment on attachment 8809863 [details] [diff] [review]
> fix-close-MERGED-add-closedflag-IMPORT-directory.patch
> 
> Review of attachment 8809863 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/import/applemail/src/nsAppleMailImport.cpp
> @@ +572,5 @@
> >  
> >        // add the data to the mbox stream
> >        if (NS_SUCCEEDED(nsEmlxHelperUtils::AddEmlxMessageToStream(currentEntry, outStream))) {
> >          mProgress++;
> > +        msgStore->FinishNewMessage(outStream, msgHdr, &closed);
> 
> I'm not sure how this would compile since the function takes two parameters
> only.
> 
> Do you intend to modify this function? Not in this patch though.

Yes, I do intend to change this function.


The order of the patch application ought to be

1. Nulling-filestream-after-close-under-IMPORT-directory.patch

the following four patches MUST be applied simultaneously.

 fix-close-MERGED-base-dir.patch
 fix-close-MERGED-imap-dir.patch
 fix-close-MERGED-local-dir.patch
*fix-close-MERGED-add-closedflag-IMPORT-directory.patch

The change of the said function is done in fix-close-MERGED-base-dir.patch.

TIA
(Assignee)

Comment 66

5 months ago
Created attachment 8810175 [details] [diff] [review]
bug 1242030-e fix-close-MERGED-add-closedflag-IMPORT-directory.patch

Change to reflect the change in  8810174: Nulling-filestream-after-close-under-IMPORT-directory.patch
Attachment #8810175 - Flags: review?(jorgk)

Comment 67

5 months ago
Comment on attachment 8810174 [details] [diff] [review]
bug 1242030-a Nulling-filestream-after-close-under-IMPORT-directory.patch

OK, this doesn't do any damage and nulling 'outputStream' after closing it for good housekeeping seems like a good idea that adds robustness.

Now regarding the subject of comments and documentation. Sadly we have a lot of poorly documented "legacy" code. What would help is to document it. What does *not* help is interspersing some half-true comments which originated from some hearsay.

Take GetNewMsgOutputStream. If DXR serves me well, there are two implementations:
nsMsgBrkMBoxStore::GetNewMsgOutputStream() and nsMsgMaildirStore::GetNewMsgOutputStream() to cater for the two stores we support: mbox and maildir. In comment #64 you got yourself confused: you mention mbox, *imap* and maildir. Imap is not a store type as all, it is a protocol. And yes, we retrieve messages via imap and then store them offline locally in one of the two stores.

The concept of a "reusable" stream is documented here:
https://dxr.mozilla.org/comm-central/rev/82f47febc327f5487158ed2a09331e195ba71ae2/mailnews/base/public/nsIMsgPluggableStore.idl#120

You see what I'm saying? Anecdotal half-truth interspersed in a comment somewhere at a call site somewhere far away from the function doing the (obscure) processing does not help. I appreciate your concern for the newcomers, and we are all newcomers when we first visit code we haven't seen before, but that type of comment will only confuse the newcomer.

If you want a to do a good deed, go and analyse the functions in question and document them. And then you can refer to that saying, for example:
  We don't leak file descriptors here, see ...
As for these file descriptors: Where are they? Usually file descriptors come from Unix file operations, but there are none here. Due to many layers of M-C abstraction we are far removed from the file operations. Are you referring to the output streams?

Another thing: The source you are fixing here is:
nsAppleMailImport.cpp | nsOE5File.cpp | nsOEMailbox.cpp | nsOutlookMail.cpp
As far as I know, Apple import is broken for other reasons since 2011 (bug 688166). OE worked but is dead since 2014, it died with XP. Outlook import is permanently disabled (bug 1176748) and I don't know what OE5 is. So whilst we appreciate your work, this actually doesn't address any issues in any active code.
Attachment #8810174 - Flags: review?(jorgk) → review+

Comment 68

5 months ago
Comment on attachment 8810175 [details] [diff] [review]
bug 1242030-e fix-close-MERGED-add-closedflag-IMPORT-directory.patch

As I said before:
I *cannot* review this since the code will positively *not* compile. We *must* review the patches in the order they need to be applied. I fact, it makes no sense to split dependent changes into multiple patches. Each patch in itself must apply and compile and work. The patch that changes the parameters to a function *must* also fix all the call sites.

Once again, we are investing a lot of time and effort into what is basically *dead* code:
nsAppleMailImport.cpp | nsBeckyMail.cpp | nsOE5File.cpp | nsOEMailbox.cpp | nsOutlookMail.cpp

Of these, only Becky is live and working.
Attachment #8810175 - Flags: review?(jorgk)
(Assignee)

Updated

5 months ago
Attachment #8807620 - Attachment description: fix-close-MERGED-base-dir.patch → bug 1242030-b fix-close-MERGED-base-dir.patch
(Assignee)

Comment 69

5 months ago
(In reply to Jorg K (GMT+1) from comment #68)
> Comment on attachment 8810175 [details] [diff] [review]
> fix-close-MERGED-add-closedflag-IMPORT-directory.patch
> 
> As I said before:
> I *cannot* review this since the code will positively *not* compile. We
> *must* review the patches in the order they need to be applied. I fact, it
> makes no sense to split dependent changes into multiple patches. Each patch
> in itself must apply and compile and work. The patch that changes the
> parameters to a function *must* also fix all the call sites.
> 
> Once again, we are investing a lot of time and effort into what is basically
> *dead* code:
> nsAppleMailImport.cpp | nsBeckyMail.cpp | nsOE5File.cpp | nsOEMailbox.cpp |
> nsOutlookMail.cpp
> 
> Of these, only Becky is live and working.

I can certainly create a gigantic patch that compiles:

From my original post:

> I am loading merged patches from the bug ug 1122698, bug 1134527, bug 1134529, bug  1174500 
> The reason for consolidation is that the patch as a whole is much easier to 
> read when consolidated.

> however, I have split the resulting patch into a few smaller chunks that address files under only 
> certain directories

 [omission]

> This required the changes to files under mailnews/import directory as well.
> The first patch in the following list addresses this need.

> A Nulling-filestream-after-close-under-IMPORT-directory.patch: Patches for...
> A fix-close-MERGED-base-dir.patch: Bug 1122698 Fixing unchecked Close, unc...
> A fix-close-MERGED-imap-dir.patch: Fixing unchecked Close, unchecked Flush...
> A fix-close-MERGED-local-dir.patch: Fixing unchecked Close, unchecked Flus...
> and  another patch that is explained later:
> A enable-buffering-1st-step.patch (explained later in a different bugzilla.)

(I added fix-close-MERGED-add-closedflag-IMPORT-directory.patch to take care of "the changes to files under mailnews/import directory as well": these files are not tested locally because I use linux for testing the code and these files can be compiled only on try-comm-central for me.

These files under mailnews/import could not be tested for quite some time since try-comm-central had an infra issue that broke OSX and Windows buitld in 2015 and thus I did not realize that I had to modify these files until much later. 

I can certainly create a single big patch that compiles.
You can see that these patches as a group have been tested on try-comm-central and ran fine.
E.g.:

b36b0c6cb71a	ISHIKAWA, Chiaki — bug 1242030-e Add third argument to (Finish|Discard)NewMessage().
230a83bc2cd6	ISHIKAWA, Chiaki — bug 1242030-d (consolidation of 1122698, 1134527, 1134529, 1174500)
122b1a7d92b2	ISHIKAWA, Chiaki — bug 1242030-c (consolidation of 1122698, 1134527, 1134529, 1174500)
1f528b25cb13	ISHIKAWA, Chiaki — bug 1242030-b (consolidation of 1122698, 1134527, 1134529, 1174500)
325f6e8485ec	ISHIKAWA, Chiaki — bug 1242030-a: (consolidation of 1122698, 1134527, 1134529, 1174500)

Whether one wants to review such a big patch is another question.
However, I am attaching one FYI.

AHA, I was wondering why you chose to read  fix-close-MERGED-add-closedflag-IMPORT-directory.patch first.
After modifying and uploading the patches so many times, I simply decided to use the mercurial's qpatch file name as comment for the patch. I am editing the comment a little bit to add "1242030-{a,b,c,d,e}".
(Assignee)

Updated

5 months ago
Attachment #8807621 - Attachment description: fix-close-MERGED-local-dir-Rev02.patch → bug 1242030-d fix-close-MERGED-local-dir-Rev02.patch
(Assignee)

Updated

5 months ago
Attachment #8809863 - Attachment description: fix-close-MERGED-add-closedflag-IMPORT-directory.patch → bug 1242030-e fix-close-MERGED-add-closedflag-IMPORT-directory.patch
(Assignee)

Updated

5 months ago
Attachment #8807618 - Attachment description: fix-close-MERGED-imap-dir.patch → bug 1242030-c fix-close-MERGED-imap-dir.patch
(Assignee)

Updated

5 months ago
Attachment #8810175 - Attachment description: fix-close-MERGED-add-closedflag-IMPORT-directory.patch → bug 1242030-e fix-close-MERGED-add-closedflag-IMPORT-directory.patch
(Assignee)

Updated

5 months ago
Attachment #8810174 - Attachment description: Nulling-filestream-after-close-under-IMPORT-directory.patch → bug 1242030-a Nulling-filestream-after-close-under-IMPORT-directory.patch
(Assignee)

Updated

5 months ago
Attachment #8809863 - Attachment is obsolete: true
(Assignee)

Comment 70

5 months ago
Created attachment 8810249 [details] [diff] [review]
FYI: Big consolidated patch (excluding bug 1242030-a Nulling-filestream-after-close-under-IMPORT-directory.patch)

This is a big consolidated patch excluding  bug 1242030-a Nulling-filestream-after-close-under-IMPORT-directory.patch.

Although, it is big, it may be much easier to see the changes in one glance.
(browsing might be more like it.)

If this is preferred, let us go with this patch.

TIA
(Assignee)

Updated

5 months ago
Flags: needinfo?(jorgk)
(Assignee)

Comment 71

5 months ago
(In reply to Jorg K (GMT+1) from comment #67)
> Comment on attachment 8810174 [details] [diff] [review]
> bug 1242030-a Nulling-filestream-after-close-under-IMPORT-directory.patch
> 

Thank you for the review. One down, four more (or a big consolidated patch) to go.

> OK, this doesn't do any damage and nulling 'outputStream' after closing it
> for good housekeeping seems like a good idea that adds robustness.
> 
> Now regarding the subject of comments and documentation. Sadly we have a lot
> of poorly documented "legacy" code. What would help is to document it. What
> does *not* help is interspersing some half-true comments which originated
> from some hearsay.
> 
> Take GetNewMsgOutputStream. If DXR serves me well, there are two
> implementations:
> nsMsgBrkMBoxStore::GetNewMsgOutputStream() and
> nsMsgMaildirStore::GetNewMsgOutputStream() to cater for the two stores we
> support: mbox and maildir. In comment #64 you got yourself confused: you
> mention mbox, *imap* and maildir. Imap is not a store type as all, it is a
> protocol. And yes, we retrieve messages via imap and then store them offline
> locally in one of the two stores.

Yes, I was confused.
Now that you mentioned it and considering the confusion I had, I think it would be better to
splits the local directory into
local -> pop3
         mbox
         maildir
         common (common operation for both mbox and maildir formats, i.e., work with any future
                 pluggable storage format.)

When I was writing the patch the year before and last year, 
I was confused when I had to look for pop3-related code in local directory and further confused when there are maildir-related code in local. (OK, maybe "local" is meant for saying, we have code for handling "local" storage as in "local vs remote", but then pop3 still ought to be in a separate directory IMHO since imap is in an individual directory. But I digress.)
 
> 
> The concept of a "reusable" stream is documented here:
> https://dxr.mozilla.org/comm-central/rev/
> 82f47febc327f5487158ed2a09331e195ba71ae2/mailnews/base/public/
> nsIMsgPluggableStore.idl#120
> 

Thanks for the pointer. I was wrong in looking at the implementation(s) of the code themselves.
The CPP code does not have any comments to it.
E.g.:

NS_IMETHODIMP
nsMsgMaildirStore::GetNewMsgOutputStream(nsIMsgFolder *aFolder,
                                         nsIMsgDBHdr **aNewMsgHdr,
                                         bool *aReusable,
                                         nsIOutputStream **aResult)

I am so used to reading the code with comments at the place of implementation, and I did not realize I should go back to .idl.

Maybe I should ask the dxr developer for a feature to open a separate window automatically 
to show the IDL definition
whenever a user searches for a function name that is defined in an IDL.
(The latter seems to go a long way to save developers many clicks: today we have to ask for "definition" and then visit the named IDL file at the beginning of the automatically generated header file?)

> You see what I'm saying? Anecdotal half-truth interspersed in a comment
> somewhere at a call site somewhere far away from the function doing the
> (obscure) processing does not help. 

Generally speaking, yes, I agree.

But in this particular instance, 
I hate to disagree, but the comment was useful to me, and
this particular comment was not anecdotal half-truth to me.
(See comment 15 )

> I appreciate your concern for the
> newcomers, and we are all newcomers when we first visit code we haven't seen
> before, but that type of comment will only confuse the newcomer.
> 
> If you want a to do a good deed, go and analyse the functions in question
> and document them. And then you can refer to that saying, for example:
>   We don't leak file descriptors here, see ...

Hmm, maybe I should modify .idl comment in more detail, then.

> As for these file descriptors: Where are they? Usually file descriptors come
> from Unix file operations, but there are none here. Due to many layers of
> M-C abstraction we are far removed from the file operations. Are you
> referring to the output streams?

Yes. The descriptor for the output stream, and until it was clear to me that the output stream is kept around for reusable stream, the processing in the code was very obscure.
(True, there is an abstraction, but IMHO, the particular abstraction may not have been either great (do we have to know that a particular stream is reusable or not: for performance reasons, maybe, yes, but still we could have gone a little further or something. I still need to be worried about 
the file descriptor and file seek position, etc. We may never get away with file seek position, of course, but when I think about it, I will check what the .idl says about the file position when we make a file stream buffered.) I don't see great merits in the I/O handling abstraction in TB code except for the portability alone.
As far as I know all the modern OS have the notion of file descriptor/file handle, etc. It is hard to do away with it. Of course, the function names and the like are different and so a wrapper for portability
is good, but I am not sure if the particular abstraction was good or not.
Maybe in FF, since it has to deal with network stream all the time and there is UDP/TCP differences, etc. So the abstraction may have been good.) But again, I digress.


> 
> Another thing: The source you are fixing here is:
> nsAppleMailImport.cpp | nsOE5File.cpp | nsOEMailbox.cpp | nsOutlookMail.cpp
> As far as I know, Apple import is broken for other reasons since 2011 (bug
> 688166). OE worked but is dead since 2014, it died with XP. Outlook import
> is permanently disabled (bug 1176748) and I don't know what OE5 is. So
> whilst we appreciate your work, this actually doesn't address any issues in
> any active code.

As I mention in comment 69, I only modified these files when I realized that I have not tested OSX and Windows very much due to try-comm-central infrastructure issues in 2015 and early 2016 and ran build jobs early this year. (Of course, it helped me to realize the subtle issue of the timing of Close for maildir operation under Windows and eventually I could fix it.)
But the patches in these files came rather late, and I may want to add a few more
checking of the return value of Close(), but given the status of the files, they can wait.

From what you describe, I take that there are no import test for OSX and Windows using some
prepared data(?), then. That is a disappointment. Surely there was a test data for importing operation during the codes in questions were created.

TIA

Comment 72

5 months ago
Let me see whether I can make any sense of this.

As far as I can see, this is a giant clean-up patch that focusses on error handling. That of course can be split into smaller chunks.

Also, you propose to add a parameter to DiscardNewMessage() and FinishNewMessage(). Once again, there are two implementations, one for mbox the other one for maildir.

Your issue if the closing of the output stream. Well for maildir I can tell you that it *is* closed:
https://dxr.mozilla.org/comm-central/rev/dd4587a78cfd200881956427c8d3fd468aaf9758/mailnews/local/src/nsMsgMaildirStore.cpp#642
https://dxr.mozilla.org/comm-central/rev/dd4587a78cfd200881956427c8d3fd468aaf9758/mailnews/local/src/nsMsgMaildirStore.cpp#670

For mbox it is also closed here
https://dxr.mozilla.org/comm-central/rev/dd4587a78cfd200881956427c8d3fd468aaf9758/mailnews/local/src/nsMsgBrkMBoxStore.cpp#695
but not here, most likely because the stream is reusable:
https://dxr.mozilla.org/comm-central/rev/dd4587a78cfd200881956427c8d3fd468aaf9758/mailnews/local/src/nsMsgBrkMBoxStore.cpp#705

I can't approve the change of the IDL and frankly, I don't think it's necessary. Maildir should just close the stream and make a lot of noise when closing fails. I see no point communicating the close-failure to the caller so it can attempt the close again? Why would it work this time?

Mbox only closes the stream in 'discard' but not in 'finish'. Your patch is not going to change this, just look at this snippet:
if (!reusable) {
  if(!closed) <<== new code.
    outputStream->Close();
  outputStream = nullptr;
}	

So if I see it correctly, all this will do is attempt again to close a stream that failed to close before in the maildir case. I don't think I want to approve this, but let's see what Kent says.

Once we decide on the general way forward, we can see how to organise the patches. If it's just improving the error handling, it can certainly be cut into chunks.

Also, I noticed that at least some of this relies on the new code from bug 939548. So lets get that landed first.
Flags: needinfo?(jorgk) → needinfo?(rkent)
(Assignee)

Comment 73

5 months ago
(In reply to Jorg K (GMT+1) from comment #72)
> Let me see whether I can make any sense of this.
> 
> As far as I can see, this is a giant clean-up patch that focusses on error
> handling. That of course can be split into smaller chunks.
> 
> Also, you propose to add a parameter to DiscardNewMessage() and
> FinishNewMessage(). 

This is for reasons during error handling.

First principle: I wanted to to make DsicardMessage() and FinishMessage() not to close the 
filestream that is pointed by the argument passed to them.

The existing crude error recovery scheme in pop3 code tried close the stream and
sets the filestream variable to nullptr to detect the filestream was already closed (or avoid closing an already closed stream.)
[My memory is hazy here. Bulk of the patch was created about 15 months ago or so.]
In the existing code, when I began adding error return value check, I was horrified to 
see Close() returning errors. But they turned out to be NS_BASE_STREAM_CLOSED in many places.
Close() was called on already closed filestream during error processing. 
I was puzzled and yet at the same time realized that someone in the past may have tried to check the return value of Close() for error, but due to this spaghetti-like convoluted code that spews NS_BASE_STREAM_CLOSED when one checks for Close() value, might have given up. This is a very possible scenario.
 
Anyway, I chased why and I realized that the two routines are to blame.
DiscardNewMessage(), FinishNewMessage(): they close the filestream that is pointed at by passed argument and so the
callers who hold the original filestream variables don't know 
they are closed or not and can't use the "set filestream variable nullptr to signal that it is closed" scheme then.
So Basically, I tried to move the "close" operation out of DiscardNewMessage() and FinishNewMessage()
to outside these two functions so that other functions can use the filestream variable status effectively (null or not) and check the Close() return value at appropriate times without seeing this NS_BASE_STREAM_CLOSED in normal cases.
Yes, the change is mainly for making the existing crude error recovery to work at all.
Without my patches the code to check for Close() has to special-case NS_BASE_STREAM_CLOSED in many places, and
that would have been ugly, and the checking of Close() was necessary to make the crude existing error handling to work.

2nd principle: For Maildir under windows, I had to close the stream despite the first principle.

And the reason for adding "closed" flag is to make Maildir operation to work under Windows,
Under Windows, for Maildir to work, 
I cannot seem to defer the closing operation (meaning NOT closing in DiscardMessage() or FinishMessage())
to a later timing in the call sequences during the mail message processing.
From a lot of dumping on try-comm-central and
dumping on linux locally, I figured that this is because the filestream ended up pointing to a file and/or directory 
that is being renamed (moved) or deleted. Under linux and OSX (basically a derivative of FreeBSD), 
a file or directory can be renamed even in there is an open descriptor to it (I think most POSIX-like OS allows it. Most of it.)
However, under Windows, it cannot. ( I don't know if MS still claims Windows as POSIX-compliant.)
Thus I was forced to change the semantics a bit despite my first principle.

That is basically how the patch was developed to the current form.

I think the next paragraph needs to be read/understood from the viewpoint of the above.
However ...

> Once again, there are two implementations, one for mbox
> the other one for maildir.
> 
> Your issue if the closing of the output stream. Well for maildir I can tell
> you that it *is* closed:
> https://dxr.mozilla.org/comm-central/rev/
> dd4587a78cfd200881956427c8d3fd468aaf9758/mailnews/local/src/
> nsMsgMaildirStore.cpp#642
> https://dxr.mozilla.org/comm-central/rev/
> dd4587a78cfd200881956427c8d3fd468aaf9758/mailnews/local/src/
> nsMsgMaildirStore.cpp#670
> 
> For mbox it is also closed here
> https://dxr.mozilla.org/comm-central/rev/
> dd4587a78cfd200881956427c8d3fd468aaf9758/mailnews/local/src/
> nsMsgBrkMBoxStore.cpp#695
> but not here, most likely because the stream is reusable:
> https://dxr.mozilla.org/comm-central/rev/
> dd4587a78cfd200881956427c8d3fd468aaf9758/mailnews/local/src/
> nsMsgBrkMBoxStore.cpp#705
> 
> I can't approve the change of the IDL and frankly, I don't think it's
> necessary. Maildir should just close the stream and make a lot of noise when
> closing fails. I see no point communicating the close-failure to the caller
> so it can attempt the close again? Why would it work this time?
> 
> Mbox only closes the stream in 'discard' but not in 'finish'. Your patch is
> not going to change this, just look at this snippet:
> if (!reusable) {
>   if(!closed) <<== new code.
>     outputStream->Close();
>   outputStream = nullptr;
> }	
> 
> So if I see it correctly, all this will do is attempt again to close a
> stream that failed to close before in the maildir case. I don't think I want
> to approve this, but let's see what Kent says.

However, ...
the original patch was created almost 15 months ago and I have to check this by re-reading this again myself.

I am not sure what you refer to by "this".  It may be the case for non-error cases, but
once an error crops up, the current code cannot detect it properly and the crude error recover does not work at all when the
buffered-write is enabled.

But the basic principle was as I explained in the preceding paragraph(s).
There *IS* a possibility of copy&paste error during the maintenance of the patches that may lead you to the above summary, 
but at least such copy&paste error does not ADD any new errors to |make mozmill| and |make xpcshell-test| and that is what I have checked from time to time on try-comm-central to make sure that my patches that have been changed to cope with bitrot 
do not introduce new errors.
If only all the tree OSs (linux, OSX, and Windows) build worked fine all the time. 

I could only find and fix the issue with Maildir under Windows when finally "Windows" build became operational early this year.
Initially I was tempted to dismiss it as temporary glitch or something like many other timing-related(?) bugs.
But as the issue persisted and so I checked the operation of TB by dumping affected code and finally I figured
it is the open file descriptor to a file or directory that is being renamed or deleted.
If try-comm-central did not have Windows or OSX left in the sand for a prolonged time,
I could have created the whole patch
in the three-four months early in 2015 and noticed the issue with Maildir under Windows sooner and might have come up with a better solution.
If anyone is interested, be my guest.
 
> Once we decide on the general way forward, we can see how to organise the
> patches. If it's just improving the error handling, it can certainly be cut
> into chunks.
> 
> Also, I noticed that at least some of this relies on the new code from bug
> 939548. So lets get that landed first.

Yes.

Thank you again.

Comment 74

5 months ago
I see what you're saying. Trying to track errors of Close() (which IMHO should never fail unless you call it twice) is not helped by overall confused logic which in fact tries to close the same thing twice, à la comment #15: "so it doesn't matter that we're trying to close it again". That's true only if there is no error checking. IMHO the program logic should always track whether it expects a file to be open or not.

"This" referred to the approach you're taking by changing the IDL and adding another parameter.

What I'd like to see is this:
Look at the logic and find the spots were we try to close things twice. Then fix this logic. Once again, I don't see that what you're proposing is meaningful, take:
nsMsgMaildirStore::DiscardNewMessage(
...
  *closedflag = true;
nsMsgMaildirStore::FinishNewMessage(
...
  *closedflag = true;
So here you're always returning true on the new parameter, so why have it?

nsMsgBrkMBoxStore::DiscardNewMessage(
...
  *closedflag = false;
nsMsgBrkMBoxStore::FinishNewMessage(
...
  *closedflag = false;
So here you're always returning true on the new parameter, so why have it?

This code makes no sense:
if (!reusable) {
  // !reusable means mbox.
  if(!closed) <<== new code, always returns 'false' for mbox.
    outputStream->Close();
  outputStream = nullptr;
}	

You don't need the 'closed' flag here if you really want to close the stream (which I haven't looked at).

So please present a short and succinct patch that addresses things not being closed or closed twice. Also please lose all the debugging you added. We can leave debugging in some "hotspots", but let's not clutter the source code with debugging which won't be useful for the next problem.

I believe we can do it without interface changes, so nothing is stopping us from splitting the clean-up into multiple chunks.

Comment 75

5 months ago
I'm not really sure what I am being requested to look at here. I think this cleanup is important work, but I really do not have the time nor (in most cases) the knowledge to give good opinions.

Yes it is good if the same method that opened a file closes it, but that is difficult to accomplish in C++ code sometimes. Yes the existing code is really hard to work with and needs refactoring. But I do not have the time to investigate myself.
Flags: needinfo?(rkent)

Comment 76

5 months ago
The question is whether you are in favour of changing the IDL:
Old:
void discardNewMessage(in nsIOutputStream aOutputStream,	
                       in nsIMsgDBHdr aNewHdr);	

New:
void discardNewMessage(in nsIOutputStream aOutputStream,
                       in nsIMsgDBHdr aNewHdr,
                       out boolean closeflag);

Especially looking at the planned changes, see comment #74, I don't think it makes sense.
Flags: needinfo?(rkent)

Comment 77

5 months ago
Well the planned change in the IDL just seems to codify what at least on the surface appears to be a poor design. Like I see code like this:

  // non-reusable streams will get closed by the store.
  if (reusable)
    destOutputStream->Close();
  return store->FinishNewMessage(destOutputStream, aHdr);

which really sounds like it is unclear who is responsible for closing the file. Yes I would prefer a design where the responsibility is clear rather than amorphous like this, and that would say that adding a parameter about whether the stream was closed or not is a bad idea. But I have not investigated whether fixing that design problem is straightforward.

That is not a clear answer, but to give more would require me to dive in, understand all the issues, and propose a workable clear plan.
Flags: needinfo?(rkent)
(Assignee)

Comment 78

5 months ago
I have a little confusion.

> What I'd like to see is this:
> Look at the logic and find the spots were we try to close things twice. Then
> fix this logic. Once again, I don't see that what you're proposing is
> meaningful, take:
> nsMsgMaildirStore::DiscardNewMessage(
> ...
>   *closedflag = true;
> nsMsgMaildirStore::FinishNewMessage(
> ...
>   *closedflag = true;
> So here you're always returning true on the new parameter, so why have it?
> 
> nsMsgBrkMBoxStore::DiscardNewMessage(
> ...
>   *closedflag = false;
> nsMsgBrkMBoxStore::FinishNewMessage(
> ...
>   *closedflag = false;
> So here you're always returning true on the new parameter, so why have it?

You mean 'false' on the last line, don't you?
For Mbox, I don't close the stream in DiscardNewMessage() and FinishNewMessage().

And this closedflag business was necessitated to the difficult to fathom interaction of open file descriptor to the file and/or directory that is RENAMED/MOVED in the case of Maildir and I needed to close it at the early timing (as in the existing legacy code) and that was why
DiscardNewMessage() and FinishNewMessage() needed to closed it albeit reluctantly.
 
rkent's comment:
>which really sounds like it is unclear who is responsible for closing the file. Yes I would prefer a >design where the responsibility is clear rather than amorphous like this,

I would very much prefer that the caller is responsible to close the stream (so that the caller side rather than the leaf function closes it WITHOUT setting the class variable that refers to the filestream to null. This approach was the only way the crude error recovery in the existing could work, I think.
The crude error handling code relied on filestream variable being set to null after the early close due to error detection and error recovery attempt, but DiscardNewMessage() and FinishNewMessage() closing the stream without setting the class variable to nullptr caused massive screw up during error processing.

I am not even sure if the code worked at all as far as the close() error is concerned....)

Given that there won't be another new storage format in sight in the life time of TB, I agree there can be some simplifications of code as jorg pointed out. . (I never bothered to apply the logic such as mbox is reusable and maildir is not reusable, etc. I changed the code mechanically to avoid errors and to show how the code was changed from the original.
(Assignee)

Comment 79

3 months ago
Created attachment 8826640 [details] [diff] [review]
Big consolidated patch (excluding bug 1242030-a Nulling-filestream-after-close-under-IMPORT-directory.patch)

Hi,

I have updated the Big consolidated patch (excluding bug 1242030 - a Nulling-filestream-after-close-under-IMPORT-directory.patch) to fix the bitrot caused by the changes since last November when it was first uploaded.

It was last modified back in November, but I could not follow it up due to my day job.

Now I have requested a review, but given that there were about two months lapse.
Here is what I would request for the initial feedback/review.

Point 1:
I still don't grok the concern you raised in comment 74, and I wondered about it in comment 78.
Can you elaborate a bit on that?

Point 2:
Other than this issue, I would appreciate the general concern about how the error ought to be reported using what function, etc.
TB as mailer is getting short shrift as far as error notification the user is concerned.

What would Firefox browser tell the user if the page the user requested does not exist at a sight? A big error 404 in the main browser window.

What would Firefox browser tell the user if the site the user tried to access does not seem to exist or seems to exist but does not return a response in reasonable time?  A big error page that says "Server not found" in the main browser window in the first case or some error message regarding timeout or transient network error and try again later type of message in the second case.
Again in the main window.

Right now, contrast this, when the download of the e-mail message fails. I am not sure exactly what error would be provided to the user clearly.
Is this shown in the Error Console which is NOT NECESSARILY OPEN all the time (it is not open by default)?
I am debugging the I/Oerror and how TB copes with it by dumping the error messags to TTY console where TB is manually invoked. This should not be the case for a user-friendly robust mail client.

The above is what I mean by saying that TB is getting short shrift.
We really should think of using Error console or other means of output to show
errors more clearly to the users. Don't listen to browser developers here.
They have it so easy: the main web window is always there for them to report the
error so clearly in a visible manner.

Back to the patch.
I would try to grok point 1 and point 2 to modify the patch and
then go through the review cycles several times (given the size of this patch, I don't expect quick turn around nor the review of the whole patch. A partial review and comment from time to time should give me a clue how to proceed, and I will upload modified patch from time to time.

BTW, the patch was originally created as large patch like this one, but then
it was mentioned that it was difficult to read through the big patch (how one could review such a big patch?) so I tried to separate them per directory. But here it is again in a big chunk as a standalone patch that applies cleanly on its own.
This patch and a few follow-up patches passed the treeherder test rather well (except for the known issues outside the realm of this patch).

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=44fc60a362f9510ecddcd5107f4a39d599c0a3d7

This patch appears as  https://hg.mozilla.org/try-comm-central/rev/07061f4eda60 in the above submission.

TIA

PS: 
I hasten to add that this patch alone WITHOUT the followup patches work.
Attachment #8810249 - Attachment is obsolete: true
Attachment #8826640 - Flags: review?(jorgk)

Comment 80

3 months ago
Looks like I have to repeat what I said in comment #74, so here goes:

You propose to change the IDL and add a parameter 'closeflag' to both discardNewMessage() and finishNewMessage().

nsMsgBrkMBoxStore::DiscardNewMessage() and nsMsgBrkMBoxStore::FinishNewMessage() always return 'false' (since they keep the mbox file open for further operations), nsMsgMaildirStore::DiscardNewMessage() and nsMsgMaildirStore::FinishNewMessage() always return 'true' (since they close the file, or at least try).

I pointed out that
if (!reusable && !closed)
  outputStream->Close();
makes no sense.
For mbox, reusable==true and closed==false, so for mbox, the Close() will not run.
For maildir, reusable==false and closed==true, so for maildir, the Close() will not run.

Before I review this, can you please do the following:
1) Remove all trailing white-space since I don't want to complain about them individually.
2) Take a good hard look at all the chatty comments and reduce them to a bare minimum.
3) Use XXX, not xxx where required.
4) MOZ_ASSERT() can have a second string argument. Use it.
5) Use MOZ_ASSERT() if possible instead of NS_ERROR().
6) Use do { } while false, like we did in bug 939548, instead of goto.
7) When calling xx->Close(), set xx to nullptr after issuing a warning.
   Currently you're undecided whether which comes first. It should be consistent.
8) Make sure you always use two spaces of indentation.

And do split the patch into multiple parts, at least.
1) nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp
2) nsImapIncomingServer.cpp | nsImapMailFolder.cpp | nsImapOfflineSync.cpp | nsImapProtocol.cpp | nsImapService.cpp | nsImapUndoTxn.cpp
3) nsAppleMailImport.cpp | nsBeckyMail.cpp | nsOE5File.cpp | nsOEMailbox.cpp | nsOutlookMail.cpp
4) nsLocalMailFolder.cpp | nsMailboxProtocol.cpp | nsMovemailService.cpp | nsMsgBrkMBoxStore.cpp | nsMsgLocalStoreUtils.cpp | nsMsgMaildirStore.cpp | nsParseMailbox.cpp
5) nsPop3Protocol.cpp | nsPop3Service.cpp | nsPop3Sink.cpp | nsPop3Sink.h

Updated

3 months ago
Attachment #8826640 - Flags: review?(jorgk)
(Assignee)

Comment 81

3 months ago
(In reply to Jorg K (GMT+1) from comment #80)
> Looks like I have to repeat what I said in comment #74, so here goes:
> 
> You propose to change the IDL and add a parameter 'closeflag' to both
> discardNewMessage() and finishNewMessage().
> 
> nsMsgBrkMBoxStore::DiscardNewMessage() and
> nsMsgBrkMBoxStore::FinishNewMessage() always return 'false' (since they keep
> the mbox file open for further operations),
> nsMsgMaildirStore::DiscardNewMessage() and
> nsMsgMaildirStore::FinishNewMessage() always return 'true' (since they close
> the file, or at least try).
> 
> I pointed out that
> if (!reusable && !closed)
>   outputStream->Close();
> makes no sense.
> For mbox, reusable==true and closed==false, so for mbox, the Close() will
> not run.
> For maildir, reusable==false and closed==true, so for maildir, the Close()
> will not run.

Now I see it.

> 
> Before I review this, can you please do the following:
> 1) Remove all trailing white-space since I don't want to complain about them
> individually.

Fine. But there are white-spaces left over from the original file: I suppose you don't mind
the wholesale removal of trailing blanks from the original files.
I found the trailing blanks in the original files rather annoying myself.
 
> 2) Take a good hard look at all the chatty comments and reduce them to a
> bare minimum.
Will do.
> 3) Use XXX, not xxx where required.
Will do.

> 4) MOZ_ASSERT() can have a second string argument. Use it.
I will. Not sure if the second argument was available a year ago or so.

> 5) Use MOZ_ASSERT() if possible instead of NS_ERROR().
Yes, I will. I was moving in this direction after realizing NS_ERROR() will abort?

> 6) Use do { } while false, like we did in bug 939548, instead of goto.
OK. Will do.

> 7) When calling xx->Close(), set xx to nullptr after issuing a warning.
>    Currently you're undecided whether which comes first. It should be
> consistent.
I was not aware where I was inconsistent, but will look them up.

> 8) Make sure you always use two spaces of indentation.
I hope you won't mind my fixing EMACS modeline in the original files because
basically I am just trying to follow what EMACS modeline of each files dictates my 
emacs session. Unfortunately it seems there are files with 2 or 4 indentations.
I hope OTHERS won't mind changing these modelines. It is hard to cate to different people's taste.
Yes, there ARE files that the EMACS modeline setting and the indentation in the file does not match at all :-(

> 
> And do split the patch into multiple parts, at least.
> 1) nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp
> 2) nsImapIncomingServer.cpp | nsImapMailFolder.cpp | nsImapOfflineSync.cpp |
> nsImapProtocol.cpp | nsImapService.cpp | nsImapUndoTxn.cpp
> 3) nsAppleMailImport.cpp | nsBeckyMail.cpp | nsOE5File.cpp | nsOEMailbox.cpp
> | nsOutlookMail.cpp
> 4) nsLocalMailFolder.cpp | nsMailboxProtocol.cpp | nsMovemailService.cpp |
> nsMsgBrkMBoxStore.cpp | nsMsgLocalStoreUtils.cpp | nsMsgMaildirStore.cpp |
> nsParseMailbox.cpp
> 5) nsPop3Protocol.cpp | nsPop3Service.cpp | nsPop3Sink.cpp | nsPop3Sink.h

I can certainly do this, but then these separate patches are likely to be applied in a group at the same time.

I will keep busy.

TIA

Comment 82

3 months ago
Yes, I do mind a wholesale removal of trailing spaces since it makes a review absolutely impossible. Please don't do that. I was referring to trailing spaces you added. Please click on "Review", they are all clearly visible in *pink*. Even non-reviewers are allowed to look at their own patch in review mode.

Yes, all the parts will land together, but I can review one at a time and we don't have to touch a once finished patch again.
(Assignee)

Comment 83

3 months ago
I am going to upload five patches that try to address the requests.

1) nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp
2) nsImapIncomingServer.cpp | nsImapMailFolder.cpp | nsImapOfflineSync.cpp | nsImapProtocol.cpp | nsImapService.cpp | nsImapUndoTxn.cpp
3) nsAppleMailImport.cpp | nsBeckyMail.cpp | nsOE5File.cpp | nsOEMailbox.cpp | nsOutlookMail.cpp
4) nsLocalMailFolder.cpp | nsMailboxProtocol.cpp | nsMovemailService.cpp | nsMsgBrkMBoxStore.cpp | nsMsgLocalStoreUtils.cpp | nsMsgMaildirStore.cpp | nsParseMailbox.cpp
5) nsPop3Protocol.cpp | nsPop3Service.cpp | nsPop3Sink.cpp | nsPop3Sink.h

Comments about your requests and how I tried to meet them.

(In reply to ISHIKAWA, Chiaki from comment #81)
> (In reply to Jorg K (GMT+1) from comment #80)
> > Looks like I have to repeat what I said in comment #74, so here goes:
> > 
> > You propose to change the IDL and add a parameter 'closeflag' to both
> > discardNewMessage() and finishNewMessage().
> > 
> > nsMsgBrkMBoxStore::DiscardNewMessage() and
> > nsMsgBrkMBoxStore::FinishNewMessage() always return 'false' (since they keep
> > the mbox file open for further operations),
> > nsMsgMaildirStore::DiscardNewMessage() and
> > nsMsgMaildirStore::FinishNewMessage() always return 'true' (since they close
> > the file, or at least try).
> > 
> > I pointed out that
> > if (!reusable && !closed)
> >   outputStream->Close();
> > makes no sense.
> > For mbox, reusable==true and closed==false, so for mbox, the Close() will
> > not run.
> > For maildir, reusable==false and closed==true, so for maildir, the Close()
> > will not run.
> 
> Now I see it.
> 

These redundant or cant-happen codes are all in import directory.
They were done much later than other parts, and when I modified the codes I was not quite sure if these
strange conditions happen or not. You show that the condition does not.

For now, I turn the code into MOZ_ASSERT() to see if my change ever triggers this strange
condition (it should not). 
For a big change suggested in this bugzilla entry, this sanity check should not 
hurt. If the patches stabilize and we become confident, we may remove the MOZ_ASSERT() 
 
> > Before I review this, can you please do the following:
> > 1) Remove all trailing white-space since I don't want to complain about them
> > individually.
> 
> Fine. But there are white-spaces left over from the original file: I suppose
> you don't mind
> the wholesale removal of trailing blanks from the original files.
> I found the trailing blanks in the original files rather annoying myself.
>  
I did not do the wholesale removal.
I tried to delete extra trailing blanks my patch introduced, but also reverted my whitespace changes
my patch has introduced. 

[In the long run, we may want to set up a "flag day" when only the whitespace changes are applied to the source tree
to fix the strange indentations left in the code and remove trailing blanks.]

> > 2) Take a good hard look at all the chatty comments and reduce them to a
> > bare minimum.
> Will do.

I have minimized the comment. 
But it is up to you to decide to judge if it is minimum or can still be trimmed.
I left some hints for future error recovery, etc. hoping for new bloods to work on remaining issues.
But maybe I am too optimistic. If you, aceman and I are the only people who would touch the code, maybe we can further trim down the comment since we all know what are attempted.

> > 3) Use XXX, not xxx where required.
> Will do.

I turned xxx into XXX, but is there any guideline about 
 - describing lacking features,
 - describing remaining buggy "features",
 - suggesting future improvement (even possible algorithms), etc.
other than simply using "XXX" ?
 
> > 4) MOZ_ASSERT() can have a second string argument. Use it.
> I will. Not sure if the second argument was available a year ago or so.

I used MOZ_ASSERT().
In the long run, someone who is familiar with the code might want to turn 
warning messages printed by NS_WARN_IF() construct (?), but I found the use of
"<<" to shut up "value not used" warning extremely UGLY. Can't we have a macro or something for C-C TB?

> 
> > 5) Use MOZ_ASSERT() if possible instead of NS_ERROR().
> Yes, I will. I was moving in this direction after realizing NS_ERROR() will
> abort?

Yes, I tried to change all NS_ERROR()'s introduced in the patch to MOZ_ASSERT()'s.

> 
> > 6) Use do { } while false, like we did in bug 939548, instead of goto.
> OK. Will do.
> 
There is only one place where I introduced goto (where a semaphore/lock was acquired and
needs to be released in the error exit path.)
I used |do { } while (false)| construct and replaced |goto| with |break|.

> > 7) When calling xx->Close(), set xx to nullptr after issuing a warning.
> >    Currently you're undecided whether which comes first. It should be
> > consistent.
> I was not aware where I was inconsistent, but will look them up.

I followed the suggestion. Now error/warning message is generated and then the pointer is set to nullptr.
 
> > 8) Make sure you always use two spaces of indentation.
> I hope you won't mind my fixing EMACS modeline in the original files because
> basically I am just trying to follow what EMACS modeline of each files
> dictates my 
> emacs session. Unfortunately it seems there are files with 2 or 4
> indentations.
> I hope OTHERS won't mind changing these modelines. It is hard to cate to
> different people's taste.
> Yes, there ARE files that the EMACS modeline setting and the indentation in
> the file does not match at all :-(

I tried. I modified the emacs mode line setting in the affected file, too.
There is a place where my new modification was inside a strangely indented (not following the two space
indentation) which I re-indented using the two space rule (including my modification).
 
 
> > 
> > And do split the patch into multiple parts, at least.
> > 1) nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp
> > 2) nsImapIncomingServer.cpp | nsImapMailFolder.cpp | nsImapOfflineSync.cpp |
> > nsImapProtocol.cpp | nsImapService.cpp | nsImapUndoTxn.cpp
> > 3) nsAppleMailImport.cpp | nsBeckyMail.cpp | nsOE5File.cpp | nsOEMailbox.cpp
> > | nsOutlookMail.cpp
> > 4) nsLocalMailFolder.cpp | nsMailboxProtocol.cpp | nsMovemailService.cpp |
> > nsMsgBrkMBoxStore.cpp | nsMsgLocalStoreUtils.cpp | nsMsgMaildirStore.cpp |
> > nsParseMailbox.cpp
> > 5) nsPop3Protocol.cpp | nsPop3Service.cpp | nsPop3Sink.cpp | nsPop3Sink.h
> 

Five patches are grouped according to the file grouping above.

> I can certainly do this, but then these separate patches are likely to be
> applied in a group at the same time.
> 
> I will keep busy.
> 
> TIA

The patches have been tested extensively under Debian GNU/Linux 64-bit and
the latest one saw only three errors in |make mozmill| and they are also in the comm-central tryserver jobs.
 
I could not get the patches built on try-comm-central due to some strange build issues.

I took care of windows-only syntax error noticed earlier, etc.

TIA

PS: I took the liberty of MERGING  bug 1242030-a Nulling-filestream-after-close-under-IMPORT-directory.patch  into this patches (It has been merged into the 3rd patch now.)
(Assignee)

Comment 84

3 months ago
Created attachment 8830774 [details] [diff] [review]
1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp

Patch for the following files.
1) nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp
Attachment #8807618 - Attachment is obsolete: true
Attachment #8807620 - Attachment is obsolete: true
Attachment #8807621 - Attachment is obsolete: true
Attachment #8810174 - Attachment is obsolete: true
Attachment #8810175 - Attachment is obsolete: true
Attachment #8826640 - Attachment is obsolete: true
Attachment #8830774 - Flags: review?(jorgk)
(Assignee)

Comment 85

3 months ago
Created attachment 8830778 [details] [diff] [review]
1242030-new-part-2-imap.patch

Patch for the following files under mailnews/imap directory:

2) nsImapIncomingServer.cpp | nsImapMailFolder.cpp | nsImapOfflineSync.cpp |
 nsImapProtocol.cpp | nsImapService.cpp | nsImapUndoTxn.cpp
Attachment #8830778 - Flags: review?(jorgk)
(Assignee)

Comment 86

3 months ago
Created attachment 8830782 [details] [diff] [review]
1242030-new-part-3-import.patch

Patch for the following files under mailnews/import directory:

3) nsAppleMailImport.cpp | nsBeckyMail.cpp | nsOE5File.cpp | nsOEMailbox.cpp
 | nsOutlookMail.cpp

I am locally creating patches under Debian GNU/Linux and so can't test the windows binaries locally (especially the import from the other mail programs).
Attachment #8830782 - Flags: review?(jorgk)
(Assignee)

Comment 87

3 months ago
Created attachment 8830785 [details] [diff] [review]
1242030-new-part-4-local-less-pop3.patch

Patches for the following files under mailnews/local: (except for pop3-related files, which are changed in the part-5).

4) nsLocalMailFolder.cpp | nsMailboxProtocol.cpp | nsMovemailService.cpp |
 nsMsgBrkMBoxStore.cpp | nsMsgLocalStoreUtils.cpp | nsMsgMaildirStore.cpp |
 nsParseMailbox.cpp
Attachment #8830785 - Flags: review?(jorgk)
(Assignee)

Comment 88

3 months ago
Created attachment 8830787 [details] [diff] [review]
1242030-new-part-5-pop3.patch

This is the patch for the following POP3-related files under mailnews/local:


5) nsPop3Protocol.cpp | nsPop3Service.cpp | nsPop3Sink.cpp | nsPop3Sink.h
Attachment #8830787 - Flags: review?(jorgk)

Comment 89

3 months ago
Comment on attachment 8830774 [details] [diff] [review]
1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp

Review of attachment 8830774 [details] [diff] [review]:
-----------------------------------------------------------------

Great. In this size I can review this patches. Thanks for the persistence. Just a few nits here.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +780,4 @@
>    nsCOMPtr<nsIMsgDBHdr> hdr;
>    rv = mDatabase->GetMsgHdrForKey(msgKey, getter_AddRefs(hdr));
> +  NS_ENSURE_TRUE(hdr, NS_OK);   // XXX is returning NS_OK in existing code correct?
> +  NS_ENSURE_SUCCESS(rv, rv);    // XXX why are we returning ERROR value here and not NS_OK?

Remove comment. Returning error is normal. You're already questioning the two 'ENSURE's above. Most likely, we're done when we can't find the database or the key.

@@ +788,5 @@
>    rv = GetMsgInputStream(hdr, &reusable, aFileStream);
> +
> +  // XXX: Ugh, error check was missing previously. 2015 Dec.
> +  // FURTHERMORE, we need to set the message status to OFFLINE before early error return!
> +  // March, 2016.

Remove comment. We don't need to know about 2015 and 2016 ;-)

@@ +795,5 @@
> +    fprintf(stderr,"(debug)  nsMsgDBFolder::GetOfflineFileStream:  GetMsgInputStream(hdr, &reusable, aFileStream); rv=0x%08xn", (unsigned int) rv);
> +#endif
> +
> +    if (mDatabase)
> +      mDatabase->MarkOffline(msgKey, false, nullptr);

Explain WHY you need to do this.

@@ +903,5 @@
> +#ifdef DEBUG
> +  // During testing of running TB under linux using another linux CIFS server
> +  // as mail store, we get error here.
> +  // Quite likely a READ error at low-level.
> +  // This error is observed again on March 15, 2015

Remove date.

@@ +915,5 @@
>    nsCOMPtr<nsISeekableStream> seekableStream(do_QueryInterface(*aInputStream));
>    if (seekableStream)
>      rv = seekableStream->Seek(PR_SEEK_SET, offset);
> +  if (!(seekableStream || !offset)) {
> +    MOZ_ASSERT(0, "non-zero offset w/ non-seekable stream");

That's not how MOZ_ASSERT() should be used. You use it like:

MOZ_ASSERT(this condition must be true, ...);

@@ +1722,5 @@
>    m_bytesAddedToLocalMsg = result.Length();
>  
>    nsCOMPtr <nsISeekableStream> seekable;
>  
> +  MOZ_ASSERT(m_tempMessageStream);

Add second argument here.

@@ +1788,5 @@
>    if (seekable)
>    {
>      mDatabase->MarkOffline(messageKey, true, nullptr);
> +
> +    MOZ_ASSERT(m_tempMessageStream);

Add second argument here.

@@ +1818,5 @@
>         mDatabase->MarkOffline(messageKey, false, nullptr);
>         // we should truncate the offline store at messageOffset
>         ReleaseSemaphore(static_cast<nsIMsgFolder*>(this));
> +       if (msgStore) {
> +         // DiscardNewMessage may not close the stream all the time. See bug 1121842

I don't think the reference to the bug helps much. I'd prefer to remove it.

@@ +1824,5 @@
> +         // Close m_tempMessageStream here if not closed by DiscardNewMessage.
> +         if(!closed && m_tempMessageStream) {
> +           rv2 = m_tempMessageStream->Close();
> +         }
> +         m_tempMessageStream = nullptr; // catch reuse with SIGSEGV

What? We catch a reuse with a SIGSEGV?? Put:
Avoid access of closed stream, or similar.

@@ +1827,5 @@
> +         }
> +         m_tempMessageStream = nullptr; // catch reuse with SIGSEGV
> +       }
> +       // We should check for errors of rv1 and rv2 but for now only
> +       // prints error messages.

English derailed here: ..., but for now we only print ...
Attachment #8830774 - Flags: review?(jorgk) → feedback+
(Assignee)

Comment 90

3 months ago
(In reply to Jorg K (GMT+1) from comment #89)
> Comment on attachment 8830774 [details] [diff] [review]
> 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp
> 
> Review of attachment 8830774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great. In this size I can review this patches. Thanks for the persistence.
> Just a few nits here.
> 
Thank you for the comment.
I will see what I can do clean up my patch.

TIA
(Assignee)

Comment 91

3 months ago
Created attachment 8832545 [details] [diff] [review]
(take 2) 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp
Attachment #8830774 - Attachment is obsolete: true
Attachment #8832545 - Flags: review?(jorgk)
(Assignee)

Updated

3 months ago
Attachment #8832545 - Attachment description: (take 2) 12421242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp030-new-part-1.patch → (take 2) 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp
(Assignee)

Comment 92

3 months ago
Created attachment 8832546 [details] [diff] [review]
(take 2) 1242030-new-part-2-imap.patch
Attachment #8830778 - Attachment is obsolete: true
Attachment #8830778 - Flags: review?(jorgk)
Attachment #8832546 - Flags: review?(jorgk)
(Assignee)

Comment 93

3 months ago
Created attachment 8832548 [details] [diff] [review]
(take 2) 1242030-new-part-3-import.patch
Attachment #8830782 - Attachment is obsolete: true
Attachment #8830782 - Flags: review?(jorgk)
Attachment #8832548 - Flags: review?(jorgk)
(Assignee)

Comment 94

3 months ago
Created attachment 8832549 [details] [diff] [review]
(take 2) 1242030-new-part-4-local-less-pop3.patch
Attachment #8830785 - Attachment is obsolete: true
Attachment #8830785 - Flags: review?(jorgk)
Attachment #8832549 - Flags: review?(jorgk)
(Assignee)

Updated

3 months ago
Attachment #8832548 - Attachment description: 1242030-new-part-3-import.patch → (take 2) 1242030-new-part-3-import.patch
(Assignee)

Comment 95

3 months ago
Created attachment 8832550 [details] [diff] [review]
(take 2) 1242030-new-part-5-pop3.patch
Attachment #8830787 - Attachment is obsolete: true
Attachment #8830787 - Flags: review?(jorgk)
Attachment #8832550 - Flags: review?(jorgk)
(Assignee)

Comment 96

3 months ago
(In reply to Jorg K (GMT+1) from comment #89)
> Comment on attachment 8830774 [details] [diff] [review]
> 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp
> 
> Review of attachment 8830774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great. In this size I can review this patches. Thanks for the persistence.
> Just a few nits here.
> 
Thank you again for your comment. I hope the atom vs string and folder release issues will be resolved soon.

In the meantime, I updated the patches.
Here are some comments.

> ::: mailnews/base/util/nsMsgDBFolder.cpp
> @@ +780,4 @@
> >    nsCOMPtr<nsIMsgDBHdr> hdr;
> >    rv = mDatabase->GetMsgHdrForKey(msgKey, getter_AddRefs(hdr));
> > +  NS_ENSURE_TRUE(hdr, NS_OK);   // XXX is returning NS_OK in existing code correct?
> > +  NS_ENSURE_SUCCESS(rv, rv);    // XXX why are we returning ERROR value here and not NS_OK?
> 
> Remove comment. Returning error is normal. You're already questioning the
> two 'ENSURE's above. Most likely, we're done when we can't find the database
> or the key.

I removed the two comments mentioned here, but also removed the other ENSURE comments as well.
I am not sure if that was the intention, but back to the original no-comment status.

> 
> @@ +788,5 @@
> >    rv = GetMsgInputStream(hdr, &reusable, aFileStream);
> > +
> > +  // XXX: Ugh, error check was missing previously. 2015 Dec.
> > +  // FURTHERMORE, we need to set the message status to OFFLINE before early error return!
> > +  // March, 2016.
> 
> Remove comment. We don't need to know about 2015 and 2016 ;-)

I removed the comment.

> @@ +795,5 @@
> > +    fprintf(stderr,"(debug)  nsMsgDBFolder::GetOfflineFileStream:  GetMsgInputStream(hdr, &reusable, aFileStream); rv=0x%08xn", (unsigned int) rv);
> > +#endif
> > +
> > +    if (mDatabase)
> > +      mDatabase->MarkOffline(msgKey, false, nullptr);
> 
> Explain WHY you need to do this.

I added a comment thusly:

+    // We are exiting early without doing the processing in the latter half of this
+    // function.
+    // One thing the latter half does must be repeated here before early return.
+    // "If we could not find a offline stream, clear the offline flag on the msag and return false,
+    // which will fall back to reading the message from the server.

> 
> @@ +903,5 @@
> > +#ifdef DEBUG
> > +  // During testing of running TB under linux using another linux CIFS server
> > +  // as mail store, we get error here.
> > +  // Quite likely a READ error at low-level.
> > +  // This error is observed again on March 15, 2015
> 
> Remove date.
I removed the date line, but retained the first three lines.


> 
> @@ +915,5 @@
> >    nsCOMPtr<nsISeekableStream> seekableStream(do_QueryInterface(*aInputStream));
> >    if (seekableStream)
> >      rv = seekableStream->Seek(PR_SEEK_SET, offset);
> > +  if (!(seekableStream || !offset)) {
> > +    MOZ_ASSERT(0, "non-zero offset w/ non-seekable stream");
> 
> That's not how MOZ_ASSERT() should be used. You use it like:
> 
> MOZ_ASSERT(this condition must be true, ...);
> 

OK, in many places in this patch file and the other four patch files, I tried follow this suggestion.

However, there are cases where MOZ_ASSERT() is used to print "must not happen" "cannot happen" type of sanity check purposes after if() { } else if ()  {} ... else { MOZ_ASSERT(0, "..."); }

In those cases, I left the usage of MOZ_ASSERT(0, "... string to explain the situation...")
since there is NO single succinct condition for the first argument of MOZ_ASSERT().

Also, in many places I left the construct of
  if (!expr) {
    MOZ_ASSERT(expr, "failure message");
  }

Since we want to check the condition for |if (!expr)|  to report some error conditions or
in the long term, invoke some recovery, etc.
Use of MOZ_ASSERT(expr, "...") is a short term solution for this and may be replaced in the future.
In that sense, keeping if (!expr) { } construct surrounding it would be useful for coding purposes
(and making it easy for merging the changes in the planned future patch sets, too, on my local PC.
Sometimes |diff| produces really ugly output if |{ }| is added after |if (expr)|, etc. and having the |{ }| from the beginning makes it easier to deal with handling conflicting patches.:
The follow-on patch I am thinking of is:
Bug 1170606 - C-C T-B fixes to take care of short read (part of [META] Failure to deal with short read)  }
 

> @@ +1722,5 @@
> >    m_bytesAddedToLocalMsg = result.Length();
> >  
> >    nsCOMPtr <nsISeekableStream> seekable;
> >  
> > +  MOZ_ASSERT(m_tempMessageStream);
> 
> Add second argument here.

This is more like a sanity check, and so I modified this as

+    MOZ_ASSERT(m_tempMessageStream, "Temporary message stream must not be nullptr. (Sanity check).");

> 
> @@ +1788,5 @@
> >    if (seekable)
> >    {
> >      mDatabase->MarkOffline(messageKey, true, nullptr);
> > +
> > +    MOZ_ASSERT(m_tempMessageStream);
> 
> Add second argument here.

ditto as above.

> 
> @@ +1818,5 @@
> >         mDatabase->MarkOffline(messageKey, false, nullptr);
> >         // we should truncate the offline store at messageOffset
> >         ReleaseSemaphore(static_cast<nsIMsgFolder*>(this));
> > +       if (msgStore) {
> > +         // DiscardNewMessage may not close the stream all the time. See bug 1121842
> 
> I don't think the reference to the bug helps much. I'd prefer to remove it.

I removed it.
So the comment reads as
+         // DiscardNewMessage may not close the stream all the time.

> 
> @@ +1824,5 @@
> > +         // Close m_tempMessageStream here if not closed by DiscardNewMessage.
> > +         if(!closed && m_tempMessageStream) {
> > +           rv2 = m_tempMessageStream->Close();
> > +         }
> > +         m_tempMessageStream = nullptr; // catch reuse with SIGSEGV
> 
> What? We catch a reuse with a SIGSEGV?? Put:
> Avoid access of closed stream, or similar.

Changed.
(Original intention was to disovering places where the illegal/unwanted use of already closed stream was done by crashing the code.)



+         m_tempMessageStream = nullptr; // avoid accessing closed stream

This change is also performed in other patch files.



> 
> @@ +1827,5 @@
> > +         }
> > +         m_tempMessageStream = nullptr; // catch reuse with SIGSEGV
> > +       }
> > +       // We should check for errors of rv1 and rv2 but for now only
> > +       // prints error messages.
> 
> English derailed here: ..., but for now we only print ...

Fixed this.

I modified other patch files to reflect the suggested changes of MOZ_ASSERT() and changed the comment about SIGSEGV.
So I refreshed other patches, too.

TIa

Comment 97

3 months ago
Comment on attachment 8832545 [details] [diff] [review]
(take 2) 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp

Review of attachment 8832545 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +782,5 @@
> +
> +    // We are exiting early without doing the processing in the latter half of this
> +    // function.
> +    // One thing the latter half does must be repeated here before early return.
> +    // "If we could not find a offline stream, clear the offline flag on the msag and return false,

Nits: Remove opening quote. "an offline stream". Typo: msag.

@@ +1005,5 @@
>    NS_ENSURE_ARG_POINTER(aMsgDatabase);
>    nsresult rv = OpenBackupMsgDatabase();
> +#ifdef DEBUG
> +  // Mounting linux CIFS-server and using it as mail store produced
> +  // errors with TB running under linux when network error is encountered.

Nit: Linux.

@@ +1826,1 @@
>  #ifdef _DEBUG

Not your fault, but should this be DEBUG instead of _DEBUG?
Attachment #8832545 - Flags: review?(jorgk) → review+

Comment 98

3 months ago
Comment on attachment 8832548 [details] [diff] [review]
(take 2) 1242030-new-part-3-import.patch

Review of attachment 8832548 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine. If you really want the paranoia, make it all the same.
You realise that this is 80% dead code. Becky works, Apple, Outlook Express (x2) and Outlook are all dead.
Sorry, I'm not doing the patches in order since I'm looking for some easy wins on a Saturday night ;-)

::: mailnews/import/applemail/src/nsAppleMailImport.cpp
@@ +573,5 @@
>        }
> +      if (!reusable) {
> +        MOZ_ASSERT(closed, "DiscardNewMessage ought to have closed the stream for non-reusable stream");   // See Bug 1242030 comment 80
> +        outStream = nullptr;
> +      }

I think we can lose this paranoia.

::: mailnews/import/becky/src/nsBeckyMail.cpp
@@ +536,5 @@
> +      rv = msgStore->FinishNewMessage(outputStream, msgHdr, &closed);
> +      // Sanity check
> +      // This condition should never happen as of Jan, 2017.
> +      // See Bug 1242030 comment 80
> +      MOZ_ASSERT(reusable || closed, "!reusable && !closed should not happen.");

Or at least make all the paranoias the same. Here's a different variation with no setting of the stream to nullptr.

@@ +560,2 @@
>      if (NS_FAILED(rv))
> +      msgStore->DiscardNewMessage(outputStream, msgHdr, &closed);

And here you don't have paranoia. I didn't check whether 'reusable' is available here.
Attachment #8832548 - Flags: review?(jorgk) → feedback+
(Assignee)

Comment 99

3 months ago
Thank you for the comments. I was away from my PC for the weekend. So I will look into the issues you raised tomorrow.
(Assignee)

Comment 100

3 months ago
Created attachment 8834473 [details] [diff] [review]
(take 3) 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp

(In reply to Jorg K (GMT+1) from comment #97)
> Comment on attachment 8832545 [details] [diff] [review]
> (take 2) 1242030-new-part-1.patch: nsIMsgPluggableStore.idl |
> nsMsgDBFolder.cpp
> 
> Review of attachment 8832545 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!

Thanks!

> 
> ::: mailnews/base/util/nsMsgDBFolder.cpp
> @@ +782,5 @@
> > +
> > +    // We are exiting early without doing the processing in the latter half of this
> > +    // function.
> > +    // One thing the latter half does must be repeated here before early return.
> > +    // "If we could not find a offline stream, clear the offline flag on the msag and return false,
> 
> Nits: Remove opening quote. "an offline stream". Typo: msag.
> 

Fixed.

> @@ +1005,5 @@
> >    NS_ENSURE_ARG_POINTER(aMsgDatabase);
> >    nsresult rv = OpenBackupMsgDatabase();
> > +#ifdef DEBUG
> > +  // Mounting linux CIFS-server and using it as mail store produced
> > +  // errors with TB running under linux when network error is encountered.
> 
> Nit: Linux.

OK, I thought since Linux is not an acronym, it started with a lower letter unlike acronyms such as FORTRAN which stands for FORmula TRANslation.
I fixed this and other instances of "linux" I inserted into the patches
into "Linux". (So I am updating all the patch files together).
However, do note that the predefined C macro under Linux is "linux".
How confusing...

> @@ +1826,1 @@
> >  #ifdef _DEBUG
> 
> Not your fault, but should this be DEBUG instead of _DEBUG?

I think DEBUG is what is intended. So I changed into that form. At the same time, I noticed there are "#ifdef DEBUG_bienvenu1" constructs in this file, and it could have been the demangled/mistyped form for that. But I have no idea.

TIA
Attachment #8832545 - Attachment is obsolete: true
Attachment #8834473 - Flags: review?(jorgk)
(Assignee)

Comment 101

3 months ago
Created attachment 8834478 [details] [diff] [review]
(Take 3) 1242030-new-part-3-import.patch

(In reply to Jorg K (GMT+1) from comment #98)
> Comment on attachment 8832548 [details] [diff] [review]
> (take 2) 1242030-new-part-3-import.patch
> 
> Review of attachment 8832548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine. If you really want the paranoia, make it all the same.

Sorry I am not being paranoid uniformly :-)

> You realise that this is 80% dead code. Becky works, Apple, Outlook Express
> (x2) and Outlook are all dead.
> Sorry, I'm not doing the patches in order since I'm looking for some easy
> wins on a Saturday night ;-)

That is fine!

I only changed this file at the last moment when I modified the parameters to
a couple of functions and ONLY AFTER I realized that these would not build under Windows (!) on try-central.
 
> ::: mailnews/import/applemail/src/nsAppleMailImport.cpp
> @@ +573,5 @@
> >        }
> > +      if (!reusable) {
> > +        MOZ_ASSERT(closed, "DiscardNewMessage ought to have closed the stream for non-reusable stream");   // See Bug 1242030 comment 80
> > +        outStream = nullptr;
> > +      }
> 
> I think we can lose this paranoia.

Fixed.
 
> ::: mailnews/import/becky/src/nsBeckyMail.cpp
> @@ +536,5 @@
> > +      rv = msgStore->FinishNewMessage(outputStream, msgHdr, &closed);
> > +      // Sanity check
> > +      // This condition should never happen as of Jan, 2017.
> > +      // See Bug 1242030 comment 80
> > +      MOZ_ASSERT(reusable || closed, "!reusable && !closed should not happen.");
> 
> Or at least make all the paranoias the same. Here's a different variation
> with no setting of the stream to nullptr.

Well, since this was near the function inside this code snippet appears,
I thought it was a little overkill to set the stream to nullptr. But for the sake of code uniformity (that should make review easier), I inserted the assignment to nullptr. 
I also did this in another place for the sake of code uniformity.
In that place, the stream is actually set to another value, but again the code uniformity shall prevail.
 
> @@ +560,2 @@
> >      if (NS_FAILED(rv))
> > +      msgStore->DiscardNewMessage(outputStream, msgHdr, &closed);
> 
> And here you don't have paranoia. I didn't check whether 'reusable' is
> available here.

Well, in reviewing this myself, I could not stand up the failure to check the return value of ->Close() near the line. So I modified the code quite a bit from the view point of error checking. I am not sure if you like it or not.

TIA
Attachment #8832548 - Attachment is obsolete: true
Attachment #8834478 - Flags: review?(jorgk)
(Assignee)

Comment 102

3 months ago
Created attachment 8834480 [details] [diff] [review]
(take 3) 1242030-new-part-4-local-less-pop3.patch

"linux" -> "Linux" change, etc.
Attachment #8832549 - Attachment is obsolete: true
Attachment #8832549 - Flags: review?(jorgk)
Attachment #8834480 - Flags: review?(jorgk)
(Assignee)

Comment 103

3 months ago
Created attachment 8834482 [details] [diff] [review]
(take 3) 1242030-new-part-5-pop3.patch

"linux" -> "Linux" change, etc.
Attachment #8832550 - Attachment is obsolete: true
Attachment #8832550 - Flags: review?(jorgk)
Attachment #8834482 - Flags: review?(jorgk)
(Assignee)

Comment 104

3 months ago
imap patch did not have any "linux" mentioned by me.

Comment 105

2 months ago
Comment on attachment 8834473 [details] [diff] [review]
(take 3) 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp

#ifdef DEBUG_bienvenu are sadly leftovers from the quirky old past.

Could you please go through your patch again and make all the debug output the same. In nsPop3Sink.cpp we settled for:
fprintf(stderr,"(seekdebug) ...
so here I'd like to have:
fprintf(stderr,"(debug) ...
You have a mix in your patch:
fprintf(stderr,"(debug) nsMsgDBFolder ...
fprintf(stderr,"{debug} %s:%d Strange ...
fprintf(stderr, "nsMsgDBFolder.cpp:%d: ...
fprintf(stderr,"OpenBackupMsgDatabase(); returns ...
And inherited from D/B:
#ifdef DEBUG
       nsAutoCString message("Offline message too small: messageSize=");	
       nsAutoCString message("Offline message too small: messageSize=");
       message.AppendInt(messageSize);	
Please convert this into a print as well.
Otherwise a neat function to do this sort of thing is nsPrintfCString.

r+ with this addressed.
Attachment #8834473 - Flags: review?(jorgk) → review+
(Assignee)

Comment 106

2 months ago
(In reply to Jorg K (GMT+1) from comment #105)
> Comment on attachment 8834473 [details] [diff] [review]
> (take 3) 1242030-new-part-1.patch: nsIMsgPluggableStore.idl |
> nsMsgDBFolder.cpp
> 
> #ifdef DEBUG_bienvenu are sadly leftovers from the quirky old past.
> 
> Could you please go through your patch again and make all the debug output
> the same. In nsPop3Sink.cpp we settled for:
> fprintf(stderr,"(seekdebug) ...
> so here I'd like to have:
> fprintf(stderr,"(debug) ...
> You have a mix in your patch:
> fprintf(stderr,"(debug) nsMsgDBFolder ...
> fprintf(stderr,"{debug} %s:%d Strange ...
> fprintf(stderr, "nsMsgDBFolder.cpp:%d: ...
> fprintf(stderr,"OpenBackupMsgDatabase(); returns ...
> And inherited from D/B:
> #ifdef DEBUG
>        nsAutoCString message("Offline message too small: messageSize=");	
>        nsAutoCString message("Offline message too small: messageSize=");
>        message.AppendInt(messageSize);	
> Please convert this into a print as well.
> Otherwise a neat function to do this sort of thing is nsPrintfCString.
> 
> r+ with this addressed.

I will convert the printed strings to along the line of
> fprintf(stderr,"(debug) ...

TIA

Comment 107

2 months ago
Comment on attachment 8834478 [details] [diff] [review]
(Take 3) 1242030-new-part-3-import.patch

Review of attachment 8834478 [details] [diff] [review]:
-----------------------------------------------------------------

Please make this consistent. You haven't noticed that OE import has been removed in the meantime?

::: mailnews/import/applemail/src/nsAppleMailImport.cpp
@@ +590,3 @@
>      }
> +
> +    // XXX we may want to report more detailed error according to the values of |rv| or |rv2|

Nit. Uppercase after the XXX and full stop at the end.

@@ +592,5 @@
> +    // XXX we may want to report more detailed error according to the values of |rv| or |rv2|
> +    if (NS_FAILED(rv) || NS_FAILED(rv2)) {
> +      ReportStatus(u"ApplemailImportMailboxConvertError", mailboxName, errorLog);
> +      SetLogs(successLog, errorLog, aSuccessLog, aErrorLog);
> +      return NS_ERROR_FAILURE;

I'm not so happy to change existing behaviour here. If there was an error, we should still go through to the end.

Besides, this is dead code. So this will never be tested.

::: mailnews/import/becky/src/nsBeckyMail.cpp
@@ +535,5 @@
> +      bool closed = false;
> +      rv = msgStore->FinishNewMessage(outputStream, msgHdr, &closed);
> +      // Sanity check
> +      // This condition should never happen as of Jan, 2017.
> +      // See Bug 1242030 comment 80

Lose the comment.

@@ +536,5 @@
> +      rv = msgStore->FinishNewMessage(outputStream, msgHdr, &closed);
> +      // Sanity check
> +      // This condition should never happen as of Jan, 2017.
> +      // See Bug 1242030 comment 80
> +      MOZ_ASSERT(reusable || closed, "!reusable && !closed should not happen.");

Personally, I'd lose the paranoia, too.

@@ +537,5 @@
> +      // Sanity check
> +      // This condition should never happen as of Jan, 2017.
> +      // See Bug 1242030 comment 80
> +      MOZ_ASSERT(reusable || closed, "!reusable && !closed should not happen.");
> +      outputStream = nullptr;   // for code uniformity

This isn't right. The old code was:
if (!reusable) outputStream->Close();
That's pretty much nonsense since if it's not reusable, was already closed in FinishNewMessage().
So as above this should read:
if (closed)
  outputStream = nullptr; // for code uniformity
Or am I missing something?

::: mailnews/import/oexpress/nsOE5File.cpp
@@ +474,4 @@
>  
> +      if (!reusable) {
> +        MOZ_ASSERT(closed, "FinishNewMessage ought to have closed the stream for non-reusable stream");  // See Bug 1242030 comment 80
> +        outputStream = nullptr;

Above you had |if (closed) outputStream = nullptr;|.
And the paranoia looked differently, too:
MOZ_ASSERT(reusable || closed, "!reusable && !closed should not happen.");

::: mailnews/import/oexpress/nsOEMailbox.cpp
@@ +344,5 @@
>      IMPORT_LOG0( "Mbx CopyMbxFileBytes failed\n");
>    }
>    if (!reusable)
>    {
> +    MOZ_ASSERT(closed, "DiscardNewMessage ought to have closed the stream for non-reusable stream");   // See Bug 1242030 comment 80

Same comment as above.

::: mailnews/import/outlook/src/nsOutlookMail.cpp
@@ +435,3 @@
>        }
> +      if (!reusable) {
> +        MOZ_ASSERT(closed, "DiscardNewMessage ought to have closed the stream for non-reusable stream");   // See Bug 1242030 comment 80

Same comment as above.
Attachment #8834478 - Flags: review?(jorgk)
(Assignee)

Comment 108

2 months ago
(In reply to Jorg K (GMT+1) from comment #107)
> Comment on attachment 8834478 [details] [diff] [review]
> (Take 3) 1242030-new-part-3-import.patch
> 
> Review of attachment 8834478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please make this consistent. You haven't noticed that OE import has been
> removed in the meantime?
> 

I will revise the patch.

In the meantime, I did not realize that OE import is gone.
I refreshed the source and only then did I realize the source is gone. The patch won't apply.
Strange, DXR still shows the files. I guess it is the delay between the servers and the original HG repository, but it surely is confusing, and I don't find the log that says anything about oexpress removal (I searched "oexpress" in a non case-sensitive manner.)

Anyway, I will upload the modified patch.
(Assignee)

Comment 109

2 months ago
(In reply to ISHIKAWA, Chiaki from comment #106)
> (In reply to Jorg K (GMT+1) from comment #105)
> > Comment on attachment 8834473 [details] [diff] [review]
> > (take 3) 1242030-new-part-1.patch: nsIMsgPluggableStore.idl |
> > nsMsgDBFolder.cpp
> > 
> > #ifdef DEBUG_bienvenu are sadly leftovers from the quirky old past.
> > 
> > Could you please go through your patch again and make all the debug output
> > the same. In nsPop3Sink.cpp we settled for:
> > fprintf(stderr,"(seekdebug) ...
> > so here I'd like to have:
> > fprintf(stderr,"(debug) ...
> > You have a mix in your patch:
> > fprintf(stderr,"(debug) nsMsgDBFolder ...
> > fprintf(stderr,"{debug} %s:%d Strange ...
> > fprintf(stderr, "nsMsgDBFolder.cpp:%d: ...
> > fprintf(stderr,"OpenBackupMsgDatabase(); returns ...
> > And inherited from D/B:
> > #ifdef DEBUG
> >        nsAutoCString message("Offline message too small: messageSize=");	
> >        nsAutoCString message("Offline message too small: messageSize=");
> >        message.AppendInt(messageSize);	
> > Please convert this into a print as well.
> > Otherwise a neat function to do this sort of thing is nsPrintfCString.
> > 
> > r+ with this addressed.
> 
> I will convert the printed strings to along the line of
> > fprintf(stderr,"(debug) ...
> 
> TIA

I did this for relevant patches in a wholesale manner.

Also, I applied the changed mentioned in the comment below.


(In reply to Jorg K (GMT+1) from comment #107)
> Comment on attachment 8834478 [details] [diff] [review]
> (Take 3) 1242030-new-part-3-import.patch
> 
> Review of attachment 8834478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please make this consistent. You haven't noticed that OE import has been
> removed in the meantime?
> 

OE import has been deprecated and so the patch does not mention the OE files any more.
(Probably the people who used old OE can now first import the old e-mails to the latest MS mailer and then import the messages to TB now. But I don't use MS mailer, and so can't comment on the suitability of such usage.)


> ::: mailnews/import/applemail/src/nsAppleMailImport.cpp
> @@ +590,3 @@
> >      }
> > +
> > +    // XXX we may want to report more detailed error according to the values of |rv| or |rv2|
> 
> Nit. Uppercase after the XXX and full stop at the end.
> 

Done.

> @@ +592,5 @@
> > +    // XXX we may want to report more detailed error according to the values of |rv| or |rv2|
> > +    if (NS_FAILED(rv) || NS_FAILED(rv2)) {
> > +      ReportStatus(u"ApplemailImportMailboxConvertError", mailboxName, errorLog);
> > +      SetLogs(successLog, errorLog, aSuccessLog, aErrorLog);
> > +      return NS_ERROR_FAILURE;
> 
> I'm not so happy to change existing behaviour here. If there was an error,
> we should still go through to the end.

Fixed.
We now go on processing without returning.
 
> Besides, this is dead code. So this will never be tested.
(Is Apple Mail, too? But now at least it compiles.)

> ::: mailnews/import/becky/src/nsBeckyMail.cpp
> @@ +535,5 @@
> > +      bool closed = false;
> > +      rv = msgStore->FinishNewMessage(outputStream, msgHdr, &closed);
> > +      // Sanity check
> > +      // This condition should never happen as of Jan, 2017.
> > +      // See Bug 1242030 comment 80
> 
> Lose the comment.

Done.

> 
> @@ +536,5 @@
> > +      rv = msgStore->FinishNewMessage(outputStream, msgHdr, &closed);
> > +      // Sanity check
> > +      // This condition should never happen as of Jan, 2017.
> > +      // See Bug 1242030 comment 80
> > +      MOZ_ASSERT(reusable || closed, "!reusable && !closed should not happen.");
> 
> Personally, I'd lose the paranoia, too.

Done.
 
> @@ +537,5 @@
> > +      // Sanity check
> > +      // This condition should never happen as of Jan, 2017.
> > +      // See Bug 1242030 comment 80
> > +      MOZ_ASSERT(reusable || closed, "!reusable && !closed should not happen.");
> > +      outputStream = nullptr;   // for code uniformity
> 
> This isn't right. The old code was:
> if (!reusable) outputStream->Close();
> That's pretty much nonsense since if it's not reusable, was already closed
> in FinishNewMessage().
> So as above this should read:
> if (closed)
>   outputStream = nullptr; // for code uniformity
> Or am I missing something?

This part and other parts have been changed somewhat especially the part that was touched
in a hurry about a week ago.
 
> ::: mailnews/import/oexpress/nsOE5File.cpp
> @@ +474,4 @@
> >  
> > +      if (!reusable) {
> > +        MOZ_ASSERT(closed, "FinishNewMessage ought to have closed the stream for non-reusable stream");  // See Bug 1242030 comment 80
> > +        outputStream = nullptr;
> 
> Above you had |if (closed) outputStream = nullptr;|.
> And the paranoia looked differently, too:
> MOZ_ASSERT(reusable || closed, "!reusable && !closed should not happen.");

Fixed/Modified.
 
> ::: mailnews/import/oexpress/nsOEMailbox.cpp
> @@ +344,5 @@
> >      IMPORT_LOG0( "Mbx CopyMbxFileBytes failed\n");
> >    }
> >    if (!reusable)
> >    {
> > +    MOZ_ASSERT(closed, "DiscardNewMessage ought to have closed the stream for non-reusable stream");   // See Bug 1242030 comment 80
> 
> Same comment as above.

Fixed/modified.
 
> ::: mailnews/import/outlook/src/nsOutlookMail.cpp
> @@ +435,3 @@
> >        }
> > +      if (!reusable) {
> > +        MOZ_ASSERT(closed, "DiscardNewMessage ought to have closed the stream for non-reusable stream");   // See Bug 1242030 comment 80
> 
> Same comment as above.

Fixed/Modified.

I am going to post the updated patches now.
(Assignee)

Comment 110

2 months ago
Created attachment 8838451 [details] [diff] [review]
(take 4: 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp r=jorgk

Fixed |fprintf(stderr, "(debug) ..."| style.
Attachment #8834473 - Attachment is obsolete: true
Attachment #8838451 - Flags: review+
(Assignee)

Comment 111

2 months ago
Created attachment 8838457 [details] [diff] [review]
(take 4)  1242030-new-part-3-import-rev04.patch

Fixed some flawed logic/modification according comment 107.
Attachment #8834478 - Attachment is obsolete: true
Attachment #8838457 - Flags: review?(jorgk)
(Assignee)

Comment 112

2 months ago
Created attachment 8838459 [details] [diff] [review]
(take 4) 1242030-new-part-5-pop3.patch

"(debug) " string modification. 
I also added missing " ": |stderr,"| => |stderr, "|
Attachment #8834480 - Attachment is obsolete: true
Attachment #8834480 - Flags: review?(jorgk)
(Assignee)

Comment 113

2 months ago
Created attachment 8838460 [details] [diff] [review]
(take 4)  1242030-new-part-4-local-less-pop3.patch

"(debug) " string modification. 
I also added missing " ": |stderr,"| => |stderr, "| (so now I changed a few existing lines as well.)
Attachment #8834482 - Attachment is obsolete: true
Attachment #8834482 - Flags: review?(jorgk)
Attachment #8838460 - Flags: review?(jorgk)
(Assignee)

Updated

2 months ago
Attachment #8838459 - Flags: review?(jorgk)

Comment 114

2 months ago
Comment on attachment 8838451 [details] [diff] [review]
(take 4: 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp r=jorgk

Review of attachment 8838451 [details] [diff] [review]:
-----------------------------------------------------------------

I had to revisit this, sorry.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +776,5 @@
>    rv = GetMsgInputStream(hdr, &reusable, aFileStream);
> +
> +  if (NS_FAILED(rv)) {
> +#ifdef DEBUG
> +    fprintf(stderr, "(debug) nsMsgDBFolder::GetOfflineFileStream:  GetMsgInputStream(hdr, &reusable, aFileStream); rv=0x%08xn", (unsigned int) rv);

Sadly I have to come back to part 1. After landing of bug 1340755 you need to print this differently, see:
https://hg.mozilla.org/comm-central/rev/541879fbcbe89ae7827850d6807e17ca95013eb2 for examples.

@@ +842,5 @@
> +#ifdef DEBUG
> +            // DEBUG: the error happened while checking network outage condition.
> +            // XXX TODO We may need to check if dovecot and other imap
> +            // servers are returning funny "From " line, etc.
> +            fprintf(stderr, "(debug) %s:%d Strange startOfMsg: <<%s>> \n", __FILE__, __LINE__, startOfMsg);

I just noticed another inconsistent debug print. Either put file/line everywhere or remove it everywhere.

@@ +896,5 @@
> +  // Quite likely a READ error at low-level.
> +  if (NS_FAILED(rv))
> +  {
> +    fprintf(stderr, "(debug) nsMsgDBFolder.cpp:%d: nsMsgDBFolder::GetMsgInputStream: msgStore->GetMsgInputStream(this, ...) returned error rv=%08x\n",
> +            __LINE__, (unsigned int) rv);

fix print here.

@@ +1009,5 @@
> +  // errors with TB running under Linux when network error is encountered.
> +  // 80550006: Mailnews, failure, no = 6
> +  if (NS_FAILED(rv))
> +  {
> +    fprintf(stderr, "(debug) OpenBackupMsgDatabase(); returns error=0x%08x\n", (unsigned int) rv);

and here.

@@ +1709,5 @@
>    m_bytesAddedToLocalMsg = result.Length();
>  
>    nsCOMPtr <nsISeekableStream> seekable;
>  
> +  MOZ_ASSERT(m_tempMessageStream, "Temporary message stream must not be nullptr. (Sanity check).");

While you're here, remove "(Sanity check)".

@@ +1775,5 @@
>    if (seekable)
>    {
>      mDatabase->MarkOffline(messageKey, true, nullptr);
> +
> +    MOZ_ASSERT(m_tempMessageStream, "Temporary message stream must not be nullptr. (Sanity check).");

and here.

@@ +1810,5 @@
> +         rv1 = msgStore->DiscardNewMessage(m_tempMessageStream, m_offlineHeader, &closed);
> +         // Close m_tempMessageStream here if not closed by DiscardNewMessage.
> +         if(!closed && m_tempMessageStream) {
> +           rv2 = m_tempMessageStream->Close();
> +         }

You've changed the behaviour here. Can it happen that msgStore==false? The old code closed the stream in this case.

Also, DiscardNewMessage() only closes the stream for maildir, not mbox. With your new code, the Close() will now always happen, in the old code the stream wasn't closed for mbox (since DiscardNewMessage() didn't close it). Is this change intentional? I guess so, since further down you're exiting with an error.

Perhaps you could add a short comment saying what you want to do and then make the code reflect this.

Like:
if (msgStore) {
  // DiscardNewMessage may not close the stream all the time.
  rv1 = msgStore->DiscardNewMessage(m_tempMessageStream, m_offlineHeader, &closed);
}
// Always close stream since we're inside an error branch.
if (!closed && m_tempMessageStream) {
   rv2 = m_tempMessageStream->Close();
}

@@ +1831,5 @@
>         message.Append(" numOfflineMsgLines=");
>         message.AppendInt(m_numOfflineMsgLines);
>         message.Append(" bytesAdded=");
>         message.AppendInt(m_bytesAddedToLocalMsg);
> +       fprintf(stderr, "(debug) %s\n", message.get());

I think we can lose the complicated assembly of the message like we did in the other patch.
Attachment #8838451 - Flags: review+

Comment 115

2 months ago
Comment on attachment 8838457 [details] [diff] [review]
(take 4)  1242030-new-part-3-import-rev04.patch

Review of attachment 8838457 [details] [diff] [review]:
-----------------------------------------------------------------

I really have a problem here. Apple and Outlook are currently dead, so there is no way you tested this.

If I understand it correctly, you must leave streams open for mbox to write more imported messages. Only for maildir you can close the stream. You've changed that behaviour so I assume Becky import on mbox will not not work any more. r- for that.

::: mailnews/import/applemail/src/nsAppleMailImport.cpp
@@ +529,5 @@
>  
> +    bool closed = false;
> +
> +    // Handle |Close| error in the following.
> +    nsresult rv2;

Can you move this to where it's used?

@@ +535,3 @@
>      while (NS_SUCCEEDED(directoryEnumerator->HasMoreElements(&hasMore)) && hasMore) {
> +      rv = NS_OK;
> +      rv2 = NS_OK;

Is this necessary? I don't think so, see below.

@@ +577,5 @@
>        }
> +      if (outStream) {
> +        if (!closed)
> +          rv2 = outStream->Close();
> +        outStream = nullptr;

Sorry, this seems to be wrong. You close the output stream here in all cases. You should only close it for the !reusable case as the original code did.

@@ -573,3 @@
>      }
> -    if (outStream)
> -      outStream->Close();

You need this close after the while loop in the reusable case.

::: mailnews/import/becky/src/nsBeckyMail.cpp
@@ +538,1 @@
>          outputStream->Close();

You're changing the behaviour here. The original code left left the stream open in the mbox case.

::: mailnews/import/outlook/src/nsOutlookMail.cpp
@@ +439,1 @@
>          outputStream->Close();

Same problem here.

@@ -438,3 @@
>  
> -  if (outputStream)
> -    outputStream->Close();

You need this.
Attachment #8838457 - Flags: review?(jorgk) → review-
(Assignee)

Comment 116

2 months ago
(In reply to Jorg K (GMT+1) from comment #115)
> Comment on attachment 8838457 [details] [diff] [review]
> (take 4)  1242030-new-part-3-import-rev04.patch
> 
> Review of attachment 8838457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really have a problem here. Apple and Outlook are currently dead, so there
> is no way you tested this.
> 

HI,
I will revert my latest mods (done in January) to the files under /import directory to the older one that have been compiled on tryserver from 2015 to 2016 [and presumably they are tested there, no? After reading your comment, now I am not so sure if there are tests for importing e-mails under Windows and OSX.]

I will revert the gratuitous changes I threw in last mongth and only address the comment, debug print statements, and other relevant changes you suggested.

TIA
(Assignee)

Comment 117

2 months ago
Created attachment 8842485 [details] [diff] [review]
(take 5) 1242030-new-part-3-import-rev05.patch


(In reply to ISHIKAWA, Chiaki from comment #116)
> (In reply to Jorg K (GMT+1) from comment #115)
> > Comment on attachment 8838457 [details] [diff] [review]
> > (take 4)  1242030-new-part-3-import-rev04.patch
> > 
> > Review of attachment 8838457 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I really have a problem here. Apple and Outlook are currently dead, so there
> > is no way you tested this.
> > 
> 
> HI,
> I will revert my latest mods (done in January) to the files under /import
> directory to the older one that have been compiled on tryserver from 2015 to
> 2016 [and presumably they are tested there, no? After reading your comment,
> now I am not so sure if there are tests for importing e-mails under Windows
> and OSX.]
> 
> I will revert the gratuitous changes I threw in last mongth and only address
> the comment, debug print statements, and other relevant changes you
> suggested.
> 
> TIA

This is the mods I mentioned earlier.

TIA
Attachment #8842485 - Flags: review?(jorgk)
(Assignee)

Comment 118

2 months ago
Created attachment 8842495 [details] [diff] [review]
(take 5) 1242030-new-part-1.patch: nsIMsgPluggableStore.idl | nsMsgDBFolder.cpp

This is actually more like work-in-progress patch based on your feedback earlier.
But I have to upload this to ask you a couple of quesions.

(In reply to Jorg K (GMT+1) from comment #114)
> Comment on attachment 8838451 [details] [diff] [review]
> (take 4: 1242030-new-part-1.patch: nsIMsgPluggableStore.idl |
> nsMsgDBFolder.cpp r=jorgk
> 
> Review of attachment 8838451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I had to revisit this, sorry.
> 
> ::: mailnews/base/util/nsMsgDBFolder.cpp
> @@ +776,5 @@
> >    rv = GetMsgInputStream(hdr, &reusable, aFileStream);
> > +
> > +  if (NS_FAILED(rv)) {
> > +#ifdef DEBUG
> > +    fprintf(stderr, "(debug) nsMsgDBFolder::GetOfflineFileStream:  GetMsgInputStream(hdr, &reusable, aFileStream); rv=0x%08xn", (unsigned int) rv);
> 
> Sadly I have to come back to part 1. After landing of bug 1340755 you need
> to print this differently, see:
> https://hg.mozilla.org/comm-central/rev/
> 541879fbcbe89ae7827850d6807e17ca95013eb2 for examples.


OK, I changed this patch to go with the cast in the said patch.

*HOWEVER*, I had already applied proper cast when I wanted to print error return values. I verified that they compile and work on tryserver (linux, linux64, OSX, win32, and win64.)

Do I HAVE to modify already working cast a la the mentioned patch?
They don't look neat and add nothing to the sanity of code as far as I can tell.
If the change in the mentioned patch is just to placate the C compiler's format check, my patch already accommodate that albeit slightly differently.

If it is OK, I would rather not bother with this format string issue now that my patch files already *works*.

> @@ +842,5 @@
> > +#ifdef DEBUG
> > +            // DEBUG: the error happened while checking network outage condition.
> > +            // XXX TODO We may need to check if dovecot and other imap
> > +            // servers are returning funny "From " line, etc.
> > +            fprintf(stderr, "(debug) %s:%d Strange startOfMsg: <<%s>> \n", __FILE__, __LINE__, startOfMsg);
> 
> I just noticed another inconsistent debug print. Either put file/line
> everywhere or remove it everywhere.
> 

Actually, it would be great if I can print the function name in addition to
FILE and LINE.  Is __func__ or __function___ an admissible macro?
I think I can create a macro a la something like the following.

#ifdef gcc
#define CODEPOS  _CODEPOS(__FILE__, __LINE__, " in " __func__)
#else // MS VC?
#define CODEPOS  _CODEPOS(__FILE__, __LINE__, " in " __function___)
#else
#define CODEPOS  _CODEPOS(__FILE__, __LINE__, "")

#define _CODEPOS(a, b, c) #a ":" #b c

which I hope will produce something like filepath:1234 in functionname
depending on the compiler.
I will use this STRING instead of __FILE__, __LINE__ separately to 
print the location using "%s" in the format string.

Do you have any preference for the FILE, LINE, FUNCNAME format?
 
Also, my another question is do you want me to change all other patches to change the deug print using fprintf() to look like then fprintf(stderr, "(debug) %s ...", CODEPOS ...
That is file, line, etc. information for ALL the "(debug) ..." messages?

I can certainly do that I suppose although it would take a ulittle time (since I want to make sure that my other patches also will apply after such sweeping change.)


> @@ +896,5 @@
> > +  // Quite likely a READ error at low-level.
> > +  if (NS_FAILED(rv))
> > +  {
> > +    fprintf(stderr, "(debug) nsMsgDBFolder.cpp:%d: nsMsgDBFolder::GetMsgInputStream: msgStore->GetMsgInputStream(this, ...) returned error rv=%08x\n",
> > +            __LINE__, (unsigned int) rv);
> 
> fix print here.
> 

I would handle the filepath, line number issue. (the format string issue is already taken care of.)

> @@ +1009,5 @@
> > +  // errors with TB running under Linux when network error is encountered.
> > +  // 80550006: Mailnews, failure, no = 6
> > +  if (NS_FAILED(rv))
> > +  {
> > +    fprintf(stderr, "(debug) OpenBackupMsgDatabase(); returns error=0x%08x\n", (unsigned int) rv);
> 
> and here.
> 

Ditto.

> @@ +1709,5 @@
> >    m_bytesAddedToLocalMsg = result.Length();
> >  
> >    nsCOMPtr <nsISeekableStream> seekable;
> >  
> > +  MOZ_ASSERT(m_tempMessageStream, "Temporary message stream must not be nullptr. (Sanity check).");
> 
> While you're here, remove "(Sanity check)".

Removed.

> 
> @@ +1775,5 @@
> >    if (seekable)
> >    {
> >      mDatabase->MarkOffline(messageKey, true, nullptr);
> > +
> > +    MOZ_ASSERT(m_tempMessageStream, "Temporary message stream must not be nullptr. (Sanity check).");
> 
> and here.

Done.

> 
> @@ +1810,5 @@
> > +         rv1 = msgStore->DiscardNewMessage(m_tempMessageStream, m_offlineHeader, &closed);
> > +         // Close m_tempMessageStream here if not closed by DiscardNewMessage.
> > +         if(!closed && m_tempMessageStream) {
> > +           rv2 = m_tempMessageStream->Close();
> > +         }
> 
> You've changed the behaviour here. Can it happen that msgStore==false? The
> old code closed the stream in this case.
> 
> Also, DiscardNewMessage() only closes the stream for maildir, not mbox. With
> your new code, the Close() will now always happen, in the old code the
> stream wasn't closed for mbox (since DiscardNewMessage() didn't close it).
> Is this change intentional? I guess so, since further down you're exiting
> with an error.
> 
> Perhaps you could add a short comment saying what you want to do and then
> make the code reflect this.
> 
> Like:
> if (msgStore) {
>   // DiscardNewMessage may not close the stream all the time.
>   rv1 = msgStore->DiscardNewMessage(m_tempMessageStream, m_offlineHeader,
> &closed);
> }
> // Always close stream since we're inside an error branch.
> if (!closed && m_tempMessageStream) {
>    rv2 = m_tempMessageStream->Close();
> }
> 

I added the comment per your suggestion.

> @@ +1831,5 @@
> >         message.Append(" numOfflineMsgLines=");
> >         message.AppendInt(m_numOfflineMsgLines);
> >         message.Append(" bytesAdded=");
> >         message.AppendInt(m_bytesAddedToLocalMsg);
> > +       fprintf(stderr, "(debug) %s\n", message.get());
> 
> I think we can lose the complicated assembly of the message like we did in
> the other patch.

I dumped them into a single fprintf() statement, but again you want me to add
filepath, linenumber, etc. information if I want to add to "(debug) ..." strings uniformly, correct?

TIA
Attachment #8842495 - Flags: review?(jorgk)

Updated

2 months ago
Attachment #8838457 - Attachment is obsolete: true

Updated

2 months ago
Attachment #8838451 - Attachment is obsolete: true

Comment 119

2 months ago
Comment on attachment 8838459 [details] [diff] [review]
(take 4) 1242030-new-part-5-pop3.patch

Wrong patch: The file you've enclosed here it for part 4.
Attachment #8838459 - Flags: review?(jorgk)

Comment 120

2 months ago
Created attachment 8843616 [details] [diff] [review]
1242030-new-part-1.patch (JK v1).

I did a little clean-up run here to avoid any more review cycles. Please provide part 5 (which was missing since you uploaded part 4 twice).
Attachment #8838459 - Attachment is obsolete: true
Attachment #8842495 - Attachment is obsolete: true
Attachment #8842495 - Flags: review?(jorgk)
Attachment #8843616 - Flags: review+

Comment 121

2 months ago
Created attachment 8843622 [details] [diff] [review]
1242030-new-part-3-import.patch (JK v1).

This is how I think it should be. Note that all the code is consistent across the three importers now.
Attachment #8842485 - Attachment is obsolete: true
Attachment #8842485 - Flags: review?(jorgk)
Attachment #8843622 - Flags: review+

Comment 122

2 months ago
Created attachment 8843731 [details] [diff] [review]
1242030-new-part-3-import.patch (v2).

Damn, I got this wrong.
!reusable==true means maildir and closed==true.
 reusable==true means mbox    and closed==false.

So |if (!reusable)| the file is already closed, no need to close it again, all we can do is set the stream pointer to null.

If reusable==true, we don't want to do anything in the loop.

And I already said so in comment #107:
===
if (!reusable) outputStream->Close();
That's pretty much nonsense since if it's not reusable, was already closed in FinishNewMessage().
So as above this should read:
if (closed)
  outputStream = nullptr; // for code uniformity
Or am I missing something?
===
Attachment #8843622 - Attachment is obsolete: true
Attachment #8843731 - Flags: review+

Comment 123

2 months ago
Created attachment 8843733 [details] [diff] [review]
1242030-new-part-3-import.patch (JK v2b).

Oops, wrong indentation.
Attachment #8843731 - Attachment is obsolete: true
Attachment #8843733 - Flags: review+
(Assignee)

Comment 124

2 months ago
Created attachment 8844023 [details] [diff] [review]
1242030-new-part-5-pop3.patch

(In reply to Jorg K (GMT+1) from comment #120)
> Created attachment 8843616 [details] [diff] [review]
> 1242030-new-part-1.patch (JK v1).
> 
> I did a little clean-up run here to avoid any more review cycles. Please
> provide part 5 (which was missing since you uploaded part 4 twice).

Thank you for checking the time while there are other pressing bugs.

Here is part-5.

TIA
Attachment #8844023 - Flags: review?(jorgk)

Comment 125

2 months ago
(In reply to ISHIKAWA, Chiaki from comment #124)
> Thank you for checking the time while there are other pressing bugs.
Sadly there were a few fires lately, and also the preparation for TB 52. Maybe on the weekend.

Comment 126

a month ago
Created attachment 8846441 [details] [diff] [review]
1242030-new-part-3-import.patch (JK v2c).

Make it even more consistent ;-) Also removed needless resetting of local variable which will go out of scope immediately.
Attachment #8843733 - Attachment is obsolete: true
Attachment #8846441 - Flags: review+

Comment 127

a month ago
Created attachment 8846451 [details] [diff] [review]
1242030-new-part-2-imap.patch (JK v1).

I had time to clean up half the IMAP patch. There is a spot were I was unsure. In nsImapMailFolder::CopyMessagesOffline() you can't close the stream, can you? For mbox you need to keep it open, right? See XXX Chiaki.

After you answered this I'll finish off the second part. Take a look at the interdiff to see what I've changed.
Attachment #8832546 - Attachment is obsolete: true
Attachment #8832546 - Flags: review?(jorgk)

Comment 128

a month ago
Sorry, interdiff doesn't work:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8832546&action=interdiff&newid=8846451&headers=1
(Assignee)

Comment 129

a month ago
(In reply to Jorg K (GMT+1) from comment #127)
> Created attachment 8846451 [details] [diff] [review]
> 1242030-new-part-2-imap.patch (JK v1).
> 
> I had time to clean up half the IMAP patch. There is a spot were I was
> unsure. In nsImapMailFolder::CopyMessagesOffline() you can't close the
> stream, can you? For mbox you need to keep it open, right? See XXX Chiaki.
> 
> After you answered this I'll finish off the second part. Take a look at the
> interdiff to see what I've changed.

Thank you very much! (I guess the dust settles down with the broken build on try servers?)

I will look into the issue and report back.

Comment 130

a month ago
(In reply to ISHIKAWA, Chiaki from comment #129)
> I guess the dust settles down with the broken build on try servers?
I don't understand. C-C is building most of the time and I'm working hard to keep it that way. So before you do a try build, take a look at the C-C treeherder. If it's green and the builds are working, *rebase* locally before pushing to try, then the build will work.
(Assignee)

Comment 131

a month ago
(In reply to Jorg K (GMT+1) from comment #130)
> (In reply to ISHIKAWA, Chiaki from comment #129)
> > I guess the dust settles down with the broken build on try servers?
> I don't understand. C-C is building most of the time and I'm working hard to
> keep it that way. So before you do a try build, take a look at the C-C
> treeherder. If it's green and the builds are working, *rebase* locally
> before pushing to try, then the build will work.

Thank you for keeping C-C TB in good shape so that its build succeeds.

I understand that and I was referring to a broken build last weekend when something (a header or something
went haywire) and the tree was closed. I think it is over now, correct?

I will update the patch after checking the question you posed.

TIA

Comment 132

a month ago
I tried to keep tree closures to a minimum, so far I'm doing well. No need to update anything, just answer the question, see XXX Chiaki.
(Assignee)

Comment 133

a month ago
Created attachment 8847162 [details] [diff] [review]
1242030-new-part-2-imap.patch (JK v1 then modified by CI)

Thank you again for your clean up and comment.

Back to your original question in comment 127:

(In reply to Jorg K (GMT+1) from comment #127)
> Created attachment 8846451 [details] [diff] [review]
> 1242030-new-part-2-imap.patch (JK v1).
> 
> I had time to clean up half the IMAP patch. There is a spot were I was
> unsure. In nsImapMailFolder::CopyMessagesOffline() you can't close the
> stream, can you? For mbox you need to keep it open, right? See XXX Chiaki.
> 
> After you answered this I'll finish off the second part. Take a look at the
> interdiff to see what I've changed.

I think we can "close the stream".
First of all, the original FinishNewMessage() [before my modification] always closed the first argument stream no matter what.
So the stream was closed before in the spot where the issue was raised.

So then, my question would be, was the original code buggy in that it did not re-create the output stream on the next loop?
It DOES re-creates the output stream for every loop, I think, by using
GetOfflineStoreOutputStream().


            if (inputStream && hasMsgOffline && !isLocked)
            {
-- Here -->   rv = GetOfflineStoreOutputStream(newMailHdr,
                                               getter_AddRefs(outputStream));
-- Oh? --->   NS_ENSURE_SUCCESS(rv, rv);

              CopyOfflineMsgBody(srcFolder, newMailHdr, mailHdr, inputStream,
                                 outputStream);
              nsCOMPtr<nsIMsgPluggableStore> offlineStore;
              (void) GetMsgStore(getter_AddRefs(offlineStore));
              if (offlineStore)
              {
                bool closed = false;
                nsresult rv2 = offlineStore->FinishNewMessage(outputStream,
                                                              newMailHdr,
                                                              &closed);
                if (NS_FAILED(rv2))
                  NS_WARNING("msgStore->FinishNewMessage(...) failed");
                if (closed)
                  outputStream = nullptr;
                // XXX Chiaki - If this is mbox we don't want to close the stream, do we??
                // This runs in a loop and we can't close the stream??
              }
            }

So maybe we can change the comment to read
                // We close the stream here and on the next loop, it would be re-created by GetOfflineStoreOutputStream().
or similar.

Then it dawned on me: 
Oh well, does this mean we now leak OUTPUT STREAM when outputStream is not null upon the next loop execution?
It seems so.
Before my change to FinishNewMessage(), the stream was closed always.

So we should always close the stream (we need to avoid the double close).
I have now taken care of this issue in my local patch and am uploading a patch since I verified it compiles locally.

*HOWEVER*, as I look at the code I have an uneasy feeling.
This has nothing to do with MY patch.

Near the line 7270, we have a comment that reads
      // N.B. We must not return out of the for loop - we need the matching 
      // end notifications to be sent.

I really hate to see such a very big open-coded loop.
The loop does not fit in a proverbial screenful area (even on my 4K display!).
I found after reading the loop carefully that the current code seems to
obey this "not return out of the for loop" thingy EXCEPT for the NS_ENSURE_SUCCESS() I marked with "-- Oh?--" above.

Maybe, we may have to open code the NS_ENSURE_SUCCESS() to become something like

if (NS_FAILED(rv)) {
   // print some error stuff or something.
   continue;
}

But at this stage I am not sure what we need to do in the failure case at all.
So we can probably simply insert

    // XXX TODO  WARNING. We are breaking out of the loop despite earlier comment.
    // We may want to print error/warning message and continue the loop.

or similarand let someone else to figure this out later?

This is what I inserted in the patch which I am uploading now.

TIA
Attachment #8847162 - Flags: review?(jorgk)

Comment 134

a month ago
(In reply to ISHIKAWA, Chiaki from comment #133)
> I think we can "close the stream".
> First of all, the original FinishNewMessage() [before my modification]
> always closed the first argument stream no matter what.
> So the stream was closed before in the spot where the issue was raised.
I think you confused yourself here but you won't confuse me.

Please check nsMsgBrkMBoxStore::FinishNewMessage() as currently implemented and after your change in part 4. The stream passes in is *NOT* closed. That's the whole idea of mbox.

nsMsgDBFolder::GetOfflineStoreOutputStream() calls nsMsgBrkMBoxStore::GetNewMsgOutputStream() for mbox and that function retrieves the stream from a hash table of open streams with |if (m_outputStreams.Get(URI, aResult))|.

Note the comment here:
https://dxr.mozilla.org/comm-central/rev/d03f6d1f69374c85d4939d0c7cb92021614600f4/mailnews/local/src/nsMsgBrkMBoxStore.h#39

So I believe the answer is: You shouldn't close the stream.

As you know, I was never a great friend of changing the IDL to pass out the 'closedflag'. I've just realised that nsMsgBrkMBoxStore::DiscardNewMessage() previously closed the output stream, so I see no reason to change that and to replicate closing the stream into the respective callers.

The next question I have is why FinishNewMessage() needs to change. Typically it is called on a stream obtained from GetNewMsgOutputStream() and that already passes out the reusable flag. In some cases it's obtained from GetOfflineStoreOutputStream() which sadly don't pass out the reusable flag. Since there are only a few calls, I suggest to change that interface instead and pass out the reusable flag there.

Comment 135

a month ago
Created attachment 8847340 [details] [diff] [review]
1242030-part0-pass-out-reusable.patch

Pass out 'reusable' also from GetOfflineStoreOutputStream().

Comment 136

a month ago
Created attachment 8847342 [details] [diff] [review]
1242030-new-part-1.patch (new approach - JK v1).

Let's see where the journey will take us when we make the reusable flag available at all call sites of DiscardNewMessage() and FinishNewMessage(). This is part 1 adopted to the new scheme.

Comment 137

a month ago
Created attachment 8847355 [details] [diff] [review]
1242030-new-part-3-import.patch (new approach - JK v1).

Part 3 adapted to the new scheme.

Comment 138

a month ago
Created attachment 8847363 [details] [diff] [review]
1242030-new-part-2-imap.patch (new approach - JK v1).

OK, here is part 2. The new approach works well so far.

So, Chiaki, can you please adapt parts 4 and 5 to the new approach while I keep working on the clean-up of the IMAP part in part 2 which is about 50% done.
Attachment #8838460 - Attachment is obsolete: true
Attachment #8843616 - Attachment is obsolete: true
Attachment #8844023 - Attachment is obsolete: true
Attachment #8846441 - Attachment is obsolete: true
Attachment #8846451 - Attachment is obsolete: true
Attachment #8847162 - Attachment is obsolete: true
Attachment #8838460 - Flags: review?(jorgk)
Attachment #8844023 - Flags: review?(jorgk)
Attachment #8847162 - Flags: review?(jorgk)
(Assignee)

Comment 139

a month ago
Sorry, I have been tied up with some family business and a big event at the office.
I will do the cleanup this week.

TIA

Comment 140

28 days ago
Created attachment 8852092 [details] [diff] [review]
1242030-new-part-2-imap.patch (new approach - JK v2).

OK, I finished the IMAP part. Let's keep in mind that we're improving file/stream I/O here. So I removed a lot of changes from nsImapProtocol.cpp which were interfering with the IMAP protocol.

In particular nsImapMockChannel::Close() does not fail and also nsSocketTransport::Close() does not fail.

The patch is much smaller now.

So I'm ready to move onto part 4 and 5.
Attachment #8847363 - Attachment is obsolete: true
(Assignee)

Comment 141

26 days ago
(In reply to Jorg K (GMT+1) from comment #140)
> Created attachment 8852092 [details] [diff] [review]
> 1242030-new-part-2-imap.patch (new approach - JK v2).
> 
> OK, I finished the IMAP part. Let's keep in mind that we're improving
> file/stream I/O here. So I removed a lot of changes from nsImapProtocol.cpp
> which were interfering with the IMAP protocol.
> 
> In particular nsImapMockChannel::Close() does not fail and also
> nsSocketTransport::Close() does not fail.
> 
> The patch is much smaller now.
> 
> So I'm ready to move onto part 4 and 5.

Sorry, I am in the middle of hardware transition (due to bad memory sticks, I think, but not sure).
It has not been a smooth sailing. [The first attempt to move linux image somehow failed...]
I hope I can get this HW issue sorted out this weekend. (That March 31st is the end of budget/fiscal year for many organizations does not help ...)
Once the hardware is in good condition, I can work on the patches.
Sorry that it took so long for me to get up to speed.

TIA

Updated

19 days ago
Blocks: 1354011
You need to log in before you can comment on or make changes to this bug.