+++ 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.)

This applies to the current tree.

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)!

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

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

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.

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.

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.

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