Closed Bug 1134527 Opened 5 years ago Closed 3 years ago

C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (Step-2)

Categories

(MailNews Core :: Networking: POP, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1242030

People

(Reporter: ishikawa, Assigned: ishikawa)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1122698 +++

This is a branch of meta Bug 1121842 which depends on this
entry.

This is step-2:
--- begin quote from bug 1121842

After the removal of unnecessary calls of |Close|,
we go to Step-2.

Step-2. Add error value checking of Close() and Flush();

   First step. Simply add NS_ERROR(). Better than nothing.

   At least, we will see the error printed during testing of
   DEBUG version of TB.
  (For better error handling, we will wait for Step 4.)
   
The patch for this will be posted as a separate bugzilla entry.

After Close() and Flush() are taken care of, we check
the error return of Write() and the mismatch of
the # of written bytes and requested bytes.
--- end quote

This is the work-in-progress patch for step-2.


Below is the initial comment from bug 1122698.
This is the step-1
(from bug 1121842)   <=== typo fixed when cloned.
--- quote ---
Step 1. Remove Extra ->Close() calls (and Flush() before Close().).

Let us remove extra / unnecessary / bogus |Close| calls.

The removal is based on the two proposals below.

PROPOSAL-1: DiscardNewMessage should not close the file stream passed
as the 1st argument. It is caller's responsibility to close it..

PROPOSAL-2: FinishNewMessage should not close the
file stream passed as the first argument.
It is caller's responsibility to close it.

The reasoning is given in a crude write-up I created from checking the
code at the end. (This will be posted as the next comment.)

I checked the usage of two functions above, and with a few patches,
the proposals ought to work.

This patch will be filed as a different bugzilla on which this meta
bugzilla entry depends.

--- end quote

Actually, not many |Close| are removed statically, but
their calls during execution will be decreased by 
- setting stream value to nullptr after |Close|, and
- the checking of stream value before calling |Close|.

(Well, that is the theory. We may find out more
bogus calls once we check the return value of Close().
But this is a start.)

Local |make mozmill| test passed, and |make xpcshell-tests| did not introduce any new bugs with the attached patch.
I will still mark this as Work-in-Progress in the series so that
people in the know can figure out where any modifications should be made, etc. by looking at the whole picture.
 
(Hmm. I wonder why there is component for Mail Transfer. This is a mail client.)
Assignee: nobody → ishikawa
Blocks: 1134529
This applies to the current tree.
Attachment #8566382 - Attachment is obsolete: true
Too bad this is only for POP3... I think the majority of TB users use IMAP...
(In reply to Charles from comment #2)
> Too bad this is only for POP3... I think the majority of TB users use IMAP...

I didn't know that.
Is there telemetry date on the protocol type?

Checking IMAP operation is on my todo list.
It is just that I have used only POP3 for quite some time, and I was not sure
how to set up an imap server for local testing.

Actually, if you look at the posted patches (I will roll updated patches for the
latest source tree shortly), there are already a few added code for Imap source files for better error catching.

But TESTING of error cases (network outage, etc.) has not been done well for
imap side locally by me.

But something interesting happend lately.

TB is quite user-UNfriendly when one wants to set up a mail account afresh based 
on user's own idea. TB tries to match the user's input address to an internal database to set up the mail account correctly.
BUT if we try to set up an e-mail address with domain part which is 
not in that e-mail server database, the operation is not quite crystal-clear.
User experience goes downhill.
I think there is a bugzilla entry for this obnoxious TB behavior.

By happenstance the other day, to try to add a mail test account locally,
I found that I created an account that uses IMAP.
(I was meaning to create one for POP3!
You can now appreciate how opaque the mail account creation for domain address that is not in the database can be with TB!)

Well, I said to myself then, what about the server. Without imap server, this mail account is USELESS!
Well I forgot about it completely, but my local Debian GNU/Linux has installed DOVECOT as pop3/imap server, and I was using only the pop3 portion of dovecot for my pop3 testing.
So the end result is without my intending to do so, I set up a usable IMAP e-mail account ! :-)
So the testing will continue with IMAP shortly.
(But this was quite unintentional. TB's UI for mail account creation ought to be overhauled.)

Stay tuned.
But don't hold your breath. I think I have to let the next shipping date pass, and ONLY
WHEN I CAN FEEL very confident that my patch for imap portion is in good health after testing, and others concur, maybe we can put the modified patch in the follow-up minor update.

If someone can help imap testing I will appreciate it although "testing" I have in mind
is like ripping out network cable during the mail transfer and see if TB handles such errors, etc., especially when one's profile is stored on remote file system as on a think-client corporate setting (and CIFS seems to generate most error cases from my local testing for pop3 account) 
So the testing is not the ordinary error-free case scenario and one needs a remote file system for good test coverage. 

Thank you for your interest anyway. Sometimes I am not sure how much benefit my patch
generates for others (esp. on Imap side.) although I certainly think pop3 improvement I am doing benefits me :-)
Yeah, I am creating patches only in a demand-driven manner: when I am bitten by some bugs of TB, I try to fix them. Sometimes I am doing preventive maintenance by trying to reduce
error/warning messages from test suite runs.

But I am not in a mood for major new function addition or anything like that.
Just fixing a bug that haunts me is my approach right now.

My office thinks of using an external imap-based mail service to cut down on
administrative cost, so I might as well make sure TB is up to the task of IMAP
even when network behaves sometimes erratically.

TIA
(In reply to ISHIKAWA, Chiaki from comment #3)
> (In reply to Charles from comment #2)
>> Too bad this is only for POP3... I think the majority of TB users use IMAP...

> I didn't know that.
> Is there telemetry date on the protocol type?

No idea, but I think the fact that the default Account Type in TB when creating a new account is IMAP (as you discovered recently) is sufficient to back up the assumption, absent any telemetry showing otherwise...

;)

> Checking IMAP operation is on my todo list.
> It is just that I have used only POP3 for quite some time, and I was not sure
> how to set up an imap server for local testing.

I totally understand, and believe me I meant no disrespect and it wasn't intended as criticism per se, so my apologies if it sounded that way... you are in no way obligated to work on any bugs that don't scratch your own personal itches... :)

I was just expressing my opinion, because I get hit regularly by these performance problems, and only use IMAP.

If I was a programmer, I would be jumping at the chance to start helping you work on these in a more global fashion (that would fix both POP and IMAP bugs)...

In fact, I've been meaning to ask around for what coding skills are needed to work on TB bugs, and start learning - maybe in a year or two, I would be in a position to actually do help with coding...

> So the end result is without my intending to do so, I set up a usable IMAP
> e-mail account ! :-)

Yes, and this new Account Creation stuff was a huge problem when it was introduced (back in version 3 I think). The worst parts of it were fixed soon thereafter, but it is still a bit user unfriendly - I'm just used to it now...

> So the testing will continue with IMAP shortly.
> (But this was quite unintentional. TB's UI for mail account creation ought
> to be overhauled.)
> 
> Stay tuned.
> But don't hold your breath. I think I have to let the next shipping date
> pass, and ONLY
> WHEN I CAN FEEL very confident that my patch for imap portion is in good
> health after testing, and others concur, maybe we can put the modified patch
> in the follow-up minor update.

No worries - code is ready when it is ready. What I *don't* want is swapping new bugs for old ones... ;)

> If someone can help imap testing I will appreciate it although "testing"

I will be happy to assist any way I can, as long as I have an installable build (or patch that will apply to an installed TB), and precise inbstructions on the tests you want done and how to get the results.

> But I am not in a mood for major new function addition or anything like that.
> Just fixing a bug that haunts me is my approach right now.
> 
> My office thinks of using an external imap-based mail service to cut down on
> administrative cost, so I might as well make sure TB is up to the task of
> IMAP even when network behaves sometimes erratically.

Whatever you end up doing, however big or small, will be very much appreciated (at least by me)!
Blocks: 1174500
This is an updated version against today's tree.

This is going to be in a series of patches.

========================================
PLANNED ORDER
========================================

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

   bug 1116055 

   This was considered a necessary step so that we can know that if
   any user experiences a strange seek position change.

* 2 A fix-close-step-1.patch: Bug 1122698 Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends (part 1 of  a series)

   bug 1122698

3 A fix-close-part-2.patch: Bug 1134527 Add error value checking of Close() and Flush() (part-2 of a series)

   bug 1134527

4 A fix-close-part-3-write-error-check.patch: Bug 1134529: Check Write failures (part-3 of a series)

   bug 1134529

5 A removing-a-Seek.patch: C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (step 4)

   bug 1117450

6 A 1116055-acelist.patch: One liner patch posted in Bug 1116055 to return a buffered stream from mbox::getNewOutputStream by aceman

   Bug 1176857 - C-C TB Enabling buffering for pop3 download (and possibly part of imap file operation)

   I have created a separate bug for this since it will be necessary
   to apply this patch only after other patches landed. (This is
   because enabling the buffering requires proper error checking of
   |Close()| and |Flush()|. These are only taken care of AFTER 1-5
   patches above are applied.)

   Maybe about a month after the first patch (bug 1116055) above is
   applied, and other patches 2-5 have been applied, I can enable this
   patch. (Assuming there is NO ONE who reports strange seek position
   change on their computer.)

7 A add-short-read-handling.patch: Bug 1170606 - C-C T-B fixes to take care of short read

   Bug 1170606

   This is theoretically independent of 1-6, but my local patch
   assumes the changes in 1-6 to be in place. (The patch may not apply
   cleanly without 1-6 in advance.)

  There are some additional shor-read issues UNDER M-C portion of the
  source tree.  They can be applied indepdent of this series.


I set the reviewer to Joshua Cranmer, but please feel free to take it or move it to somebody else based on the workload of the potential reviewers.

TIA
Attachment #8589776 - Attachment is obsolete: true
Attachment #8625464 - Flags: review?(Pidgeot18)
There was a typo:

* 5 A removing-a-Seek.patch: C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (step 4)

   bug 1174500 [was mistyped as 1117450 in a few bugzilla entries. :-( ]
Blocks: 1176857
Depends on: 1116055
1134527

Summary of the patch in this bugzilla.

- Added checks to |Close()| and |Flush()| calls where checks were
  missing,

- added error message print using NS_WARN_IF_FALSE() using the value
  of returned |Close()|, |Flush()| and a few other related functions.
  (For better error handling, we will wait for Step 4, but see my
  comment below regarding placing the error message into error console
  using macros proposed in bug 1116055.)

- Some stream pointer variables were not set to nullptr after
  |Close()| in Imap-related files, and so I fixed them by setting them
  to nullptr and not calling |Close()| if the variable is nullptr in the
  first place.

- No null check of |aOutputStream| in |DiscardNewMessage| (in a few
  places).  This is because I found that the pointers have already
  been set to nullptr during error processing (this does not happen in
  the course of normal processing.) This was discovered when I
  simulated network errors during testing.

- the mix-up of |m_outFileStream| and |inboxInputStream| is fixed
  further again at line 997 of nsPop3Sink.cpp.  The mix-up became
  obvious after tracing the usage of values carefully.

Regarding the error message.
NS_ERROR and friends are useful during debugging, but
we NEED the user to KNOW that I/O errors have occurred (between PC and
local disk, between PC and remote file system, etc.) during every day use.

Before the whole patch series was approved and applied, I may want to
modify the patches so that TB outputs the error messages added in my
patch sereis into error console using the macros introduced in bug
1116055.

TIA

PS: I have noticed a few XXX comments I have placed.
    They may need a scrutiny.

  At least the XXX comment at line 945 of nsPop3Sink.cpp
  seems to be correct.
  945:  // XXX would we want to return by NS_ENSURE_SUCCESS above without
  946:  // closing the m_outFileStream here? Or is it closed somewhere?

  It seems that we want to throw away the message since |Flush()|
  failed then...

  FYI, the C-C TB after the whole series of patches passed |make
  mozmill| and |make xpcshell-tests|.
  I have to admit that these tests are very weak in error simulation.

PPS: I have noticed that return values of |Seek()| and |Tell()| are not
checked very well yet.  I will add the check in a planned patch to
pick up these left-over failures to check low-level I/O function
return values (mainly Seek and Tell) after the patch

[7] Bug 1170606 - C-C T-B fixes to take care of short read

so that existing patches do not have to be modified heavily.

PPPS: It might be a good idea to merge the patches of

 - bug 1122698 Cleaning of incorrect Close, unchecked Flush, Write
   etc. in nsPop3Sink.cpp and friends (part 1 of a series)
*- bug 1134527 Add error value checking of Close() and Flush() (part-2
   of a series) [This bugzilla]
 - bug 1134529 Check Write failures (part-3 of a series)
 - bug 1174500 C-C Thunderbird - Cleaning of incorrect Close,
   unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (step 4)
 
into one big patch.
(Yet, still keeping the following patches separate.

- bug 1116055 Performance issue: Failure to use buffered write
  (comm-central thunderbird)

- bug 1176857 - C-C TB Enabling buffering for pop3 download (and
  possibly part of imap file operation)

This may make it easier to check what I/O error is being checked, what
action is taken, etc. in one pass.
Depends on: 1242030
In bug 1242030, I have uploaded merged patches from the bug 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, at the same time, I have split the resulting gigantic patch into a few smaller chunks that address files under only certain directories.

I will upload the latest road map that explains the patches and application order shortly.

TIA
As per comment 8, the patch discussed here is now merged and split per directories in bug 1242030.
So I am duping this bugzilla entry to the new bug.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1242030
Depends on: 1306914
No longer depends on: 1306914
Comment on attachment 8625464 [details] [diff] [review]
fix-close-part-2.patch

No chance that Joshua will review this. The patch most likely has rot anyway.
Attachment #8625464 - Flags: review?(Pidgeot18)
(In reply to Jorg K (GMT+2) from comment #10)
> Comment on attachment 8625464 [details] [diff] [review]
> fix-close-part-2.patch
> 
> No chance that Joshua will review this. The patch most likely has rot anyway.

You are wright. I am working on updating the patch. It now compiles and succeeds with |make mozmill| test
with April source tree. I need to re-upgrade my local source tree to make sure the patch works with the current tree.
You need to log in before you can comment on or make changes to this bug.