1120136 - C-C: mailnews/local/src/nsPop3Sink.{h,cpp} variable m_inboxOutputStream is no longer used.

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Remove the references to m_inboxOutputStream, but see the comment.

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

Comment 1

[ISHIKAWA, Chiaki]

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 2

[ISHIKAWA, Chiaki]

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.

Comment 3

[Kent James (:rkent)]

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

Comment 4

[ISHIKAWA, Chiaki]

Attachment: Patch: Take 2.

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

Comment 5

[Kent James (:rkent)]

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

[Kent James (:rkent)]

Comment on attachment 8547147 [details] [diff] [review]
Patch: Take 2.

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

Looks good to me.

Comment 7

[ISHIKAWA, Chiaki]

(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

Comment 8

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/9c41d93b92d1

Comment 9

[ISHIKAWA, Chiaki]

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