While investigating message incorporation failure (incorporation of download message into Inbox) when the output stream is buffered, [1] and [2] I came across a variable which is no longer used in nsPop3Sink.cpp. You can see that the variable is not used in any useful way anymore by MXR search. --- begin quote --- Defined as a variable in: mailnews/local/src/nsPop3Sink.cpp (View Hg log or Hg annotations) line 275 -- m_inboxOutputStream->Close(); line 276 -- m_inboxOutputStream = nullptr; line 406 -- m_inboxOutputStream->Close(); line 407 -- m_inboxOutputStream = nullptr; mailnews/local/src/nsPop3Sink.h (View Hg log or Hg annotations) line 66 -- nsCOMPtr <nsIOutputStream> m_inboxOutputStream; // the actual mailbox Referenced in: mailnews/local/src/nsPop3Sink.cpp (View Hg log or Hg annotations) line 273 -- if (m_inboxOutputStream) line 404 -- if (m_inboxOutputStream) --- end quote --- The variable is referenced (used), but never set. The attached patch removes the definition and references to it. I ran |make mozmil| and |make xpcshell-tests| locally and it seems the removal has no ill-effect. *HOWEVER*, the name of the variable suggests it probably is meant to refer to Inbox when file I/O is performed. Now, it rings a bell. I have a hunch that this may indicate a presence of a coding bug in nsPop3Sink.cpp. I am trying to find out the root cause of the failure to incorporate the downloaded message into Inbox when the output stream is buffered. After a mix of downloading, moving by filtering, etc., write to Inbox fails. I found out that the underlying file stream that is used by the code to write to Inbox is closed by another(?) thread or something. Maybe, just maybe, the usage of the variable m_inboxOutputStream was dropped by mistake in favor of using another variable for output stream to Inbox, but m_inboxOutuptStream should have been used intact in nsPop3Sink.cpp? In that way, a stream variable is closed by another (?) thread, but still the download thread can continue using valid m_inboxOututStream. Maybe. Just a wild guess. That the variable was used to |Close| a file stream if it was not nullptr suggests that it was once used one way or the other. I checked the Blame of the lines, http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Sink.cpp#273 273 if (m_inboxOutputStream) 274 { 275 m_inboxOutputStream->Close(); 276 m_inboxOutputStream = nullptr; 277 } and found that the lines were introduced in http://hg.mozilla.org/comm-central/diff/7982f8d71ae6/mailnews/local/src/nsPop3Sink.cpp and this patch had an assignment to the variable: --- begin quote --- 1.12 - nsCOMPtr<nsIOutputStream> inboxOutputStream; 1.13 - rv = MsgGetFileStream(path, getter_AddRefs(inboxOutputStream)); 1.14 + rv = MsgGetFileStream(path, getter_AddRefs(m_inboxOutputStream)); 1.15 NS_ENSURE_SUCCESS(rv, rv); 1.16 - inboxInputStream = do_QueryInterface(inboxOutputStream); 1.17 + inboxInputStream = do_QueryInterface(m_inboxOutputStream); --- end quote --- But it does not show any useful references to m_inboxOutputStream. Hmm... TIA [1] Bug 1116055 Performance issue: Failure to use buffered write (comm-central thunderbird) [2] Bug 1120046 RFC mozilla/netwerk/base/src/nsBufferedStreams.cpp: better error reporting and maybe adding thread-race lock TIA

This patch seems to delete one usage of m_inboxOutputStream. http://hg.mozilla.org/comm-central/rev/097bc36f180d (Dated Dec 2011 and is a few months newer than the above patch.) But then I wonder which patch removes the following line: > 1.14 + rv = MsgGetFileStream(path, getter_AddRefs(m_inboxOutputStream));

Comment on attachment 8547117 [details] [diff] [review] Remove the references to m_inboxOutputStream, but see the comment. Requesting review from rkent who added the code here.

(In reply to ISHIKAWA, Chiaki from comment #0) > Created attachment 8547117 [details] [diff] [review] > Remove the references to m_inboxOutputStream, but see the comment. ... > I checked the Blame of the lines, > > http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Sink. > cpp#273 > 273 if (m_inboxOutputStream) > 274 { > 275 m_inboxOutputStream->Close(); > 276 m_inboxOutputStream = nullptr; > 277 } > > and found that the lines were introduced in > http://hg.mozilla.org/comm-central/diff/7982f8d71ae6/mailnews/local/src/ > nsPop3Sink.cpp > > and this patch had an assignment to the variable: > --- begin quote --- > > 1.12 - nsCOMPtr<nsIOutputStream> inboxOutputStream; > 1.13 - rv = MsgGetFileStream(path, getter_AddRefs(inboxOutputStream)); > 1.14 + rv = MsgGetFileStream(path, > getter_AddRefs(m_inboxOutputStream)); > 1.15 NS_ENSURE_SUCCESS(rv, rv); > 1.16 - inboxInputStream = do_QueryInterface(inboxOutputStream); > 1.17 + inboxInputStream = do_QueryInterface(m_inboxOutputStream); > > --- end quote --- > > But it does not show any useful references to m_inboxOutputStream. > Hmm... The original patch had "useful references" to the variable, so I am not sure what you are implying here. Your patch looks incomplete. There are references to m_inboxOutputStream around line 406 that are not removed by the patch. Can you check that? I doubt it will compile in its current form unless I am missing something. The setup of these streams in nsPop3Protocol was mostly moved to the pluggable stores in the megapatch for pluggable stores, and apparently that patch missed the removal of this variable. So in principle that concept of the patch is fine, but let's make sure it is complete.

Hi, sorry I picked up a wrong/incorrect patch. I did not refresh the hg queue before copy and posting. Attached is the latest one. It compiles. The variable was once used by holding a reference to a folder (possibly to Inbox). But somehow it is no longer used. I was wondering if the current set of stream variables used in nsPop3Sink.cpp may need an extra stream variable, which is used to read/write Inbox *all* the time, never closed, etc. Such a variable seems to solve the strange premature Close of stream I observed in bug 1116055. From https://bugzilla.mozilla.org/show_bug.cgi?id=1116055#c20 --- begin quote--- In light of Bug 1120136 in nsPop3Sink.cpp, I hope by having a couple of stream variables (one of them is a reference to Inbox and will be open all the time), I can enable the buffering in nsPop3Sink.cpp (hopefully without locking, but we may still need locking even then.): actually, I think once the move to a message per file storage format is taken, then this problem in nsPop3Sink.cpp will be a non issue. --- end quote TIA

(In reply to ISHIKAWA, Chiaki from comment #4) > Created attachment 8547147 [details] [diff] [review] > Patch: Take 2. > > The variable was once used by holding a reference to a folder (possibly to > Inbox). > But somehow it is no longer used. > I was wondering if the current set of stream variables used in > nsPop3Sink.cpp may need an extra > stream variable, which is used to read/write Inbox *all* the time, never > closed, etc. Such a variable seems to solve the strange > premature Close of stream I observed in bug 1116055. > Post pluggable stores, you can no longer have a single stream variable that accesses a folder all the time. For maildir, the stream is per-message and not per-file. Pluggable stores does support the concept of a "reusable" stream for a folder that could be kept open for optimization purposes. > From > https://bugzilla.mozilla.org/show_bug.cgi?id=1116055#c20 > --- begin quote--- > In light of Bug 1120136 > in nsPop3Sink.cpp, I hope by having a couple of stream variables (one of them > is a reference to Inbox and will be open all the time), I can enable > the buffering in nsPop3Sink.cpp (hopefully without locking, but we may still > need locking even then.): actually, I think once the move to > a message per file storage format is taken, then this problem in > nsPop3Sink.cpp will be a non issue. > --- end quote There are no plans to disable mbox, so even if maildir is made the default, it will still be important to work well with mbox. So it is hard to see how this will become a non-issue.

Comment on attachment 8547147 [details] [diff] [review] Patch: Take 2. Review of attachment 8547147 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

(In reply to Kent James (:rkent) from comment #6) > Comment on attachment 8547147 [details] [diff] [review] > Patch: Take 2. > > Review of attachment 8547147 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me. Thank you. I will put in checkin-needed keyword. In the meantime, I filed Bug 1121842 - [META] RFC: C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. to clean up the various issues. TIA

I think this compilation and test was better. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=170c582ec307 Sorry, this is my routine test of my patches under OSX and Windows. (I am testing the patches on my local PC. Tryserver have not been able to compile and build linux version for the last couple of weeks.) At least you can see that it compiles and builds and runs most of the test successfully. Strange bugs under Windows seem to be caused by a failure of async tests under Windows from what I have gathered so far (or they may be triggered by my OTHER patches, but not this one definitely.) TIA