I realized there are places where the return value of wrapper functions of basic low-level I/O are not checked properly in IMAP code. Well there *ARE* many places where this malpractice is seen, but I came to realize a few places after I saw the post of Eric in relevant part is quoted here: === quote begin I second this. I can no more reliably sort my IMAP inbox and store the content on a CIFS mounted filler. When I select messages it takes ages even for simple ascii mail to transfer and usally choques on any serious attachment. Worse the destination folder becomes corrupted, and repairing it may lead to a lot of zero lenght messages or even worse lost emails (no in destination folder and not in imap inbox. === quote end I was debugging somethine else, and realized the lack of checking of the return value of PR_Close() was bad. (Thus I filed Bug 936990 - (META) The return value of PR_Close() should be checked (like any other system calls) I suspected similar failures to check the return values of low-level functions. Here is one of the places, I think the failure to check the return code of low-level I/O functions can result in corrupt file in the face of misbehaving file system (remote mount of CIFS-share, NFS, etc.). I am attaching a diff against comm-central source. I added a few checks for Remove() and Close() nearby, and also removes some extra whitespaces in the same routine. We can only show errors for some of the failures since passing the error status back to upper layer may not be easy as part of retrofitting bug-fixes. I ran |make mozmill| and |make xpcshell-tests| locally. They ran successfully without introducing new bugs. But these passed, but no assurance. These tests failed to simulate error conditions of file systems (file system overflow, etc.) not very well, so the testing code in question are not triggered. Maybe I can learn how to run |make mozmill| or |make xpcshell-test| in low space condition so that |write| or |close| may eventually fail. TIA

Comment on attachment 8333490 [details] [diff] [review] (Work-in-progress) checking return values of Close(), Flush(), Remove(). I am not sure who manages this code, but David Bienvenu touched this part of code for window fix, so maybe he knows? TIA

Comment on attachment 8333490 [details] [diff] [review] (Work-in-progress) checking return values of Close(), Flush(), Remove(). Review of attachment 8333490 [details] [diff] [review]: ----------------------------------------------------------------- David is not responding much these days, so you can also try Neil and Irving.

Comment on attachment 8333490 [details] [diff] [review] (Work-in-progress) checking return values of Close(), Flush(), Remove(). Review of attachment 8333490 [details] [diff] [review]: ----------------------------------------------------------------- I like the approach, but I'd like to hear from someone who understands the offline sync design about what the right approach to error reporting should be. ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +422,3 @@ > while (!inputBuffer && (inputBufferSize >= 512)) > { > inputBuffer = (char *) PR_Malloc(inputBufferSize); Unrelated to your changes, but in reviewing this code I noticed that we never PR_Free(inputBuffer) so there's a memory leak here. Also, we don't need this "keep trying to allocate smaller buffers if PR_Malloc() fails" code. We should either declare the buffer on the stack (and be extremely careful not to overflow it) or just try once to PR_Malloc a fixed size buffer, and fail if we don't get it. @@ +450,5 @@ > bytesLeft -= bytesRead; > } > + // here if read or write failed, rv has the error value from Read/Write. > + rv2 = rv; > + rv = outputStream->Flush(); // Being paranoia does not hurt when it comes to outputStream->Flush() is unnecessary and should be removed; Close() always flushes. @@ +454,5 @@ > + rv = outputStream->Flush(); // Being paranoia does not hurt when it comes to > + rv3 = outputStream->Close(); // system programming. > + if (!NS_SUCCEEDED (rv)) { > + NS_ERROR("ouputStreadm->Flush() falied"); > + } If I hadn't told you to remove the Flush() call, I would tell you to check the error return immediately after the Flush() instead of doing both Flush() and Close() and then checking the error from both. @@ +456,5 @@ > + if (!NS_SUCCEEDED (rv)) { > + NS_ERROR("ouputStreadm->Flush() falied"); > + } > + if (!NS_SUCCEEDED (rv3)) { > + NS_ERROR("ouputStreadm->Close() falied"); Spelling in the error message: "outputStream->Close() failed" @@ +468,5 @@ > m_curTempFile = do_QueryInterface(cloneTmpFile); > nsCOMPtr<nsIMsgCopyService> copyService = do_GetService(NS_MSGCOPYSERVICE_CONTRACTID); > + > + // I can not believe this. There is no way to return the error of CopyFileMessage > + // below to the upper layer?! CopyFileMessage is asynchronous; it reports errors during the copy by calling back to the nsIMsgCopyServiceListener passed as the 7th parameter ('this' on line 480). @@ +480,5 @@ > this, > m_window); > + if (!NS_SUCCEEDED(rv)) { > + MOZ_ASSERT(false, "CopyFileMessage() failed. We are hosed. Better stop right here."); > + } Error returns from CopyFileMessage() here are because the copy service could not set up the copy operation. We should treat these the same as other failures in this code. Unfortunately, I don't understand the design of nsImapOfflineSync well enough to know what the right way to handle failures is. It does not look (to me) like this code does anything useful with failures that happen during replay of offline events.

(In reply to :Irving Reid from comment #3) > ::: mailnews/imap/src/nsImapOfflineSync.cpp > > while (!inputBuffer && (inputBufferSize >= 512)) > > { > > inputBuffer = (char *) PR_Malloc(inputBufferSize); > > Unrelated to your changes, but in reviewing this code I noticed that we > never PR_Free(inputBuffer) so there's a memory leak here. > > Also, we don't need this "keep trying to allocate smaller buffers if > PR_Malloc() fails" code. We should either declare the buffer on the stack > (and be extremely careful not to overflow it) or just try once to PR_Malloc > a fixed size buffer, and fail if we don't get it. There are many occurrences of this pattern in c-c. I try to merge them in bug 558528 so if you don't like it please say so there and here it probably can be left as is as it is unrelated.

Chiaki, can you continue on this patch?

(In reply to :aceman from comment #5) > Chiaki, can you continue on this patch? OK, I will work on this. (I need to fix a few bugzilla entries, some of which are near checking in. So once I finish them off, I will work on this.)

Comment on attachment 8333490 [details] [diff] [review] (Work-in-progress) checking return values of Close(), Flush(), Remove(). (Sorry I don't know this code at all, I hope Irving can handle it!)

I modified the patch slightly. (In reply to :Irving Reid from comment #3) > Comment on attachment 8333490 [details] [diff] [review] > (Work-in-progress) checking return values of Close(), Flush(), Remove(). > > Review of attachment 8333490 [details] [diff] [review]: > ----------------------------------------------------------------- > > I like the approach, but I'd like to hear from someone who understands the > offline sync design about what the right approach to error reporting should > be. This is indeed a problem, but who can answer this question today? At least, I have incorporated the issues raised as part of comments. > ::: mailnews/imap/src/nsImapOfflineSync.cpp > @@ +422,3 @@ > > while (!inputBuffer && (inputBufferSize >= 512)) > > { > > inputBuffer = (char *) PR_Malloc(inputBufferSize); > > Unrelated to your changes, but in reviewing this code I noticed that we > never PR_Free(inputBuffer) so there's a memory leak here. > I did a free when the block, where PR_Malloc() is called, is left. > Also, we don't need this "keep trying to allocate smaller buffers if > PR_Malloc() fails" code. We should either declare the buffer on the stack > (and be extremely careful not to overflow it) or just try once to PR_Malloc > a fixed size buffer, and fail if we don't get it. Per comment from aceman, I did not handle this here, but rather left the comment about the allocation issue. > @@ +450,5 @@ > > bytesLeft -= bytesRead; > > } > > + // here if read or write failed, rv has the error value from Read/Write. > > + rv2 = rv; > > + rv = outputStream->Flush(); // Being paranoia does not hurt when it comes to > > outputStream->Flush() is unnecessary and should be removed; Close() always > flushes. > Took out Flush(). Originally I thought it was odd to see Flush() here, but assumed that the particular sequence was necessary for mozilla-created I/O functions. > @@ +454,5 @@ > > + rv = outputStream->Flush(); // Being paranoia does not hurt when it comes to > > + rv3 = outputStream->Close(); // system programming. > > + if (!NS_SUCCEEDED (rv)) { > > + NS_ERROR("ouputStreadm->Flush() falied"); > > + } > > If I hadn't told you to remove the Flush() call, I would tell you to check > the error return immediately after the Flush() instead of doing both Flush() > and Close() and then checking the error from both. Noted. Flush() is no longer there, so this is a non issue now. > @@ +456,5 @@ > > + if (!NS_SUCCEEDED (rv)) { > > + NS_ERROR("ouputStreadm->Flush() falied"); > > + } > > + if (!NS_SUCCEEDED (rv3)) { > > + NS_ERROR("ouputStreadm->Close() falied"); > > Spelling in the error message: "outputStream->Close() failed" Misspells were fixed (the same error was in another place. Copy&Paste issue.) > @@ +468,5 @@ > > m_curTempFile = do_QueryInterface(cloneTmpFile); > > nsCOMPtr<nsIMsgCopyService> copyService = do_GetService(NS_MSGCOPYSERVICE_CONTRACTID); > > + > > + // I can not believe this. There is no way to return the error of CopyFileMessage > > + // below to the upper layer?! > > CopyFileMessage is asynchronous; it reports errors during the copy by > calling back to the nsIMsgCopyServiceListener passed as the 7th parameter > ('this' on line 480). Thank you for the clarification. This is not evident to people who have started to look at the C-C source tree for the first time to look into issues that they have seen in real-world usage of TB. So I put the comment in the code by quoting the bulk of information. > @@ +480,5 @@ > > this, > > m_window); > > + if (!NS_SUCCEEDED(rv)) { > > + MOZ_ASSERT(false, "CopyFileMessage() failed. We are hosed. Better stop right here."); > > + } > > Error returns from CopyFileMessage() here are because the copy service could > not set up the copy operation. We should treat these the same as other > failures in this code. > > Unfortunately, I don't understand the design of nsImapOfflineSync well > enough to know what the right way to handle failures is. It does not look > (to me) like this code does anything useful with failures that happen during > replay of offline events. Again, I put the comment to explain the situation. My take on this. It is not entirely clear how we can report back the errors encountered during the initial temporary file creation to callers. But at least, it now is reported by NS_ERROR(). TIA

Comment on attachment 8399883 [details] [diff] [review] Suggested patch. (At least it signals errors, and is better than before) Review of attachment 8399883 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, aside from a bit of comment and spelling cleanup. Mark, do you know enough about offline replay to SR this, or do we need someone else? ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +455,5 @@ > break; > bytesLeft -= bytesRead; > } > + // here if read or write failed, rv has the error value from Read/Write. > + rv2 = outputStream->Close(); // system programming. Remove the comment from the end of this line @@ +472,5 @@ > + // CopyFileMessage is asynchronous; it reports > + // errors during the copy by calling back to the > + // nsIMsgCopyServiceListener passed as the 7th > + // parameter ('this') > + // The error of CopyFileMessage is reported asynchronously. The last line of this comment is redundant and can be removed @@ +500,5 @@ > } > + else { > + rv = tmpFile->Remove(false); > + if (!NS_SUCCEEDED(rv)) { > + NS_ERROR("tmpFile->Remove() falied"); s/falied/failed/ @@ +515,5 @@ > } > // want to close in failure case too > + rv = outputStream->Close(); > + if (!NS_SUCCEEDED(rv)) { > + NS_ERROR("outputStreadm->Close() failed"); s/outputStreadm/outputStream/

I have created the patch to reflect the spelling checks and comment change in comment 9. I will wait for Mark's comment and then upload the patch (with additional change if necessary). TIA

Comment on attachment 8399883 [details] [diff] [review] Suggested patch. (At least it signals errors, and is better than before) Review of attachment 8399883 [details] [diff] [review]: ----------------------------------------------------------------- You're right, I'm not quite familiar enough with this code to work out what we should do here. Its unclear how we should be handling the error cases wrt offline operations. I wonder if David is around and might be able to give us some insights. ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +362,5 @@ > else > ProcessNextOperation(); > } > > +// TODO/FIXME: That this function does not return error due to Please make this simply "XXX" as is our general standard. "TODO/FIXME" isn't necessary

This reflects the comments in two previous comments. On re-reading the patch, I added XXX to the comment about pending improvement related to Bug 558528. TIA

(In reply to Mark Banner (:standard8) from comment #11) > Comment on attachment 8399883 [details] [diff] [review] > Suggested patch. (At least it signals errors, and is better than before) > > Review of attachment 8399883 [details] [diff] [review]: > ----------------------------------------------------------------- > > You're right, I'm not quite familiar enough with this code to work out what > we should do here. Its unclear how we should be handling the error cases wrt > offline operations. > > I wonder if David is around and might be able to give us some insights. I've asked David to comment. Then we'll still need SR

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #13) > (In reply to Mark Banner (:standard8) from comment #11) > > Comment on attachment 8399883 [details] [diff] [review] > > Suggested patch. (At least it signals errors, and is better than before) > > > > Review of attachment 8399883 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > You're right, I'm not quite familiar enough with this code to work out what > > we should do here. Its unclear how we should be handling the error cases wrt > > offline operations. > > > > I wonder if David is around and might be able to give us some insights. > > I've asked David to comment. Then we'll still need SR I am about to begin investigating error handling issues in imap code in the next few weeks under transient network error conditions. I just noticed that this bugzilla is still pending, and I am sure that I will be modifying the said file with more checks, etc. What should I do? TIA

Comment on attachment 8404428 [details] [diff] [review] Patches that reflect the comments so far (thx) (carrying over irving's review, and superreview request.) Review of attachment 8404428 [details] [diff] [review]: ----------------------------------------------------------------- Please add bug number to the commit message and put the message on a single line (probably reduce it a bit). Do not say what is wrong, but what the patch does (how it fixes it). One hunk of the patch does not apply today. ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +440,5 @@ > + > + // > + // Make sure we catch both read and write error > + // and pass the error status, and do not forget to > + // check the value of flush() and close() error! I think flush() was already removed. @@ +1038,5 @@ > ProcessCopyOperation(currentOp); > else if (mCurrentPlaybackOpType == nsIMsgOfflineImapOperation::kMsgMoved) > ProcessMoveOperation(currentOp); > +// In the two if branches below, error can occur within ProcessAppendMsgOperation. > +// Can we break out/return the error if that happens? Add XXX.

I am preparing a new patch. Obviously, this patch got wiped out from my local hard disk when I had a serious external hard drive box issues in the summer of 2015 :-( I thought I managed to salvage most important patches, but I could not find a trace of this on my local file systems now. (Some portion of the patches survived in other patches I have, but that means I will need to go through merging hell, so to speak. :-( Anyway, stay tuned.

Here is the patch that addresses your comment. (In reply to :aceman from comment #15) > Comment on attachment 8404428 [details] [diff] [review] > Patches that reflect the comments so far (thx) (carrying over irving's > review, and superreview request.) > > Review of attachment 8404428 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please add bug number to the commit message and put the message on a single > line (probably reduce it a bit). Do not say what is wrong, but what the > patch does (how it fixes it). Add the Bug number and shortened the comment to oneline. > One hunk of the patch does not apply today. I think it is fixed now. > ::: mailnews/imap/src/nsImapOfflineSync.cpp > @@ +440,5 @@ > > + > > + // > > + // Make sure we catch both read and write error > > + // and pass the error status, and do not forget to > > + // check the value of flush() and close() error! > > I think flush() was already removed. Right. Fixed the comment. > @@ +1038,5 @@ > > ProcessCopyOperation(currentOp); > > else if (mCurrentPlaybackOpType == nsIMsgOfflineImapOperation::kMsgMoved) > > ProcessMoveOperation(currentOp); > > +// In the two if branches below, error can occur within ProcessAppendMsgOperation. > > +// Can we break out/return the error if that happens? > > Add XXX. Added XXX. That's it. Since somehow I lost this patch that contains the fix for malloc issue, now the right order of patch application for my buffer enabling series becomes this: bug 1116055 -> bug 1242030 (consolidation of 1122698, 1134527, 1134529, 1174500) -> bug 1242042 -> bug 1176857 -> bug 1242046 -> bug 1242050 -> bug 1170606 This bugzilla entry's patch needs to be squeezed in 1242030-a and 1242030-b patch in bug 1242030: A check_fileptr_change.patch: Bug 1116055: Check the unexpected change of file seek position and ask the user to report it A Nulling-filestream-after-close-under-IMPORT-directory.patch: bug 1242030-a: (consolidation of 1122698, 1134527, 1134529, 1174500) ** A checking-return-value-of-close-nsImapOfflineSync-rev03.patch: Bug 939548 - Checking the return value of low-level I/O system calls to catch file system issues ASAP. A fix-close-MERGED-base-dir.patch: bug 1242030-b (consolidation of 1122698, 1134527, 1134529, 1174500) A fix-close-MERGED-imap-dir.patch: bug 1242030-c (consolidation of 1122698, 1134527, 1134529, 1174500) A fix-close-MERGED-local-dir-Rev02.patch: bug 1242030-d (consolidation of 1122698, 1134527, 1134529, 1174500) A fix-close-MERGED-add-closedflag-IMPORT-directory.patch: bug 1242030-e Add third argument to (Finish|Discard)NewMessage(). A enable-buffering-1st-step.patch: Bug 1242042: Enabling buffering for file stream to write message for C-C TB (Enabling buffering first step.) A enable-buffering-1116055-acelist.patch: Bug 1176857: patch posted by acemen in Bug 1116055 to return a buffered stream from mbox::getNewOutputStream A removing-a-Seek-rev02.patch: Bug 1242046 - Removing unnecessary |Seek| that caused the C-C TB to operate slowly in terms of I/O A buffer-by-16K-by-default.patch: Bug 1242050 - C-C TB: use a default output stream buffer size of 16KB instead of 4KB Not sure how to handle this, but I mention it here for now. TIA

I think I was wrong on the pessimistic side about the order of applying patches. The patch here in this bugzilla entry can be applied BEFORE bug 1242030 and AFTER bug 1116055. We do not squeeze the patch here into two of the patches in bug 1242030. So modified order is bug 1116055 => bug 939548 -> bug 1242030 (consolidation of 1122698, 1134527, 1134529, 1174500) -> bug 1242042 -> bug 1176857 -> bug 1242046 -> bug 1242050 -> bug 1170606

Comment on attachment 8796893 [details] [diff] [review] Patches that reflect the comments so far (thx). Review of attachment 8796893 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by comments. ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +459,5 @@ > } > + > + // here if read or write failed, rv has the error value from Read/Write. > + rv2 = outputStream->Close(); > + if (!NS_SUCCEEDED (rv2)) { NS_FAILED(rv2), here and elsewhere. @@ +495,5 @@ > + // playback needs to be handled as well, and is > + // difficult.) > + > + if (!NS_SUCCEEDED(rv)) { > + MOZ_ASSERT(false, "CopyFileMessage() failed. We are hosed. Better stop right here."); You can just write: MOZ_ASSERT(NS_SUCCEEDED(rv), ...) Also you're aware that MOZ_ASSERT() only triggers in debug builds.

Thank you for the comment. (In reply to Jorg K (GMT+2) from comment #19) > Comment on attachment 8796893 [details] [diff] [review] > Patches that reflect the comments so far (thx). > > Review of attachment 8796893 [details] [diff] [review]: > ----------------------------------------------------------------- > > Drive-by comments. > > ::: mailnews/imap/src/nsImapOfflineSync.cpp > @@ +459,5 @@ > > } > > + > > + // here if read or write failed, rv has the error value from Read/Write. > > + rv2 = outputStream->Close(); > > + if (!NS_SUCCEEDED (rv2)) { > > NS_FAILED(rv2), here and elsewhere. !NS_SUCCEEDED(e) -> NS_FAILED(e) : done. > @@ +495,5 @@ > > + // playback needs to be handled as well, and is > > + // difficult.) > > + > > + if (!NS_SUCCEEDED(rv)) { > > + MOZ_ASSERT(false, "CopyFileMessage() failed. We are hosed. Better stop right here."); > > You can just write: > MOZ_ASSERT(NS_SUCCEEDED(rv), ...) > Also you're aware that MOZ_ASSERT() only triggers in debug builds. I see. I tweaked this to be NS_ASSERTION() in this patch, but in the subsequent patch in I changed it by opencoding it using if (NS_FAILED(rv)) { ... } for testing/debugging purposes. bug 1116055 => bug 939548 (This bugzilla entry) -> bug 1242030 (consolidation of 1122698, 1134527, 1134529, 1174500) -> bug 1242042 -> bug 1176857 -> bug 1242046 -> bug 1242050 -> bug 1170606

To assist in trackng and coordinating the work of moving all these patches forward, https://public.etherpad-mozilla.org/p/chiakis-io-patches elaborates on comment 18.

Comment on attachment 8796970 [details] [diff] [review] (update) Patches that reflect the comments so far (thx). Review of attachment 8796970 [details] [diff] [review]: ----------------------------------------------------------------- Some drive-by comments. This should really be reviewed by Kent since he is the resident IMAP and error handling guru. Improving error handling is a good idea, but this has nothing to do with the performance improvements in the series of other bugs of which this one seems to be a part for no apparent reason. ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +364,5 @@ > } > > +// XXX: That this function does not return error due to > +// low-layer file system issues during the first temporary file > +// creation is a grave mistake, isn't it? Please keep comments factual to explain what and why you are doing something. This is not a conversation. @@ +423,5 @@ > // now, copy the dest folder offline store msg to the temp file > int32_t inputBufferSize = 10240; > char *inputBuffer = nullptr; > > + // Don't forget to Release inputBuffer. We don't need this comment. You're free'ing it below, so that's good. @@ +442,5 @@ > + > + // Make sure we catch both read and write error > + // and pass the error status, and do not forget to > + // check the value of close() error! > + // Remove empty comment line. Can you please shorten this comment. @@ +457,5 @@ > break; > bytesLeft -= bytesRead; > } > + > + // here if read or write failed, rv has the error value from Read/Write. The coding standard says that comments start upper case (and end with a full stop). @@ +474,5 @@ > + > + // CopyFileMessage is asynchronous; it reports > + // errors during the copy by calling back to the > + // nsIMsgCopyServiceListener passed as the 7th > + // parameter ('this') Full stop missing. @@ +490,5 @@ > + // because the copy service could not set up the > + // copy operation. We should treat these the same > + // as other failures in this code. It is not > + // quite clear, though, what the right way to > + // handle such failure is. (The failure during If its not clear, that the patch isn't ready for review ;-) @@ +491,5 @@ > + // copy operation. We should treat these the same > + // as other failures in this code. It is not > + // quite clear, though, what the right way to > + // handle such failure is. (The failure during > + // playback needs to be handled as well, and is playback? @@ +494,5 @@ > + // handle such failure is. (The failure during > + // playback needs to be handled as well, and is > + // difficult.) > + > + NS_ASSERTION(NS_SUCCEEDED(rv), "CopyFileMessage() failed. We are hosed. Better stop right here."); Please no ugly jargon here. @@ +500,3 @@ > } > + else { > + rv = tmpFile->Remove(false); Are you sure you can remove the file if the output stream to it failed to close? @@ +503,5 @@ > + if (NS_FAILED(rv)) { > + NS_ERROR("tmpFile->Remove() failed"); > + } > + } > + // release inputBuffer Upper case. Full stop missing. Looks like you're explaining the obvious, so best remove the comment. @@ +1038,5 @@ > ProcessCopyOperation(currentOp); > else if (mCurrentPlaybackOpType == nsIMsgOfflineImapOperation::kMsgMoved) > ProcessMoveOperation(currentOp); > +// XXX In the two if branches below, error can occur within ProcessAppendMsgOperation. > +// Can we break out/return the error if that happens? Please fix comment style here. In a WIP we can discuss options, but this was presented for review as the final solution.

(In reply to Jorg K (GMT+2) from comment #22) > Comment on attachment 8796970 [details] [diff] [review] > (update) Patches that reflect the comments so far (thx). > > Review of attachment 8796970 [details] [diff] [review]: > ----------------------------------------------------------------- > > Some drive-by comments. This should really be reviewed by Kent since he is > the resident IMAP and error handling guru. > > Improving error handling is a good idea, but this has nothing to do with the > performance improvements in the series of other bugs of which this one seems > to be a part for no apparent reason. Jorg, in the error-free conditions, yes, this has nothing to do with the performance improvement. But as soon as file system errors pop up their ugly heads, we need to take care of them because the performance improvement by buffering now needs to cope with the return error of |Close()|, for example. That said, this particular patch could have been merged into the ones in bug 1242030. However, since the patch in this bugzilla entry has been commented and reviewed, I thought it should be handled independently. The failure to follow up on the review promptly was solely my failure. I lost this patch on my local PC when I had a major hardware malfunction last summer and into the fall pretty much. Also, as noted, this patch has one important fix for the forgotten memory release. I could turn this into memory free only change, and move the I/O error handling issues to, say, bug 1242030. What do you think? Anyway, here is my comment to your comments. > > ::: mailnews/imap/src/nsImapOfflineSync.cpp > @@ +364,5 @@ > > } > > > > +// XXX: That this function does not return error due to > > +// low-layer file system issues during the first temporary file > > +// creation is a grave mistake, isn't it? > > Please keep comments factual to explain what and why you are doing > something. This is not a conversation. > > @@ +423,5 @@ > > // now, copy the dest folder offline store msg to the temp file > > int32_t inputBufferSize = 10240; > > char *inputBuffer = nullptr; > > > > + // Don't forget to Release inputBuffer. > > We don't need this comment. You're free'ing it below, so that's good. This was a reminder from an older version to perform the forgotten memory release (noticed by Irving Reid). I will modify this, but wait for the idea re what to do with this patch in relation to the ones in bug 1242030, shrink this into memory free issue only or what? > > @@ +442,5 @@ > > + > > + // Make sure we catch both read and write error > > + // and pass the error status, and do not forget to > > + // check the value of close() error! > > + // > > Remove empty comment line. Can you please shorten this comment. Will do. > > @@ +457,5 @@ > > break; > > bytesLeft -= bytesRead; > > } > > + > > + // here if read or write failed, rv has the error value from Read/Write. > > The coding standard says that comments start upper case (and end with a full > stop). > Will do. > @@ +474,5 @@ > > + > > + // CopyFileMessage is asynchronous; it reports > > + // errors during the copy by calling back to the > > + // nsIMsgCopyServiceListener passed as the 7th > > + // parameter ('this') > > Full stop missing. Will do. > > @@ +490,5 @@ > > + // because the copy service could not set up the > > + // copy operation. We should treat these the same > > + // as other failures in this code. It is not > > + // quite clear, though, what the right way to > > + // handle such failure is. (The failure during > > If its not clear, that the patch isn't ready for review ;-) I think I have entered a comment from one of the reviewers as the food for thought for the future. Nobody seems to know what the right way to handle it... > > @@ +491,5 @@ > > + // copy operation. We should treat these the same > > + // as other failures in this code. It is not > > + // quite clear, though, what the right way to > > + // handle such failure is. (The failure during > > + // playback needs to be handled as well, and is > > playback? Again, I have entered the essence of comments from the reviewers. "playback" seems to refer to the playback of operations done to imap offline storage or something like that. Again, a very delicate handling and nobody seems to be aware of how to do this right. > > @@ +494,5 @@ > > + // handle such failure is. (The failure during > > + // playback needs to be handled as well, and is > > + // difficult.) > > + > > + NS_ASSERTION(NS_SUCCEEDED(rv), "CopyFileMessage() failed. We are hosed. Better stop right here."); > > Please no ugly jargon here. What about CopyFileMessage() failed. Fatal. TB stops now before causing further corruption of mail store. > > @@ +500,3 @@ > > } > > + else { > > + rv = tmpFile->Remove(false); > > Are you sure you can remove the file if the output stream to it failed to > close? Under linux, and OSX, yes. But you are right, under Windows, this |Remove| will fail if the file is still open. > > @@ +503,5 @@ > > + if (NS_FAILED(rv)) { > > + NS_ERROR("tmpFile->Remove() failed"); > > + } > > + } > > + // release inputBuffer > > Upper case. Full stop missing. Looks like you're explaining the obvious, so > best remove the comment. Will do (remove the comment). Again, in a sense, this release is the only unique operation that is outside the realm of performance improvement and its prerequisite error handling. I could possibly shrink this patch into this memory release patch and move other error handling issues, etc. to bug 1242030. What do people think? > > @@ +1038,5 @@ > > ProcessCopyOperation(currentOp); > > else if (mCurrentPlaybackOpType == nsIMsgOfflineImapOperation::kMsgMoved) > > ProcessMoveOperation(currentOp); > > +// XXX In the two if branches below, error can occur within ProcessAppendMsgOperation. > > +// Can we break out/return the error if that happens? > > Please fix comment style here. In a WIP we can discuss options, but this was > presented for review as the final solution. Jorg, I have to disagree with you here on one point. There are so many loose coding issues in C-C TB and I want to mark them for future improvements when I spot them during my patch creation work. But for the life of me, many of them can be handled by me nowdue to the man-power shortage obviously. I want to point them out so that someone out there may be able to pick them up when they encounter them in the code. If we insist on solving all the issues before a patch goes in (that is solving all the XXX issues that are in the newly created patches, we can't go anywhere with C-C TB code.) I will certainly change the comment style, but the XXX issue pointed out ought to be checked later by someone hopefully (I am afraid that this coding is buggy, but the bug or feature seems to have been around for so many years, I think we can wait for another half a year or so until someone has the time to look at it although frankly I am not holding my breath.) I am for the opinion to turn this bugzilla into memory release issue only and move other changes into one of the patches in bug 1242030. This patch could have gone into the tree earlier if my PC did not crash last year :-( TIA

> many of them can be handled by me nowdue to the man-power shortage obviously. NOT of course. many of them can NOT be handled by me now due to the man-power shortage obviously.

(In reply to ISHIKAWA, Chiaki from comment #23) > > Please no ugly jargon here. > What about CopyFileMessage() failed. Fatal. TB stops now before causing further > corruption of mail store. If it's fatal, just say fatal. Jargon like "hosed" (or "screwed" or f*ed) is inappropriate for comments in professional software (although sadly I can see it's used a few times: https://dxr.mozilla.org/comm-central/search?q=hosed&redirect=false). > > > +// XXX In the two if branches below, error can occur within ProcessAppendMsgOperation. > > > +// Can we break out/return the error if that happens? > > Please fix comment style here. In a WIP we can discuss options, but this was > > presented for review as the final solution. > There are so many loose coding issues in C-C TB and I want to mark them for > future improvements when I spot them during my patch creation work. Sure. Simply mark them with XXX but not as a question/answer dialogue. So for example above put: XXX Consider breaking the loop and returning the error. or XXX Missing error handling here.

(In reply to Jorg K (GMT+2) from comment #25) > (In reply to ISHIKAWA, Chiaki from comment #23) > > > > Please no ugly jargon here. > > What about CopyFileMessage() failed. Fatal. TB stops now before causing further > > corruption of mail store. > If it's fatal, just say fatal. Jargon like "hosed" (or "screwed" or f*ed) is > inappropriate for comments in professional software (although sadly I can > see it's used a few times: > https://dxr.mozilla.org/comm-central/search?q=hosed&redirect=false). > All right. I think I picked up the "bad" words and phrases after reading so many open source files. > > > > +// XXX In the two if branches below, error can occur within ProcessAppendMsgOperation. > > > > +// Can we break out/return the error if that happens? > > > Please fix comment style here. In a WIP we can discuss options, but this was > > > presented for review as the final solution. > > There are so many loose coding issues in C-C TB and I want to mark them for > > future improvements when I spot them during my patch creation work. > Sure. Simply mark them with XXX but not as a question/answer dialogue. > So for example above put: > XXX Consider breaking the loop and returning the error. > or > XXX Missing error handling here. I get your points.

I have split the patch into two parts. One about the very clear-cut failure to release allocated memory buffer. This is the one. And, another that handles the detection of Close() errors, etc. That one will follow. My intention is to get the memory release patch quickly, and then if it is OK to merge the patch for Close() errors, etc. into another patch.

I have addressed the comments of jorg and separated the patch for memory release issue for easy integration. Before proceeding to explain how the comments are addressed. If it is OK, maybe we might want to merge this patch in another patch. This patch is part of my efforts to enable buffering in TB operation (especially related to write during POP3 download, saving of files, etc. Actually it handles IMAP operation, too. And related necessary handling of errors from Close() now that buffering causes to Close() to return errors.) bug 1116055 (landed thanks to jorgk) => bug 939548 - Check return Close() et al nsImapOfflineSync::ProcessAppendMsgOperation Now split into TWO: part-a handles the memory release issue that was noticed during review. part-b handles the I/O error issues, the original patch. [The problem is that the patch in 939548 went missing on my local disk after a hardware failure, and probably better handled in a patch in bug 1242030. But it is OK to merge the patch if the reviewer is OK to merge this in this patch. -> bug 1242030 "consolidated" (imap+local+base no review requested, +2import patches one neil=r+) -> bug 1242042 Enabling buffering file stream to write message (1patch, no review requested yet) -> bug 1176857 Enable buffer pop3DL + imap (1patch no review requested yet) -> bug 1242046 remove Seek=>slow I/O (1 triv patch no review requested yet) -> bug 1242050 output stream buffer 4>16KB (aceman review+landed) -> bug 1170606 deal with short read (8 patches, 3 trivial?, 5 not trivial no reviews requested) <- This can wait.) Anyway, part-b patch can either enter here, or can be merged into a patch in bug 1242042: in the latter case, I can re-create the patch in bug 1242042 to take care of the merge. That said, here is the response to earlier comments from Jorg. This is overlaid to my earlier response to Jorg's comments. >> >> ::: mailnews/imap/src/nsImapOfflineSync.cpp >> @@ +364,5 @@ >> > } >> > >> > +// XXX: That this function does not return error due to >> > +// low-layer file system issues during the first temporary file >> > +// creation is a grave mistake, isn't it? >> >> Please keep comments factual to explain what and why you are doing >> something. This is not a conversation. Modified. >> @@ +423,5 @@ >> > // now, copy the dest folder offline store msg to the temp file >> > int32_t inputBufferSize = 10240; >> > char *inputBuffer = nullptr; >> > >> > + // Don't forget to Release inputBuffer. >> >> We don't need this comment. You're free'ing it below, so that's good. >This was a reminder from an older version to perform the forgotten memory release (noticed by Irving Reid). > >I will modify this, but wait for the idea re what to do with this patch in relation to the ones in bug 1242030, shrink this into memory free issue only or what? > I split the patch into two parts: part-a handles the memory release issue. >> >> @@ +442,5 @@ >> > + >> > + // Make sure we catch both read and write error >> > + // and pass the error status, and do not forget to >> > + // check the value of close() error! >> > + // >> >> Remove empty comment line. Can you please shorten this comment. > >Will do. Done. >> >> @@ +457,5 @@ >> > break; >> > bytesLeft -= bytesRead; >> > } >> > + >> > + // here if read or write failed, rv has the error value from Read/Write. >> >> The coding standard says that comments start upper case (and end with a full >> stop). >> >Will do. Done. >> @@ +474,5 @@ >> > + >> > + // CopyFileMessage is asynchronous; it reports >> > + // errors during the copy by calling back to the >> > + // nsIMsgCopyServiceListener passed as the 7th >> > + // parameter ('this') >> >> Full stop missing. > >Will do. Done. Maybe I should add that the comment is here for the future developer who tries to hack this portion of the code, and was copied from a comment in bug 939548. >> >> @@ +490,5 @@ >> > + // because the copy service could not set up the >> > + // copy operation. We should treat these the same >> > + // as other failures in this code. It is not >> > + // quite clear, though, what the right way to >> > + // handle such failure is. (The failure during >> >> If its not clear, that the patch isn't ready for review ;-) > >I think I have entered a comment from one of the reviewers >as the food for thought for the future. >Nobody seems to know what the right way to handle it... Ditto above. >> >> @@ +491,5 @@ >> > + // copy operation. We should treat these the same >> > + // as other failures in this code. It is not >> > + // quite clear, though, what the right way to >> > + // handle such failure is. (The failure during >> > + // playback needs to be handled as well, and is >> >> playback? > >Again, I have entered the essence of comments from the reviewers. >"playback" seems to refer to the playback of operations done to imap offline storage or something like that. Again, a very delicate handling and nobody seems to be aware of how to do this right. Ditto above. I added the reference to playback from mozilla website. >> >> @@ +494,5 @@ >> > + // handle such failure is. (The failure during >> > + // playback needs to be handled as well, and is >> > + // difficult.) >> > + >> > + NS_ASSERTION(NS_SUCCEEDED(rv), "CopyFileMessage() failed. We are hosed. Better stop right here."); >> >> Please no ugly jargon here. > >What about >CopyFileMessage() failed. Fatal. TB stops now before causing further corruption of mail store. I modified the message. (BTW, this NS_ASSERTION is open coded into NS_FAILED() and NS_WARNING() combination in later patches. I found the error too restrictive during heavy error testing by removing network connection to remotely mounted file system.) >> >> @@ +500,3 @@ >> > } >> > + else { >> > + rv = tmpFile->Remove(false); >> >> Are you sure you can remove the file if the output stream to it failed to >> close? > >Under linux, and OSX, yes. >But you are right, under Windows, this |Remove| will fail if the file is still open. I added the comment to this effect. There is no easy way to cope with the error when the remotely mounted system is totally inaccessible. As a whole, TB code is not robust enough [it does not seem to be designed with such disasters in mind.] >> >> @@ +503,5 @@ >> > + if (NS_FAILED(rv)) { >> > + NS_ERROR("tmpFile->Remove() failed"); >> > + } >> > + } >> > + // release inputBuffer >> >> Upper case. Full stop missing. Looks like you're explaining the obvious, so >> best remove the comment. >Will do (remove the comment). Again, in a sense, this release is the only unique operation that is outside the realm of performance improvement and its prerequisite error handling. >I could possibly shrink this patch into this memory release patch and move other error handling issues, etc. to bug 1242030. >What do people think? Removed the comment, and split the patch into two: part-a hanldes this (more or less) one liner case. >> >> @@ +1038,5 @@ >> > ProcessCopyOperation(currentOp); >> > else if (mCurrentPlaybackOpType == nsIMsgOfflineImapOperation::kMsgMoved) >> > ProcessMoveOperation(currentOp); >> > +// XXX In the two if branches below, error can occur within ProcessAppendMsgOperation. >> > +// Can we break out/return the error if that happens? >> >> Please fix comment style here. In a WIP we can discuss options, but this was >> presented for review as the final solution. > Fixed the comment. TIA

Comment on attachment 8802269 [details] [diff] [review] part-a: memory release patch Review of attachment 8802269 [details] [diff] [review]: ----------------------------------------------------------------- Yes this needs doing, but please do the requested changes. ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +419,5 @@ > char *inputBuffer = nullptr; > > + // XXX: Also see bug 558528 (We may want to allocate a > + // big buffer once for all and fail if allocation > + // fails.) I just did r+ on a patch that does this in bug 558528, so this comment is both not needed, and has been bit-rotted by those changes. @@ +465,5 @@ > else > tmpFile->Remove(false); > + > + if (inputBuffer) > + PR_Free(inputBuffer); This free should be done as soon as is reasonable. This point is later than optimal. Please do this instead right after the while (bytesLeft > 0 && NS_SUCCEEDED(rv)) loop ends.

You can use PR_FREEIF(inputBuffer); https://dxr.mozilla.org/comm-central/rev/01ab78dd98805e150b0311cce2351d5b408f3001/mozilla/nsprpub/pr/include/prmem.h# Or am I missing something?

(In reply to Kent James (:rkent) from comment #29) > Comment on attachment 8802269 [details] [diff] [review] > part-a: memory release patch > > Review of attachment 8802269 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yes this needs doing, but please do the requested changes. Thank you for your comment. > > ::: mailnews/imap/src/nsImapOfflineSync.cpp > @@ +419,5 @@ > > char *inputBuffer = nullptr; > > > > + // XXX: Also see bug 558528 (We may want to allocate a > > + // big buffer once for all and fail if allocation > > + // fails.) > > I just did r+ on a patch that does this in bug 558528, so this comment is > both not needed, and has been bit-rotted by those changes. Right. I recreated the patch and removed this part of the change. > > @@ +465,5 @@ > > else > > tmpFile->Remove(false); > > + > > + if (inputBuffer) > > + PR_Free(inputBuffer); > > This free should be done as soon as is reasonable. This point is later than > optimal. Please do this instead right after the while (bytesLeft > 0 && > NS_SUCCEEDED(rv)) loop ends. Moved the release to the suggested place. (In reply to Jorg K (GMT+2) from comment #30) > You can use PR_FREEIF(inputBuffer); > https://dxr.mozilla.org/comm-central/rev/ > 01ab78dd98805e150b0311cce2351d5b408f3001/mozilla/nsprpub/pr/include/prmem.h# > Or am I missing something? I can certainly use PR_FREEIF() to release it, so I did. TIA

Fixed the bitrot caused by the latest patches. TIA

Comment on attachment 8803495 [details] [diff] [review] part-b: (update take 2) Patches that reflect the comments so far (thx) [less the memory release in part-a] Oops, I corrupted the patch somehow when I tried to update it. I will upload this again.

Updated for bitrot. We could merge this into one of the patches in bug 1242030, that modifies the files in ./imap subdirectory.

Comment on attachment 8803491 [details] [diff] [review] part-a: memory release patch (updated) [landed: comment #37] Review of attachment 8803491 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 8803491 [details] [diff] [review] part-a: memory release patch (updated) [landed: comment #37] https://hg.mozilla.org/comm-central/rev/2294e347fd430898dfb226cf61b1496cd031d475

Comment on attachment 8803502 [details] [diff] [review] part-b: (update for bitrot) Patches that reflect the comments so far (thx) [less the memory release in part-a] Review of attachment 8803502 [details] [diff] [review]: ----------------------------------------------------------------- The intention of this patch is to improve error handling and reporting without a radical refactoring of the design, so let's do the right thing here. That means reporting errors at the appropriate level when there is an existing error handling procedure, but otherwise making sure that errors are at least visible to developers. If we are going to clean this up, let's do it right. ProcessAppendMsgOperation does not have any mechanism for reporting errors, but we do not want to hide them from developers. Existing code like: if (NS_FAILED(rv)) return ; // ### return error code. is not really appropriate unless an error is expected as a part of normal operation (in which case there should at least be a comment on why an error is expected). Generally, it is not worth the effort to do an error check on do_GetService on core C++ services prior to use, as that type of failure is usually a catastrophic programming error, and we should just crash. But we will anyway one or two lines later when the service is used, so just let it crash naturally. So in this particular case of the rdf service, just remove that "if (NS_FAILED(rv)) ..." line. And do not add any new ones! This whole method sort of follows the design of: rv = DoSomething() if (NS_SUCCEEDED(rv) ) { DoMore() ... } which hides the error. I don't particularly like that design, as each call ends up increasing the indent level by one. This method is a poster child for why that is a bad design, as by the time we get to some real work like outputStream->Write, we have increased the indent by nine levels due to error checks! You cannot tell the difference between error checks and real logic flow, and the result is a mess. The style that I have promoted instead uses an idiomatic do {] while (false); with a break to exit the method for an unexpected error. You could rewrite this routine to use that if you prefer. At the very least though, we should change this method so that there are no hidden errors. As an example, we have this code with an unexpected (but now hidden) error: rv = rdf->GetResource(moveDestination, getter_AddRefs(res)); if (NS_SUCCEEDED(rv)) Those should be instead using existing macros: if (!NS_WARN_IF(NS_FAILED(rv)) { That is confusing though because of the double negative, really I would prefer if we introduced a new macro so that we could write instead: if (MSG_SUCCEEDED_OR_WARN(rv)) { With the do () while (false); idiom you would use instead: if (NS_WARN_IF(NS_FAILED(rv)) break; You can choose how much rewriting that you want to do here. My comments on your specific changes will refer to the minimum changes that you need. ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +430,5 @@ > rv = inputBuffer ? NS_OK : NS_ERROR_OUT_OF_MEMORY; > + > + // xxx make sure we catch both read/write error, and > + // do not forget to check the value of close() error, > + // too! This comment is long and implies further work needed, yet the errors are caught, right? Just remove it, if there is a case where the error is not caught, add a comment at that point. @@ +444,1 @@ > NS_ASSERTION(bytesWritten == bytesRead, "wrote out incorrect number of bytes"); Your comment is not really correct, this is not a matter of error handling, but of a false programming assumption (that bytesWritten == bytesRead when the idl seems to imply that is not considered an error). If this is a programming assumption, then an assert seems appropriate. So your comment is not really adding anything. Unless you want to investigate this and determine that byteWritten != bytesRead is an allowable condition, I think you are better off to say nothing at all. @@ +447,5 @@ > + // TODO/FIXME: Good that there is an error check. > + // But there may be a case when we may want to > + // retry (EINTR type of issues) due to transiernt > + // write error to a remote mount or interrupted > + // system call for whatever the reason. As you are probably starting to figure out by now, I am really not enthusiastic about long, chatty comments like this. How about: // TODO: handle transient file errors from e.g. network issues @@ +458,5 @@ > + // Here if read or write failed, rv has the error > + // value from Read/Write. > + nsresult rv2 = outputStream->Close(); > + if (NS_FAILED(rv2)) { > + NS_ERROR("ouputStream->Close() failed"); IIUC Close() errors can be caused by transient network or file issues. NS_ERROR() is a break in debug code, and a noop in release code, and is typically intended for catching programming errors. That does not seem appropriate here. Just leave the rv2 = ..., which you check later. @@ +472,5 @@ > + > + // CopyFileMessage is asynchronous; it reports > + // errors during the copy by calling back to the > + // nsIMsgCopyServiceListener passed as the 7th > + // parameter ('this'). This comment is too long. How about: // CopyFileMessage returns error async to this->OnStopCopy @@ +474,5 @@ > + // errors during the copy by calling back to the > + // nsIMsgCopyServiceListener passed as the 7th > + // parameter ('this'). > + > + if (copyService) { Failure to create the copyService is probably a programming error (which should ASSERT) or an out of memory error (which should MOZ_CRASH). In neither case is a silent skipping of code appropriate. In general, I don't think that error checks of do_GetService are worth the effort. They will crash anyway when used, so crashing slightly earlier is not worth the code. So please remove this if (copyService) @@ +492,5 @@ > + // right way to handle such failure is. (The > + // failure during playback needs to be handled as > + // well, and is difficult.) > + // "playback" refers to the sync operation for Imap. > + // See https://wiki.mozilla.org/MailNews:Better_Faster_IMAP This whole comment is too long, and does not really add any value. Please remove. Whatever needs saying should be in the Assert. @@ +494,5 @@ > + // well, and is difficult.) > + // "playback" refers to the sync operation for Imap. > + // See https://wiki.mozilla.org/MailNews:Better_Faster_IMAP > + > + NS_ASSERTION(NS_SUCCEEDED(rv), "CopyFileMessage() failed. Fatal. TB stops now before causing further corruption of mail store."); This is not really the point "before causing further corruption" of a debug assert (which is a noop in release code). Leave the assertion, but simply say "CopyFileMessage() failed. Error in call setup?". @@ +502,5 @@ > + // xxx This remove will fail under Windows if the output stream > + // fails to close above. > + rv = tmpFile->Remove(false); > + if (NS_FAILED(rv)) { > + NS_ERROR("tmpFile->Remove() failed"); NS_ERROR should be for fatal programming errors, but failure to remove a temp file is not like that. Change to a warning instead. By setting rv, you are leaving an unchecked result in release builds, which is not good, particularly since the design of this code is to generally set rv for errors that are going to result in stopping of further action. That is not appropriate here. I think that the appropriate code would be: else { Unused << NS_WARN_IF(NS_FAILED(tmpFile->Remove(false))); } @@ +514,5 @@ > } > + // We should try to close in failure case too. > + rv = outputStream->Close(); > + if (NS_FAILED(rv)) { > + NS_ERROR("outputStream->Close() failed"); Again, this should not be NS_ERROR but a warning.

(In reply to Kent James (:rkent) from comment #38) > // TODO: handle transient file errors from e.g. network issues How about using XXX which we have been promoting elsewhere?

(In reply to Jorg K (GMT+2) from comment #39) > (In reply to Kent James (:rkent) from comment #38) > > // TODO: handle transient file errors from e.g. network issues > How about using XXX which we have been promoting elsewhere? I notice in my own code I've always used XXX todo Looking around, that is also common. If you have been promoting xxx that is fine with me, but if there is a task that needs doing I would prefer xxx todo You are correct that simple TODO is inadequate as I see that also catches plenty of real code.

Uppercase XXX. So for a "to do" we can use XXX TODO:

(In reply to Kent James (:rkent) from comment #38) > Comment on attachment 8803502 [details] [diff] [review] > part-b: (update for bitrot) Patches that reflect the comments so far (thx) > [less the memory release in part-a] > > Review of attachment 8803502 [details] [diff] [review]: > ----------------------------------------------------------------- > > The intention of this patch is to improve error handling and reporting > without a radical refactoring of the design, so let's do the right thing > here. That means reporting errors at the appropriate level when there is an > existing error handling procedure, but otherwise making sure that errors are > at least visible to developers. If we are going to clean this up, let's do > it right. I will recreate the patch per your suggestion. It just took a long time from the status of closed tree early last weekend and chase some strange NEW local errors to make sure that my patch is not causing them (still not sure, but the new issues are related to UI during |make mozmill|, I am not THAT worried too much. Also, strange thing is that I don't see it on try-comm-central. But it certainly makes my local verification more difficult.) TIA

I was in the middle of creating the patch as your suggestion. Then I it a snag. I wholehearted agree with the style especially: do { ... error detection and break; // to go out the block. ... error detection and break; // to go out the block. ... } while (false) // while (0) There are so many pieces of code in C-C TB that get obscured by rampant use of "if() else" nesting during error handling. In relation to this, I especially dislike the sequence such as rv = funcA(); ... if (NS_SUCCEEDED(rv)) { rv = funcB(); } ... if (NS_SUCCEEDED(rv)) { rv = funcC(); } ... if (NS_SUCCEEDED(rv)) { rv = funcD(); } ... // Can anyone tell if above functions funcB, funcC, funcD have been // invoked when we get here? NO! // We ought to have jumped to the error return using "goto" or // "exit" using "do { ...} while(false)" construct! ... return rv; The above construct can benefit from the style of "do { } while (false)" very much. Before proceeding, I would like to point out, though, there is a coding style that caters the needs for resource allocation and release in the face of error detection. Restrained or disciplined use of "goto" comes into play. function testX() ... allocate resource A ... error detection goto Exit_label_A; ... allocate resource B ... error detection goto Exit_label_B; ... allocate resource C ... error detetion goto Exit_label_C; ... finish the operation. Now cleaning up // note the eelease order is reverse of the allocation. Exit_label_C: free resource C; // and maybe necessary post processing. // FALLTHROUGH Exit_label B: free resource B; // and maybe necessary post processing. // FALLTHROUGH Exit_lable A: free resource A; // and maybe necessary post processing. // FALLTHROUGH return; } A simple do { ... error detection break; ... error detection break; ... error detection break; ... } while (false) // or while (0) is good if the error handling is exactly the same after "break"ing out of the do-loop (that is executed only once). If error handling differs or post-processing differs, we have to bring the tag, so to speak, to perform different error handling to outside the loop, or do the error processing where the error occurs before break. Anyway, I merrily tried to change the function to see if I can use the coding style in the said function. The particular routine in question has error processing that requires different type of error clean-up (in else clause) as well as where some error handling could be merged into "do { ... error detection then break ...} while (false)" style. The original routine was so nested with if () { } else { } and the flow of error handling was not so easy to understnd and so I broke the function into combination of if() goto lable_xxx and necessary labels. And I took out the now seemingly unnecessary "{", "}". It flattend the function structure rather well. THEN A PROBLEM. The compiler refused to play along. Well, sort of. C++ has the feature to automatically handles allocation/release of certain class objects with the notion of the block. When I look at the function, it certainly uses it. Since arbitrary use of "goto" in such programming scheme may cause premature release (from the viewpoint of the programmer), or cause execution without properly doing automatic allocation [it may be difficult to specify the behavior of code in a rigorous manner when such code is allowed, I suppose], etc. GCC version 6 which I use on my local test PC issues stern warning if the goto happens in the middle of where such automatic allocation/release ought to occur. (I think going "out" the block where such allocatin/deallocation occurs should be OK, and yes, gcc6 seems to ALLOW such code if I say, -fpermissive to allow such construct(?). Aha, OK, NOW I figure it out, if I properly define the block that allocates and releases a class object by enclosing the portion of the code by "{", "}", i.e. clearly making into a block, I can go OUT the block without causing a compiler warning to be issued.). Please recall the latest tendency of compilers (GCC, CLANG, etc.) becoming picky about these programming issues, and these warnings do help us catch subtle coding issues. However, this compiler warning about goto causes a problem since I let the compiler to cause "ERROR" if a certain type of "WARNING" occurs. Remember, since September, try-comm-central fails compilation when signed vs unsinged compare is done. It seems, at least with GCC6, this WARNING about goto in areas where allocation/release occurs in an indisciplined manner [without properly marking the area as a block] causes ERRORS by default. I suspect this warnings may one day cause errors on try-comm-central, too. (Or maybe even today.) FYI, I show the warning and errors I obtained when I use goto's in listing 1. I found out that if I properly close the scope where the allocatin and release ought to be done inside a block "{", "}", I can use goto to exit such a block [and releasing the class object automatically] without a warning. That is, this is now an explict going OUTSIDE the properly defined block, I suppose. In listing 1, I show errors from different runs of compilers as I enclosed some portion of the code immediately after goto statements one by one, eventually eliminating all the errors except for the misspelling of a label at the end. Now with the "{", "}" blocks, tthe code compiles and Listing 2 shows how function looks WITHOUT my additional error handling yet. (The code is correct as far as try-comm-central is concerned. See. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b9092dd34cf940254ab94852f038f17fcbc83dc3 The patch used then was https://hg.mozilla.org/try-comm-central/rev/90fa1f91d38d The following comment and similar ones there is incorrect // some local variables does automatic allocation and de-alllcation when // a block is exited, and we can't go out with goto. I did not understand the nature of the warning when I tinkered with the code yet when I wrote the above comment very well. This should read, instead.: // Local class objects are automatically allocated and released, but // we need to define the scope clearly or the compiler won't let us exit with // goto without a stern warning. I wonder if "exit" is allowed without marking the blocks cleanly in this manner(?) Anyway, to play it safe with future compilers, I had to enclose these areas of code where certain class objects are LIVE by "{", and "}". But then, when think about it, for the blocks to demark the boundaries of allocatin/release of class objects, I might as well merge it with the preceding if (! expr) goto ... to make it if (expr) { } as in the original which is a little step backward, BUT still the indentation is much flatter than before. rkent, what do you think of modifying THIS FUNCTION in particular at this patch time? Am I going in the direction you want to see? (Actually, there are a lot more changes coming in the bug 1242030: for files under imap subdirectories in fix-close-MERGED-imap-dir.patch (it has bitrotted due to the latest changes.). cf. This bug (I mean the bugzilla entry we are reading) got missed merging with bug 1242030 because of local hardware failure last year. I failed to recover the final patch of this bugzilla entry and did not realize I had to merge this piece into bug 1242030. I will have ample opportunity to use "do { ...} while (false)" in bug 1242030, and now that I know the possible issues with the compiler of the future, can cope with it one way or the other. I am attaching the current patch also for inspection. But I would like to hear the consensus on the direction of modifying this particular function before proceeding further. TIA

The current WIP state of the function.

Oops, I forgot to mention that Listing 2 does not contain the additional error handling proposed by my original patch. It only shows the possible direction of how function ProcessAppendMsgOperation is restructured. TIA

I am attaching a WIP patch that addressed the suggestions, but WITHOUT the major restructuring of ProcessAppendMsgOperation(). When I attempted the restructuring, I got side-tracked by the compiler issue I mentioned in earlier comments, and not sure how to proceed. The restructuring causes the subsequent merging very difficult and so I want to know people's opinions before moving on after settling on some SOLUTION regarding the restructuring issue of THIS PARTICULAR FUNCTION. TIA

(In reply to ISHIKAWA, Chiaki from comment #47) > Created attachment 8805300 [details] [diff] [review] > (WIP) part-b: Patches that reflect the comments so far (thx) without the > major restrucuting [less the memory release in part-a] > I uploaded the above for completeness's sake to let everyone involved know the status of the patch. But I think we are all better off to wait for some decision about what to do with the function ProcessAppendMsgOperation so that I can produce another self-contained patch that includes all the suggestions in one bunch. TIA

Comment on attachment 8805300 [details] [diff] [review] (WIP) part-b: Patches that reflect the comments so far (thx) without the major restrucuting [less the memory release in part-a] Review of attachment 8805300 [details] [diff] [review]: ----------------------------------------------------------------- I'm not exactly sure of what feedback you are looking forward. The feedback that I am giving is that I generally think that the do {} while false() error block is an improvement here. I did not check the details of how each class of error was handled. ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ +397,5 @@ > return; > > nsCOMPtr <nsIOutputStream> outputStream; > + // XXX TODO In the middle of trying new coding style of error check > + do // begin error-prone block I would not call this "error-prone block", what I would say it: // error handling block, break on error. @@ +406,1 @@ > nsCString moveDestination; A normal style, I think that having no blank line before before the "if (NS_WARN_IF(NS_FAILED(rv)))" and one blank line after "break;" should be the standard. You code is inconsistent in this practice, but it should be consistent. @@ +416,4 @@ > > + nsCOMPtr<nsIMsgFolder> destFolder(do_QueryInterface(res, &rv)); > + > + if (NS_WARN_IF( !(NS_SUCCEEDED(rv) && destFolder))) For example, no space before this line. @@ +433,5 @@ > + // XXX TODO make sure seekStream is non-null during release build. > + > + rv = seekStream->Seek(PR_SEEK_SET, messageOffset); > + > + mozilla::Unused << NS_WARN_IF(NS_FAILED(rv)); This is the same type of construct, do not have a space before this line.

This is the patch with the restructuring of the flow control of ProcessAppendMsgOperation.

(In reply to ISHIKAWA, Chiaki from comment #50) > Created attachment 8805745 [details] [diff] [review] > part-b: Patches that reflect the comments so far (thx) with a major > restructuring of a function [less the memory release in part-a] ( > > This is the patch with the restructuring of the flow control of > ProcessAppendMsgOperation. I don't know if it's intentional, but are many whitespace or other changes causing lines to be included in the patch where the code doesn't actually change. Just two examples 1. at return mCurrentUIDValidity; 2. and the last two lines of // pause auto-sync service nsresult rv; autoSyncMgr->Pause();

Typically we apply the "boy-scout rule" (http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule), that is, we fix trailing spaces and other white-space issues as we encounter them in the vicinity of the changes.

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #51) > (In reply to ISHIKAWA, Chiaki from comment #50) > > Created attachment 8805745 [details] [diff] [review] > > part-b: Patches that reflect the comments so far (thx) with a major > > restructuring of a function [less the memory release in part-a] ( > > > > This is the patch with the restructuring of the flow control of > > ProcessAppendMsgOperation. > > I don't know if it's intentional, but are many whitespace or other changes > causing lines to be included in the patch where the code doesn't actually > change. Just two examples > > 1. at > return mCurrentUIDValidity; > > 2. and the last two lines of > // pause auto-sync service > > nsresult rv; > > autoSyncMgr->Pause(); (In reply to Jorg K (GMT+1) from comment #52) > Typically we apply the "boy-scout rule" > (http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule), > that is, we fix trailing spaces and other white-space issues as we encounter > them in the vicinity of the changes. It is possible that I may have removed trailing blanks in one go using Emacs's delete-trailing-space or something like that because I have introduced them while modifying the patches and it was easier to remove all of them with a single command. Well, somebody has to remove them sooner or later, no? But let me check if there are indentation issues. I mean extra space/indent is not what I intended.

I have no objection to the whitespace change. I just thought I'd point it out before rkent comes to review

jorgk, can you take review of part-b? You already have a lot of input here. And kent mentioned on IRC that his current focus is on reviewing bug 1151366, so his review on this bug will not come soon.

Comment on attachment 8805745 [details] [diff] [review] part-b: Patches that reflect the comments so far (thx) with a major restructuring of a function [less the memory release in part-a] ( I'll see what I can do.

(In reply to Jorg K (GMT+1) from comment #56) > Comment on attachment 8805745 [details] [diff] [review] > part-b: Patches that reflect the comments so far (thx) with a major > restructuring of a function [less the memory release in part-a] ( > > I'll see what I can do. Thank you. I will try to accommodate any comments although I will be tied up in a big conference-like event next week... TIA

Sorry, a review would have taken too many loops. I cleaned it up the way it needs to be. Chiaki, can you please check. It compiles, but I haven't run it.

(In reply to Jorg K (GMT+1) from comment #58) > Created attachment 8807639 [details] [diff] [review] > Part b. > > Sorry, a review would have taken too many loops. I cleaned it up the way it > needs to be. > > Chiaki, can you please check. It compiles, but I haven't run it. Thank you for your cleanup efforts. I will run the patch locally and see it produces any unwanted errors, etc.

Changed variable name and fixed obvious copy/paste error.

Comment on attachment 8807652 [details] [diff] [review] Part b (JK v2). Review of attachment 8807652 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/imap/src/nsImapOfflineSync.cpp @@ -436,5 @@ > - bytesLeft -= bytesRead; > - } > - PR_FREEIF(inputBuffer); > - > - outputStream->Flush(); That Flush() got removed, why?

(In reply to Jorg K (GMT+1) from comment #61) > Comment on attachment 8807652 [details] [diff] [review] > Part b (JK v2). > > Review of attachment 8807652 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mailnews/imap/src/nsImapOfflineSync.cpp > @@ -436,5 @@ > > - bytesLeft -= bytesRead; > > - } > > - PR_FREEIF(inputBuffer); > > - > > - outputStream->Flush(); > > That Flush() got removed, why? There is a "->Close()" later on, and "->Close()" calls "->Flush()" internally. Unless we want to differentiate the errors that may happen with "->Flush()" and errors that may happen with "->Close()", I thought it might be just simple to remove "->Flush()" here.

Also, there was a comment from Irving Reid: in comment 3 (In reply to :Irving Reid (No longer working on Firefox) from comment #3) > Comment on attachment 8333490 [details] [diff] [review] > (Work-in-progress) checking return values of Close(), Flush(), Remove(). > > Review of attachment 8333490 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +450,5 @@ > > bytesLeft -= bytesRead; > > } > > + // here if read or write failed, rv has the error value from Read/Write. > > + rv2 = rv; > > + rv = outputStream->Flush(); // Being paranoia does not hurt when it comes to > > outputStream->Flush() is unnecessary and should be removed; Close() always > flushes. > So that was another good reason why I removed it. BTW, the modified patch does not seem to cause regressions. I mean with the CURRENT C-C TB I somehow get 16 errors during |make mozmill| locally [more timeouts after they disappeared about a week ago :-( ], but your modified patch does not ADD any new errors or discernible warnings. I will re-spin additional patches that are laid over this patch (especially the patch for IMAP directory) and see if the whole patch sets for enabling buffered-write is OK. TIA

I'm fine with removing the Flush(). If you like my patch which is just a refactoring, I guess we have to ask for another review elsewhere.

Comment on attachment 8807652 [details] [diff] [review] Part b (JK v2). Aceman, can you please give this a quick review. The diff looks terrible, so please apply the patch and then look at the resulting code. It's basically a refactoring using a |do { ... } while (false);| to break out of the block in any error condition.

(In reply to Jorg K (GMT+1) from comment #64) > I'm fine with removing the Flush(). If you like my patch which is just a > refactoring, I guess we have to ask for another review elsewhere. (In reply to Jorg K (GMT+1) from comment #60) > Created attachment 8807652 [details] [diff] [review] > Part b (JK v2). > > Changed variable name and fixed obvious copy/paste error. I have taken a closer look at your modification. - An early error return removes one extra indentation. - Use of a flag variable to differentiate the post processing immediately after exiting do { } while (false) block instead of using a goto to differentiate the post-loop processing. Fine. - Also a few shuffling around of statements to accommodate the flow changes. I won't have much issue in the changes. TIA (In reply to Jorg K (GMT+1) from comment #65) > Comment on attachment 8807652 [details] [diff] [review] > Part b (JK v2). > > Aceman, can you please give this a quick review. The diff looks terrible, so > please apply the patch and then look at the resulting code. > > It's basically a refactoring using a |do { ... } while (false);| to break > out of the block in any error condition. Aceman, can you please take a look at Jorg's patch? As he suggests, |diff| command really gets confused by the changes in this file. I have had tough time to re-create the patches over and over in the last dozen months or so. But if you compare the changes in the already modified versions of the source file, the change will be obvious (which is not the case with diff output.) In the meantime, let me re-spin the further patches to handle the bitrot caused by the changes in this patch, and see if there are any issues cropping up after I simulate a few network errors.(This is something not handled in the automatic test of |make mozmill|.) Any issues can be handled with a newer patch down the lane IMHO. TIA

(In reply to ISHIKAWA, Chiaki from comment #66) > I have taken a closer look at your modification. > - An early error return removes one extra indentation. It's all a matter of coding style and clarity. If you can avoid an if-branch with an early return, do it. So instead of if (foobar) { // short code block } else { // long code block } return; do if (foobar) { // short code block return; } // long code block return; Equally, write the condition so that the shorter block comes first so you don't have to go looking for it many lines down in the else case. > - Use of a flag variable to differentiate the post processing immediately > after exiting do { } while (false) block instead of using a goto to > differentiate the post-loop processing. Fine. I'm not 100% proud of this, but I don't see a better option other than 'goto' or another |do {} while (false)|. Let's see what Aceman says.

(In reply to Jorg K (GMT+1) from comment #67) > (In reply to ISHIKAWA, Chiaki from comment #66) > > I have taken a closer look at your modification. > > - An early error return removes one extra indentation. > It's all a matter of coding style and clarity. > If you can avoid an if-branch with an early return, do it. > So instead of > > if (foobar) { > // short code block > } else { > // long code block > } > return; > > do > > if (foobar) { > // short code block > return; > } > > // long code block > return; > > Equally, write the condition so that the shorter block comes first so you > don't have to go looking for it many lines down in the else case. E.g.: if (cond) { ... long lines of processing ... ... } else { short one line processing } ===> if (!cond) { short one line processing; } else { ... long lines of processing ... ... } In the past, on a few occasions, in a different source file and in a different project, I proposed exactly that [since hunting down the else part was such a chore], but the code maintainers somehow did not like the proposals. Well, like you say, it is a matter of coding style and one's habit or preference. Although I am for the change to put the short block first FOR MAINTENANCE REASONS, some code maintainers found the processing of code block that are in the NEGATION (!) of the condition unnatural for the thinking process. Oh well. I guess that is why I did not propose this obvious change. > > > - Use of a flag variable to differentiate the post processing immediately > > after exiting do { } while (false) block instead of using a goto to > > differentiate the post-loop processing. Fine. > I'm not 100% proud of this, but I don't see a better option other than > 'goto' or another |do {} while (false)|. Let's see what Aceman says. Because the different exits from the block code require different post processing, this is inevitable. Either we use a flag variable or use a goto to differentiate the post-processing (or worse, the original if-nesting which we should avoid at all cost.) TIA PS: I thought I would try try-comm-central build, but I think there was a change in M-C portion and C-C portion in the last 12 hours regarding nsISupportsArray thingy, and I think I need to refresh the source code again to make the build succeed on try-comm-central. Tough luck.

(In reply to ISHIKAWA, Chiaki from comment #66) > In the meantime, let me ... see if there are any issues cropping up after > I simulate a few network errors. Aceman tells me that his review is waiting for your results here.

(In reply to Jorg K (GMT+1) from comment #69) > (In reply to ISHIKAWA, Chiaki from comment #66) > > In the meantime, let me ... see if there are any issues cropping up after > > I simulate a few network errors. > Aceman tells me that his review is waiting for your results here. Ouch, sorry for the wait. My hardware changed, I no longer had a USB stick in the router that offers simple NAS capability to simulate remote But it is more the time it takes to simulate a network error and creates the error situation and test the result. It takes probably a couple of hours sitting in front of a PC. [Simulating network outage and then later, "restoring" the network outage is a manual job right now. I did not bother to automate this last year. I think I should have.] Comment 66 was written before I had a large conference last week. I could not do such a test. Given the very busy schedule of preparing a big conference next month I am not sure if I can have a full two hours simply doing nothing but simulating and waiting for the error processing. Hmm...

Let me explain. I run the test of TB within linux: Send many e-mails with known contents (random text) to the local account. The account has a remote storage (accessed via LAN). After the sending and filtering, the content of the messages are checked to see if they have been received intact and not corrupt. (Testing of mail copying / moving was done by simulating network errors manually). Simulating network error: Now the network cable of the linux PC is unplugged and plugged back to simulate a short network outage. This caused OS level timeout processing. Since the remote storage is mounted vis CIFS (aka samba) the linux OS timeout and CIFS driver timeout weigh in here. Also, the remote system back in early 2015, I used Windows 7 and a router with NAS capability. BOTH of these targets had DIFFERENT timeout settings. I don't quite figure out what it is for the router. But Windows 7 had something around 2 minutes. CIFS driver under linux has something around 15-20 seconds. So I had to simulate unplugging and plugging back the cable at about 15-20 seconds (actually slightly longer to make sure some error processing kicks in. Shorter one is ignored by the OS and the driver.), at about 2 minutes (slightly longer again. This is about the 2 min [or was it 1:15?] timeout handling at CIFS level.), and at about 10 minutes (this is TCP/IP timeout). (Actually, the network cable was the virtual network cable of VirtualBox. I disabled/enabled the network interface of VirtualBox to simulate the plugging/unplugging of the network cable. This probably could be automated, but read on.) But unfortunately, there is no mechanism to synchronize the network error timing to the reading/writing of remote storage of the account (this accounts write / copy the mail messages into the remote storage.) This means that the simulated error may never trigger the error processing inside the TB code. TB may get hung until the low-level network error is handled by the low-level error recovery, but once the low-level error recovery masked the error and simply succeeded, then TB proceeds as if nothing happens. Or the network error may happen, but it happens at the timing of between one network message download and another and TB may not enter the complex processing where the new error checks have been inserted. So interesting test may not happen at all. TCP/IP 10 minutes time out surely causes a network error all the time: but it takes 10 minutes before the error manifests, however because of the reason above, I may not get a meaningful test at that point in time (due to the independent nature of the mail send/receive test.) It is a hit and miss situation. Only way to test the error handling is to cause many network errors against many message sending/receiving operation. So I have to repeat the test again and again with simulated network. and monitor if TB console (tty console screen) shows any interesting error behavior, and if not, repeat the mail send/receive test again and simulate the network error again and pray hoping that a meaningful error situation that triggers the error processing in TB code happens. It is not a simple task. I did this maybe over 40 days time span. After the bulk of patch was written, I repeated this maybe for two weeks, each day running a test session, to weed out if my error checks do detect network errors, and I did capture a few rare cases. But I think we need to modify M-C code portion as well in order to make TB more robust against network errors. Simply simulating network outage of 10, 20, 30, 60, 90, 120, 150 and then 660 seconds repeatedly and run mail send test/receive for a couple of hours *MAY* produce interesting results, but analyzing the produced log may not be easy (and I did not have that much disk space inside virtualbox back when I worked on the patch and still not have enough disk space set aside for the voluminous test log that can be produced over the 2-hour span.) Any good idea for saving time would be appreciated. Oh, for that matter, any person interested in can run TB binary on try-comm-central with my patches and repeat the above procedure or similar to check the error recover operation. Seeing the crude error recovery work for the first time was very impressive. TIA

Thanks for the explanation. I hope Aceman gets around to the review soon so we can land this and move on to another bug.

(In reply to Jorg K (GMT+1) from comment #72) > Thanks for the explanation. I hope Aceman gets around to the review soon so > we can land this and move on to another bug. I tried to run at least one test run, but I found out it is not easy. Firstly my DNS setup in the local LAN has changed since last time I ran tests in early Spring. My scripts to send the e-mails gets hung if the cable to the linux image is unplugged (Agah!) and other unexpected things happened. (There are a surprisingly large number of programs that seem to reference hostnames by DNS. And they seem to hung when DNS reference is made via BIND that refers to network...) This was not the case in my previous local setting. So I had to plug the cable before I hit "Get Message" button because even TB seems to hung before trying to start the download itself (!). (This caused a practical problem of unplugging the cable while the download proceeds. I now don't have much time to unplug the cable. The download with my patch becomes surprising fast, and a small number of e-mails (100-200) will be downloaded from a local mailserver before I had the time to unplug the cable. Yes, I am testing the binary with many patches applied all of the buffered-write patches to see if everything goes smoothly with the patch here. Anyway, all I could do was make sure that pop3 error recovery works as expected. Testing error condition of imap account was almost impossible now that the network cable needed to be connected to start the test at all (unless I reconfigure the local DNS setting, but that is not easy. I have a NAS based on FreeNAS and I had to do all the reconfiguration of THAT box to change the LAN's DNS setting :-( With the network connected, TB's imap account seems to start synching the local image as soon as the said account receives new e-mails. Tough luck. I am only showing below what the pop3's error recovery looks like during today's testing. During message download the following "Incorporate message begin:" and "Incorporate message complete." pair is seen for one successful download. Incorporate message begin: uidl string: 000000bb56aa867f [20203] WARNING: (debug): IncorporateBegin: !m_downloadingToTempFile path: file /NREF-COMM-CENTRAL/comm-central/mailnews/local/src/nsPop3Sink.cpp, line 539 (debug) Creating buffered output stream to mboxFile=<</home/ishikawa/test-MAIL-dir/Maildir/localhost/Inbox>> in nsMsgBrkMBoxStore::GetNewMsgOutputStream in nsMsgBrkMBoxStore.cpp; (debug) nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream, &rv);failed with rv=0x80004002 [20203] WARNING: (debug) We are enabling buffering for m_outFileStream in nsPop3Sink::IncorporateBegin in nsPop3Sink.cpp.: file /NREF-COMM-CENTRAL/comm-central/mailnews/local/src/nsPop3Sink.cpp, line 611 620: m_outFileStream->Flush() returned 0x00000000 (seekdebug) Seek was necessary in IncorporateBegin() for folder Inbox. (seekdebug) first_pre_seek_pos = 0x0000000000000000, first_post_seek_pos=0x00000000000a9341 [20203] WARNING: (debug): ApplyForwardAndReplyFilter getting called.: file /NREF-COMM-CENTRAL/comm-central/mailnews/local/src/nsPop3Sink.cpp, line 1258 Incorporate message complete. During the testing, I unplugged the cable. (At least, pop3 account in my setup does not download the message automagically until I manually request it.) On the next repetition, network was unplugged. Incorporate message begin: uidl string: 000000bc56aa867f [20203] WARNING: (debug): IncorporateBegin: !m_downloadingToTempFile path: file /NREF-COMM-CENTRAL/comm-central/mailnews/local/src/nsPop3Sink.cpp, line 539 (debug) Creating buffered output stream to mboxFile=<</home/ishikawa/test-MAIL-dir/Maildir/localhost/Inbox>> in nsMsgBrkMBoxStore::GetNewMsgOutputStream in nsMsgBrkMBoxStore.cpp; (debug) nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream, &rv);failed with rv=0x80004002 [20203] WARNING: (debug) We are enabling buffering for m_outFileStream in nsPop3Sink::IncorporateBegin in nsPop3Sink.cpp.: file /NREF-COMM-CENTRAL/comm-central/mailnews/local/src/nsPop3Sink.cpp, line 611 620: m_outFileStream->Flush() returned 0x00000000 (seekdebug) Seek was necessary in IncorporateBegin() for folder Inbox. (seekdebug) first_pre_seek_pos = 0x0000000000000000, first_post_seek_pos=0x00000000000a993c Here the dump stopped. Network cable was unplugged. dmesg showed: [614544.987026] e1000: eth0 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX [614679.277948] e1000: eth0 NIC Link is Down [614679.278028] e1000 0000:00:03.0 eth0: Reset adapter (System tried restting the adapter, but of course, the cable is unplugged, the network was not up again yet.) Eventually the CIFS timeout kicked in. [614803.818988] CIFS VFS: Server 192.168.0.201 has not responded in 120 seconds. Reconnecting... So I plugged the cable again. [614859.713886] e1000: eth0 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX --- Back to the TB side again: The first two lines are repeated for reader's convenience. There may be a few log lines that may be missing in the standard C-C TB. Eventually we see "Abort mail message delivery." which showed that the crude error recovery of pop3 worked. ( Wait a second, why do we get "Incorporate message complete." before "Abort mail message delivery"? Is there an error path which I missed plugging ?! No! Since the message "Abort mail message delivery." is printed, we do call the error exit path. Has a copy & paste error sneaked in the last 6+ months? No, I checked and this strange "Incorporate message complete:" appears before the "Abort mail message delivery" in a log created in May 2015. I suspect this strange line is due to the different buffering of stdout that prints out these messages vs stderr that prints out other error messages. E.g.,"Incorporate message complete." is printed by the following code in nsPop3Sink.cpp: #ifdef DEBUG printf("Incorporate message complete.\n"); #endif (seekdebug) Seek was necessary in IncorporateBegin() for folder Inbox. (seekdebug) first_pre_seek_pos = 0x0000000000000000, first_post_seek_pos=0x00000000000a993c [20203] WARNING: (debug) Error: ras->Seek() returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 195 [20203] WARNING: (debug) Error: ras->Seek() returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 195 (debug) Seek() failed in nsPop3Sink.cpp: rv = 0x80004005 [20203] WARNING: (debug) Flush returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 186 [20203] WARNING: (debug) Error: ras->Seek() returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 195 (debug) Seek() failed in nsPop3Sink.cpp: rv = 0x80004005 [20203] WARNING: (debug) Flush returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 186 [20203] WARNING: (debug) Error: ras->Seek() returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 195 (debug) Seek() failed in nsPop3Sink.cpp: rv = 0x80004005 [20203] WARNING: (debug) Flush returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 186 [20203] WARNING: (debug) Error: ras->Seek() returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 195 (debug) Seek() failed in nsPop3Sink.cpp: rv = 0x80004005 [20203] WARNING: (debug) Flush returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 186 [20203] WARNING: (debug) Error: ras->Seek() returned error within nsBufferedStream::Seek, so we exit early.: file /NREF-COMM-CENTRAL/comm-central/mozilla/netwerk/base/nsBufferedStreams.cpp, line 195 (debug) Seek() failed in nsPop3Sink.cpp: rv = 0x80004005 [20203] WARNING: (debug): ApplyForwardAndReplyFilter getting called.: file /NREF-COMM-CENTRAL/comm-central/mailnews/local/src/nsPop3Sink.cpp, line 1258 Incorporate message complete. Abort mail message delivery. ^G[20203] ###!!! ASSERTION: uidl not in hash table: 'val', file /NREF-COMM-CENTRAL/comm-central/mailnews/local/src/nsPop3Protocol.cpp, line 3674 I could only do this once today. Doing this 20-40 is not something I can do in the next couple of months :-( TIA

As I explained, I get tied up with some events and changing local LAN's DNS setup and modify scripts accordingly is something I cannot do until the end of the year unfortunately :-( But the test I explained can be simulated by anybody who uses linux: create a local pop3 account account for testing and places the Mail directory in one's profile on a remote server (you can even create a symlink for Mail to a remote file system to use existing test account) and then did what I do: unplugging the network cable. One can use a binary which I created on try-comm-central as I explained above. We are testing the write failure to one's local mail store on a remote file system (and not the network failure with pop3 server, mind you.) I suspect someone with a better handle on the imap setup of TB can test this better than I do. Getting the right LAN setup, one's profile setup to make testing easy, and finding the right timing to cause easy to analyze error takes time for sure ... TIA

Diff -b version of the patch to see only real changes, no whitespace/reindent.

It was in the wrong direction.

Comment on attachment 8812953 [details] [diff] [review] Part b (JK v2): diff -U 8 -b (v2) Review of attachment 8812953 [details] [diff] [review]: ----------------------------------------------------------------- I know nothing about IMAP. But if this is just unrolling the error checking without functional changes, it seems fine to me. I didn't spot any logic errors. I put comments to this simplified patch, but of course make changes to the real original patch. ::: nsImapOfflineSync.cpp.old @@ +370,4 @@ > void > nsImapOfflineSync::ProcessAppendMsgOperation(nsIMsgOfflineImapOperation *currentOp, int32_t opType) > { > nsCOMPtr <nsIMsgDBHdr> mailHdr; Move this declaration 2 lines below to where it is used. @@ +374,5 @@ > nsMsgKey msgKey; > currentOp->GetMessageKey(&msgKey); > nsresult rv = m_currentDB->GetMsgHdrForKey(msgKey, getter_AddRefs(mailHdr)); > + > + if (NS_FAILED(rv) || !mailHdr) No empty line here. In rest of the function there are blocks having code, then error check, then empty line. So do not put empty lines between code and error check. @@ +387,5 @@ > mailHdr->GetMessageOffset(&messageOffset); > mailHdr->GetOfflineMessageSize(&messageSize); > nsCOMPtr<nsIFile> tmpFile; > > + if (NS_WARN_IF(NS_FAILED( Wrong indent here. @@ +395,3 @@ > return; > > + if (NS_WARN_IF(NS_FAILED( Wrong indent here. @@ +399,4 @@ > return; > > nsCOMPtr <nsIOutputStream> outputStream; > + Why empty line? @@ +413,5 @@ > nsCString moveDestination; > currentOp->GetDestinationFolderURI(getter_Copies(moveDestination)); > nsCOMPtr<nsIRDFService> rdf(do_GetService(kRDFServiceCID, &rv)); > nsCOMPtr<nsIRDFResource> res; > + if (NS_WARN_IF(NS_FAILED(rv))) This check above the 'res' declaration. @@ +424,2 @@ > nsCOMPtr<nsIMsgFolder> destFolder(do_QueryInterface(res, &rv)); > + if (NS_WARN_IF( !(NS_SUCCEEDED(rv) && destFolder))) Why the space before '!' ? @@ +461,2 @@ > rv = outputStream->Write(inputBuffer, bytesRead, &bytesWritten); > + if (NS_FAILED(rv)) So we do not want to report these problems with NS_WARN_IF ? @@ +470,5 @@ > + nsresult rv2 = outputStream->Close(); > + if (NS_FAILED(rv2)) { > + NS_WARNING("ouputStream->Close() failed"); > + } > + outputStream = nullptr; // don't try to close it again below. Capital Don't. @@ +476,5 @@ > + // rv: Read/Write, rv2: Close > + if (NS_FAILED(rv) || NS_FAILED(rv2)) { > + // This Remove() will fail under Windows if the output stream > + // fails to close above. > + mozilla::Unused << NS_WARN_IF(NS_FAILED(tmpFile->Remove(false))); What does this do?

Great, thanks, I'll fix the nits. > > + mozilla::Unused << NS_WARN_IF(NS_FAILED(tmpFile->Remove(false))); > What does this do? It ignores the error the Mozilla way ;-) Check DXR to see other uses, oops, this will be the first in Mailnews.

Actually, I need a debug run for this since all those warnings and asserts only trigger in debug mode. I've changed the NS_ASSERTION to MOZ_ASSERT since this is the new style we should use. Debug run here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e048ee9036059c4c8b3a1365333db5d94e868a9c

Here's the patch as landed.

Thank you. As I mentioned earlier somewhere, I won't be able to do a lot until the mid-December event is over :-( [Maybe creating a renewed patch to take care of the landing of modified patch, etc.] TIA

(In reply to Jorg K (GMT+1) from comment #78) > Great, thanks, I'll fix the nits. > > > > + mozilla::Unused << NS_WARN_IF(NS_FAILED(tmpFile->Remove(false))); > > What does this do? > It ignores the error the Mozilla way ;-) Check DXR to see other uses, oops, > this will be the first in Mailnews. Ignores? We print a warning if there is an error. Or not? Is it eaten? Or is it just annotatation that we intentionally do nothing with the return value of NS_WARN_IF(). (In reply to ISHIKAWA, Chiaki from comment #83) > Thank you. > As I mentioned earlier somewhere, I won't be able to do a lot until the > mid-December event is over :-( > [Maybe creating a renewed patch to take care of the landing of modified > patch, etc.] The patch here was finished by Jorg. If you mean the other patches, it is fine if they wait a little as we need to polish TB52 now.

(In reply to :aceman from comment #84) > > > > + mozilla::Unused << NS_WARN_IF(NS_FAILED(tmpFile->Remove(false))); > > > What does this do? > > It ignores the error the Mozilla way ;-) Check DXR to see other uses, oops, > > this will be the first in Mailnews. > Ignores? We print a warning if there is an error. Or not? Is it eaten? Or is > it just annotation that we intentionally do nothing with the return value > of NS_WARN_IF(). Sure, it prints a warning, but the error returned from NS_WARN_IF() is eaten, as you said. Yes, Chiaki-san is referring to bug 1242030.