(I am not sure why there is backend store module in Thunderbird.)

I am branching this bugzilla from bug 1242030.

(Note.: the sheer large number of write system calls can overwhelm a remote CIFS server or make the message copying unduly slow. TB currently does no buffering at all while pop3 download. This means someboyd sending you 1MB photo from smartphone ends up sending the mime encoded data and usually these are 75 or 76 bytes per line. For a one MB attachment, we end up calling write system call about 15K times or so. For a single attachment. This is a clear inefficiency that I want to solve in bug 1242030.)

Now during the work over the last several years to introduce buffered write to reduce the large number of system calls, it became clear that the notion of reuse of file stream is not quite easy to handle. It makes the code unnecessarily complex and is a maintenance burden.
It was suggested that we should remove the use of reusable flag from some functions and this I did in bug 1242030.
By removing reusable flag, it became easier to modify the code to introduce buffered write.
The reduction of so many write system calls clearly gains efficiency instead of trying to save a file open system call per message. I did a comparison in bug 1242030.
The copying of messages also gain speed. Under Windows, copying many messages that took 90 seconds or so, now finishes in less than 5 seconds on my PC back then.
(Under linux, the difference is not so pronounced. Obviously, under windows10, there are so many tasks to vie for the mouse events, etc. that a context switch caused by write system call triggers a very large overhead.)

Well, it was lately suggested to extract this "remove reusable flag" portion of the patch and land this first first by breaking the whole buffered write patch set into small chunks. . I was a bit skeptical if I could do that, but I found I can.

Here are the patches to do so.
There are a few lines that I did not bother to remove from the patch here. Some error checks are in order anyway once bug 1242030 is visited after the patches in this bugzilla land.

(I made a serious typo of writing |m_outputstream| instead of the correct |m_outFileStream|. So I corrected the following paragraph)
The current plan I discussed with Ben is

1. land this "remove reusable flag" patch set.
2. • Change the POP3 error handling code to use a separate EXPLICIT |m_error| flag instead of setting |m_outFileStream| to nullptr to signal an error occurred to the upper layer functions.
3. • Then modify the code to properly enable buffered write.

Part-1 is here.
Part-2, -3 will be handled in bug 1242030 or maybe part-2 is handled in still another bugzilla entry.

TIA

I changed the name and description of the patches above, but they have been submitted to try-comm-central before the description were changed.
See
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=1f00a8ed748893fe0f4aecaa229dfd28ae04e78e

As I see it, I think I only see intermittent errors.
In the above submission there were a few extra patches I need for local compilation and local investigation of some elusive issues.
So I tried to remove at least the extra patches for C-C portion in the next submission.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=VQZqYXYbQb-ONp11CxVtmQ.0&tier=2%2C3&revision=fee8f058ed1e9ea8454c73bfad67120898b3aaf1

(In reply to ISHIKAWA, Chiaki from comment #0)

(I am not sure why there is backend store module in Thunderbird.)

The idea is that we can support different backends for storing messages. We've currently got mbox and maildir support (even if maildir is still a bit buggy - see Bug 845952). A database-backed store is an obvious future target (store your emails in sqlite, say).
Currently there are mbox-specific hacks all over the place (Bug 1719121), and I'm going through the codebase moving all those into the mbox implementation (nsMsgBrkMBoxStore).

(In reply to Ben Campbell from comment #4)

Comment on attachment 9280771 [details] [diff] [review]
DONT-USE-REUSABLE-part-1, IDL change to remove reusable flag, and a few
addition of debug message printing.

Review of attachment 9280771 [details] [diff] [review]:

Great, thanks for that - I'm really looking forward to getting all this
stuff landed!

Firstly, this patch doesn't apply cleanly, as I changed
nsMsgDBFolder::EndNewOfflineMessage() last week. Sorry about that. At a
quick glance I think the changes I made cover similar ground, so you might
not even need to change EndNewOfflineMessage() at all. I'll add some inline
comments with more details.

Secondly, the debug printfs need to be removed to land this. I'd just pull
them out into your own private patch (myself, I use the 'edit' function of
hg histedit when I want to split a changeset up into multiple changesets).
You can still attach such debug patches to the bug, just make it clear they
are for development/debugging rather than for landing.

Thank you for the comment.

Sorry, I synced my local patch with the latest patches of yours and others, but somehow I forgot to upload them.
Oops.

The biggest issue is that the patch doesn't stand on it's own. It's not
atomic - it breaks the build without some of the changes in the second patch.
I would probably just focus this first patch on removing the reusable flag
from GetNewOutputStream() - nice and simple and contains and it's already
most of the way there.
So you'll need to move just the GetNewMsgOutputStream() fixups from the
second patch into this one.

Oh, I forgot exactly why I broke the patch into two parts.
Mostly due to historical reasons caused by the existing patch set (for buffered write).
I think extracting the removal of reusable flag part this way from two different patches was the easiest.
I will look into this.

There are also some GetNewMsgOutputStream() calls in non-linux code which
will need to be patched up:

• mailnews/import/src/nsBeckyMail.cpp
• mailnews/import/src/nsAppleMailImport.cpp
• mailnews/import/src/nsOutlookMail.cpp
  And in one unit test:
• mailnews/local/test/unit/test_over4GBMailboxes.js
  (none of these seem too major)

I may have missed these. Let me check them out.
(Now I realize some errors that I see in different architecture could be due to this.)

Oh, and I'd use read-to-land formatting for the commit message:
e.g. "Bug 1773787 - Remove 'reusable' param from
nsIPluggableStore.getNewMsgOutputStream(). r=benc"
(you can still add "part-one", "part-two" eg in attachment filename, but it
doesn't need to be in the commit message)

::: mailnews/base/src/nsMsgDBFolder.cpp
@@ +1663,1 @@

The changes here in EndNewOfflineMessage() are the changes that no longer
cleanly apply. But I think it might now work fine without your changes...

@@ +1694,5 @@

• fflush(stdout);
• fprintf(stderr,"(debug) seekable is nullptr in nsMsgDBFolder::EndNewOfflineMessage.\n");
• }
  +#endif

The changes I made were to allow EndNewOfflineMessage() to work even if the
outstream isn't seekable.
This is the case when message quarantining is on, and will eventually be the
case even when it isn't (eventually, the mbox GetNewOutputStream() will
return a non-seekable stream (it'll be a custom stream which deals with all
the mbox-specific "From " handling performed safely behind the scenes).

I left the if(seekable) { block in as a sanity check, but it must work
even if seekable is null!
So take a look at my new EndNewOfflineMessage() and see if it's missing
anything you need it to do.

I will study this part as well. Maybe I may have already taken care of the superficial patch issues, but
I may have missed some deeper issues.

Thank you again.
(At least, my local patch applies cleanly to the downloaded tree from yesterday, so I should be able to create new patches in a day or two.)

It turns out that the modification under /import/ directory is in the second patch file.

(In reply to ISHIKAWA, Chiaki from comment #5)

(In reply to Ben Campbell from comment #4)

Comment on attachment 9280771 [details] [diff] [review]
DONT-USE-REUSABLE-part-1, IDL change to remove reusable flag, and a few
addition of debug message printing.

Review of attachment 9280771 [details] [diff] [review]:

Great, thanks for that - I'm really looking forward to getting all this
stuff landed!

Firstly, this patch doesn't apply cleanly, as I changed
nsMsgDBFolder::EndNewOfflineMessage() last week. Sorry about that. At a
quick glance I think the changes I made cover similar ground, so you might
not even need to change EndNewOfflineMessage() at all. I'll add some inline
comments with more details.

Secondly, the debug printfs need to be removed to land this. I'd just pull
them out into your own private patch (myself, I use the 'edit' function of
hg histedit when I want to split a changeset up into multiple changesets).
You can still attach such debug patches to the bug, just make it clear they
are for development/debugging rather than for landing.

Thank you for the comment.

Sorry, I synced my local patch with the latest patches of yours and others, but somehow I forgot to upload them.
Oops.

I intend to separate out the debug printfs.

The biggest issue is that the patch doesn't stand on it's own. It's not
atomic - it breaks the build without some of the changes in the second patch.
I would probably just focus this first patch on removing the reusable flag
from GetNewOutputStream() - nice and simple and contains and it's already
most of the way there.
So you'll need to move just the GetNewMsgOutputStream() fixups from the
second patch into this one.

Oh, I forgot exactly why I broke the patch into two parts.
Mostly due to historical reasons caused by the existing patch set (for buffered write).
I think extracting the removal of reusable flag part this way from two different patches was the easiest.
I will look into this.

Historical reason and it was the past of least labour for me.

There are also some GetNewMsgOutputStream() calls in non-linux code which
will need to be patched up:

• mailnews/import/src/nsBeckyMail.cpp
• mailnews/import/src/nsAppleMailImport.cpp
• mailnews/import/src/nsOutlookMail.cpp
  And in one unit test:
• mailnews/local/test/unit/test_over4GBMailboxes.js
  (none of these seem too major)

I may have missed these. Let me check them out.
(Now I realize some errors that I see in different architecture could be due to this.)

I figured since the try-comm-central takes care of all the architectures, I should have modified the code already.
It turns out that the changes in the /import/ directory are in the second patch file.
I think the splitting of the patch into to files was very artificial. Actually I have been testing them in combination without much thought. :-(

So with the latest synced update on my local PC, everything is in the local patch, which I will either consolidate into one patch file or
carefully comb through to make them stand in two independent parts as you suggest (since I change IDL, thus header file containing the function signature changes, I think I have to go with the single patch approach) , but I am confident the logically necessary modifications are already there.

Stay tuned.

cf. (I may want to modify the error debug part into a macro invocation which is not so intrusive visually.

               startOfMsg[msgOffset - 1] = 0;
+#ifdef DEBUG
+              // DEBUG: the error happened while checking network outage condition.
+              // XXX TODO We may need to check if dovecot and other imap
+              // servers are returning funny "From " line, etc.
+              fprintf(stderr, "(debug) Strange startOfMsg: |%s|\n", startOfMsg);
+#endif
             }
             MOZ_LOG(DBLog, LogLevel::Info,
                     ("Invalid header line in offline store: %s",
                      startOfMsg + keepMsgOffset));
             if (foundNextLine) startOfMsg[msgOffset - 1] = save;

Really strange situation ought to be reported immediately to the TB user. It should not be left behind MOZ_LOG. and
iMOZ_LOG takes up three lines here.
There are shades of urgency of mesages.

• always report ... very strange situation, maybe corrupt input. User may decide to quit immediately.
• strange/serioous ... merits investigation by developers
• error ... recoverable error. TB may want to report for developer sanity.
• informational ... only interesting to developers who look at log files.

In that regard, the above choice of "Info" level is rather bad, I think it ought to be elevated to more urgent type, and
I would turn them into a new macro call something like as follows. A tentative idea I will investigate in the next few months.

         TB_LOG(DBLog, LogLevel::Info,
                     ("Invalid header line in offline store: %s",
                      startOfMsg + keepMsgOffset)) 

which would expand to something like

#ifdef DEBUG
  /* whatever would be appropriate for serious error that needs immediate attention by developers */
  fprintf(stderr, "(debug)" "Invalid header line in offline store: %s" "\n", startOfMsg +keepMsgOffset);
#endif
             } 
             MOZ_LOG(DBLog, LogLevel::Info,
                     ("Invalid header line in offline store: %s",
                      startOfMsg + keepMsgOffset));

I need a bit of macro magic since MOZ_LOG encapsulates the format string and the arguments that follow inside a pair of "(" and ")".
So, this better macro to lessen the clutter needs a careful planning and has to wait for while.

Anyway, I hope I can upload the cleaned up patch (without the macro modification) over the weekend.

TIA

I have merged the two patches together so that it will stand alone.
Since I change .idl file and change the function signature, the usage of functions have to be changed after all, thus there is no merit of separating the definition, declaration and the usage of the functions in two patches.

Also, I have removed some enter/leave debug messages into another patch, which will follow.
That is for debugging purposes and is not essential.

I am afraid that it is created against C-C tree of a few days ago, and
it may not apply cleanly after the patch(s) in bug 1777454 land.

Actually, I left some debug output because removing reusable flag WAS a big surgery to catch any strange errors.
An additional leave/enter debug message and other debug print statement patch follows, but that is not essential.

Just FYI.

These add extra enter/leave dump statements for DEBUG build.
It should be useful for someone tracking the incorporation of incoming messages..

I wonder how other developers keep these dump statements handy while they work on main C-C tree.
Right now, I have three major hunks of patches.
A - Remove reusable flag (this bugzilla),
B - Enable buffered write (bug 1242030)
C - Handle short read (bug 1170668)
in this order.
Each of these require relatively large amount of debug printf to keep track of what is going on, especially when an error occurs during mochitest or xpcshell-test on tryserver.
Adding printf obviously changes the source tree, and when I need to test the final hunk above (bug 117066), that patch have to take care of the debug dump statements for this bugzilla and bug 1242030.

So in an indeal world where no bugs exist, I can simply create local tree with modifiation
with Patch A followed by B, further followed by C.
A- B - C
However, due to the debug printf statements for A, let's call it Da, B and C are affected during my local tests.
A+Da - B' - C'
Actually testing B' requires its own debug statements, Db', and thus now I have
A+Da - B'+Db' - C''
(C' has become C'' because it now has to accommodate the change for B', namely Db').
Of course, C'' requires its own debug statements to keep track of what goes on when we have errors on tryserver.
A+Da - B'+Db' - C''+Dc

Well, finally then, when we want to submit patches for A, we remove Da and submit the patch for A.
Now if A works well, we may want to remove Da, but in reality, for low-level error tracking we may need Da to still monitor errors on tryserver.
So the patch for B, without the needing to accommodate the Da (the debug printfs for A) have to be created carefuly when we submit the patch for B now.
B' = B + the patch needed to accommodate Da.
So we remove the additional part.

I wonder if every developer does this kind of shuffling of patches.
Or maybe I am missing something obvious.
I wonder if there is any best practice on the maintenance of debugging code and the
occasional submission of clean patches.

Fixed the bitrot due to backend changes.

Sorry there was a sleight of hand that obsoleted the wrong patch.
All is well now.

[I edited a bit to avoid unwanted formatting.]

(In reply to b1 from comment #13)

Comment on attachment 9286126 [details] [diff] [review]
DONT-USE-REUSABLE-part-1-Rev01 : fixed the bit rot due to backend changes

Review of attachment 9286126 [details] [diff] [review]:

The |Close()| calls need to be looked at.

::: mailnews/import/src/nsAppleMailImport.cpp
@@ +557,5 @@

   } else {
     msgStore->DiscardNewMessage(outStream, msgHdr);
     break;
   }

•  outStream->Close();
  

FinishNewMessage() already closes the stream, so it's closed again ...
(and many call sites below)

@@ +562,2 @@

 }
 if (outStream) outStream->Close();

... and again.

::: mailnews/import/src/nsBeckyMail.cpp
@@ +465,5 @@

   }
 } else if (IsEndOfMessage(line)) {
   inHeader = true;
   mResult = msgStore->FinishNewMessage(outputStream, msgHdr);

•  outputStream->Close();
  

Here.

::: mailnews/import/src/nsOutlookMail.cpp
@@ +411,5 @@

     IMPORT_LOG1("*** Error reading message from mailbox: %S\n",
                 (const wchar_t*)mName);
     msgStore->DiscardNewMessage(outputStream, msgHdr);
   }

•  outputStream->Close();            // no check?
  

Here. If you checked, you'd find that it's already closed.

@@ +416,4 @@

 }

}

• if (outputStream) outputStream->Close(); // no check?

Here.

Thank you for your comment.

Hmm. I ripped the portion that handles the removal of reusable flag from my patch for "buffered write" and in my haste, I may have
left code snippets that are removed in the "buffered write" patch.
But when I looked at the local patch now, I found a Close() is introduced and its return value is not checked in the "buffered write" at all (?).
Something is wrong.

Thank you for spotting this. I am investigating.

I found the following, still checking.
So interim report.

In my haste to rip apart the "removal of reuseflag" portion of the code,
I failed to take care of the |Close| in import directory files for the patch in this entry..
In my whole patch for "buffered write", these |Close()| are gone. [I have always checked the code using the whole patch. It was recent that "removal of reusable flag" code was extracted.]
So I will take care of them and produce a new patch.

However, during my current check, I found that in my "buffered write" patch, there was an extra |Close()| in imap directory file.
But I was checking the return code from |Close()|. If the path was taken during execution, I would have noticed the error from the |Close()| of already closed stream. But I did not. (Well, my local test log did not record such warning/error.)
So it is possible that the test suite does not cover that portion of the code, or that code never gets executed for real.
I fixed the code by removing the |Close()|. Good.

However, I found there is a place where |Close()| is called incorrectly in the current C-C TB.
(That is fixed in my "buffered write" patch, but as of now C-C TB source has it.)
The problem may not be fatal, but codingwise incorrect.
That is in local/nsPop3Sink::IncorporateAbort().
https://searchfox.org/comm-central/source/mailnews/local/src/nsPop3Sink.cpp#662

NS_IMETHODIMP
nsPop3Sink::IncorporateAbort(bool uidlDownload) {
  NS_ENSURE_STATE(m_outFileStream);
  nsresult rv = m_outFileStream->Close();     <====== THIS.
  NS_ENSURE_SUCCESS(rv, rv);
  if (m_msgStore && m_newMailParser && m_newMailParser->m_newMsgHdr) {
    m_msgStore->DiscardNewMessage(m_outFileStream,
                                  m_newMailParser->m_newMsgHdr);
  }
#ifdef DEBUG
  printf("Incorporate message abort.\n");
#endif
  return rv;
}

In principle, we now should let DiscardNewMessage() to handle the |Close(), so the |Close()| and return check should be removed there.
I will file a bugzilla.

I am still checking the rest of my patch (comparing the patched source tree with "removal of reusable flag" patch and the one patched with full set of "buffered write" patch.).

TIA

Comment on attachment 9286126 [details] [diff] [review]
DONT-USE-REUSABLE-part-1-Rev01 : fixed the bit rot due to backend changes

Am I right thinking there's another revision of this patch in the works?
(I did try applying it, but it didn't apply cleanly, so there's a little bitrot somewhere).
Anyway, just r? me again when ready!

(In reply to Ben Campbell from comment #16)

Comment on attachment 9286126 [details] [diff] [review]
DONT-USE-REUSABLE-part-1-Rev01 : fixed the bit rot due to backend changes

Am I right thinking there's another revision of this patch in the works?
(I did try applying it, but it didn't apply cleanly, so there's a little bitrot somewhere).
Anyway, just r? me again when ready!

Sorry, I could not upload the latest one yesterday due to the side effect of the Covid-19 vaccine which I got before noon yesterday.

I will refresh the local C-C tree to make sure there is no bitrot and upload it in a few hours.

TIA

This should apply to C-C tree cleanly (there were line shifts due to some local patches, I think.)
Also, this addresses the issue of extra }Close()| that is no longer needed after |FinishNewMessage()| or after |DisardNewMessage()| mentioned in comment 13.

The try server run is
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=71d16f6687cb8c69d0d390063b708da5171e6af8

This is non-essential. It adds some debug print statements.
The tryserver run with this debug print statements is in
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=c926ac3894f47c2341d2cb1faabc81b1866e8e1f

Comment on attachment 9286714 [details]
DONT-USE-REUSABLE-part-2-DEBUG-PRINT-for-developers debug print stmts (take 3) Non-essential.

This patch is diagnostic printfs - not intended for landing, so no review needed :-)

Any chance to get this finished? Since https://hg.mozilla.org/comm-central/rev/1846b7604187, no reusing takes place as the output streams are closed. Having all the reusable flags still around is just unneeded additional confusion.

Sorry, I missed BenC's comment 20 somehow, I will upload a patch that incorporates his comments this evening.

Need another couple of hours at least.
I accidentally merged the two patches posted (the main one and the debug print addition) and need to split them, and am checking the local compilation and then tryserver run.
Anther few hours is my estimate.

I can't get it compiled yet due to -Wsometimes-uninitialized flag introduced on the treeserver (saved me from a brownbag bug), and bug bug 1800606.
Trying to sort things out yet.

I modified the patch which had a bitrot (some chunks needed to be shifted by a few lines)
to incorporate the comment from Ben.

try server run is
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=cc0830ca7985b8bc81e0e1f6d5330b9b02464620

The patch did not introduce any ADDITIONAL ERRORs.
(I have no idea what is going on on the windows side. There are many errors, but they are there WITHOUT my patch at all.)

(In reply to Ben Campbell from comment #20)

Comment on attachment 9286713 [details] [diff] [review]
DONT-USE-REUSABLE-part-1-Rev03 (take 3) remove reusable flag

Review of attachment 9286713 [details] [diff] [review]:

Thanks!
Looking pretty good now: r- due to a few leftover printfs and chunks of dead
code, but nothing serious.

Oddly, hg import didn't apply this cleanly for me, even though all the
rejected chunks looked fine to me. patch -p1 -i <filename.patch> applied
it all fine though. No idea why hg import didn't like it...

I have no idea. Maybe the chunks needed a few lines worth shift and hg import did not like it?
The current upload should fix it against the the current C-C tree.

::: mailnews/base/src/nsMsgDBFolder.cpp
@@ +755,5 @@

+#ifdef DEBUG

• fprintf(stderr,

•        "(debug) nsMsgDBFolder::GetOfflineFileStream: "
  

•        "GetMsgInputStream(hdr, aFileStream); rv=0x%" PRIx32 "\n",
  

•        static_cast<uint32_t>(rv));
  

Rogue printf?
How about:

    NS_WARNING(nsPrintfCString(
            "(debug) nsMsgDBFolder::GetOfflineFileStream: "
            "GetMsgInputStream(hdr, aFileStream); rv=0x%" PRIx32 "\n",
            static_cast<uint32_t>(rv)).get());

instead? (and no DEBUG guard required either - NS_WARNING is compiled out
for non-debug)

I changed this.
Some other appearances of stderr were taken care of in a similar manner.

@@ +759,5 @@

•        static_cast<uint32_t>(rv));
  

+#endif

• // Return early: If we could not find an offline stream, clear the offline
• // flag which will fall back to reading the message from the server.
• if (mDatabase) mDatabase->MarkOffline(msgKey, false, nullptr);

Not suggesting any change, but I don't like this - it should be handled
higher up the call stack... but for now it does seem more consistent with
the code below to be doing it here.

Right now, I can think of making the TB code robust at this level.
Someday, if the code base is clearer, and overall error handling framework is in place.

@@ +957,5 @@

•        "(debug) nsMsgDBFolder::GetMsgInputStream: msgStore->"
  

•        "GetMsgInputStream(this, ...) returned error rv=0x%" PRIx32 "\n",
  

•        static_cast<uint32_t>(rv));
  

• }
  +#endif

Another rogue printf. Use NS_WARNING() instead?

Done.

@@ +1690,5 @@

• fprintf(stderr,

•        "(debug) seekable is nullptr in "
  

•        "nsMsgDBFolder::EndNewOfflineMessage.\n");
  

• }
  +#endif

Again, NS_WARNING()?

Done.

@@ +1744,5 @@

+#if 0

•  // was in the old code
  

•  //} else
  

•  // m_offlineHeader->SetLineCount(m_numOfflineMsgLines);
  

+#endif

I'd just delete this block - the SetLineCount() is done below.

Done.

@@ +1761,5 @@

+#ifdef DEBUG

• // We can not let this happen: I think the code assumes this.
• // That is the if-expression above is always true.
• NS_ASSERTION(msgStore, "msgStore is nullptr");
  +#endif

NS_ASSERTION() compiles out to nothing in non-DEBUG builds, so the guard
isn't needed.

Done.

Just for background: currently folders always have a non-null msgStore and
checking is inconsitant... but it seems reasonable in future for some
folders to not have a msgStore at all. So maybe one day it could be
nullptr after all.

So, for now, it is OK.
There are definitely some places in the TB code base where msgStore is not checked. A future todo issue and I hope xpcshell test and mochtest uncover the problem before TB reaches the end user.

::: mailnews/import/src/nsAppleMailImport.cpp
@@ +549,5 @@

   if (NS_FAILED(rv)) break;

•  /*In the following,
  

•    outStream is closed either by FinishNewMessage() or DiscardNewMessage()
  

•  */
  

nit: odd spacing

Fixed.

::: mailnews/import/src/nsOutlookMail.cpp
@@ +417,3 @@

 }

}

• /* outputStream has been closed either by FinsihNewMesasge() or

nit: Typo

Fixed.

::: mailnews/local/src/nsMailboxProtocol.cpp
@@ +168,5 @@

             // Keep the original and use the clone for the next operation.
             m_multipleMsgMoveCopyStream = stream;
             stream = clonedStream;

•          } /* false */
  

+#endif

Unused, so I'd just delete the block.

Removed.

@@ +310,3 @@

                                                 getter_AddRefs(stream));

•              // NS_ASSERTION(!reusable,
  

•              //              "We thought streams were not reusable!");
  

I'd just delete it - keeping it doesn't add to the understanding of the code.

Done.

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +558,3 @@

NS_ENSURE_ARG_POINTER(aResult);

• // *aReusable = false; // message per file

don't need this

Fixed.

That's it for today.

Sorry it took a couple of days due to the tryserver issue caused by bug 1800606.

Oops, I somehow reset the comment line. I am uploading the edited patch.

This is to aid the debugging. Not essential.
I use it locally.
Just FYI.

Looks good to me.
I set a try run going:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=88e43dbc33dd499fa47d221a55943a26ef3636da

If all good, I say we land it!

(In reply to Ben Campbell from comment #29)

Looks good to me.
I set a try run going:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=88e43dbc33dd499fa47d221a55943a26ef3636da

If all good, I say we land it!

Thank you. I have no idea what the errors we see in the linux runs are.
I am trying to see if it occurs locally now.

OSX seems to be busted?
Windows errors are something that occur on other people's jobs, too.
I suspect something is in very bad share in Windows land.

(In reply to ISHIKAWA, Chiaki from comment #30)

(In reply to Ben Campbell from comment #29)

Looks good to me.
I set a try run going:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=88e43dbc33dd499fa47d221a55943a26ef3636da

If all good, I say we land it!

Thank you. I have no idea what the errors we see in the linux runs are.
I am trying to see if it occurs locally now.

Hmm. I think the fails in the linux release build were due to Bug 1802068, so should be fixed. The comm/chat/components/src/test/test_logger.js certainly works for me locally now, but I'll kick off another try run just to be sure:

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=3a243f83ef82b0f9bbf50e6b2ffaa79643f45565

Non-linux, non-release builds always seem a bit flakey on the tests, so I don't entirely trust them. I think it'd be worth doing a big push on getting everything green some time soon...

Please remove the excess empty lines here before landing this:
https://hg.mozilla.org/try-comm-central/rev/0a82306503808083d85b87d583be0b2edea04761#l3.136

Any update here?

(In reply to b5 from comment #32)

Please remove the excess empty lines here before landing this:
https://hg.mozilla.org/try-comm-central/rev/0a82306503808083d85b87d583be0b2edea04761#l3.136

I am running the tryserver job with the modified patch (removed the pointed out blank lines, and also,
removed a no-longer necessary NS_ENSURE_SUCCESS().

tryserver job is
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=2dbeec4052764715e0e626c1a167f8c12424723f

Removed the extra blank lines pointed out in comment 32, and
removed

1. an extra #if DEBUG, #endif, and
   https://hg.mozilla.org/try-comm-central/rev/0a82306503808083d85b87d583be0b2edea04761#l3.134
2. an extra NS_ENSURE_SUCCESS()
   https://hg.mozilla.org/try-comm-central/rev/0a82306503808083d85b87d583be0b2edea04761#l3.37

For 2, now we return in the error branch in the immediately preceding if statement.

The try server run with the up-to-date patch that reflects comment 30 and explained in 35 produced no new
bugs (all are either intermittent errors, perma errors, or infrastructure issue).
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=2dbeec4052764715e0e626c1a167f8c12424723f

So I am carrying over benc's review to the new patch. Oops, actually he has not given the final + review yet(?)
So I am requesting the final r+ from him.
Is it OK to set checkin-needed keyword? (is this still the way to make patch brought into the tree?).

Suggest not landing this until after this week's merge. So maybe wait until Thursday or Friday.

Comment on attachment 9311851 [details]
DONT-USE-REUSABLE-part-1-Rev04.patch (take 5) Essential.

Looks good to me.

Oddly - like the previous versions - this patch doesn't cleanly hg import for me either:

$ hg import --no-commit "https://bugzilla.mozilla.org/attachment.cgi?id=9311851"
applying https://bugzilla.mozilla.org/attachment.cgi?id=9311851
patching file mailnews/base/src/nsMsgDBFolder.cpp
Hunk #1 FAILED at 732
abort: bad hunk #8 @@ -1686,45 +1722,72 @@ nsresult nsMsgDBFolder::EndNewOfflineMes
 (46 45 72 72)
(check that whitespace in the patch has not been mangled)

Any idea why? How did you export the patch?
Anyway, manually downloading and applying to the working dir with patch seems to work fine.
I've done that, fixed up one tiny typo in the nsIMsgPluggableStore.finishNewMessage() comment block and run clang-format over it.
I'll export and upload the tweaked patch and hopefully that will hg import cleanly.
(I was going to submit it via phabricator, but I couldn't upload a patch that had me as a reviewer, even though I'm not the author!)

And like Wayne said, we can hold off a couple of days to land it.

(In reply to Ben Campbell from comment #38)

I've done that, fixed up one tiny typo in the nsIMsgPluggableStore.finishNewMessage() comment block and run clang-format over it.

The comment blocks could be improved overall. The patch starts off with // comments and then drops into /* */ used in a rather non-standard way. There are some colloquialisms as well, best example here:

  /* DiscardNewMessage closes outputStream */
  /* Well, we have to call Close() explicitly
     in non-failure case to release OS resources */

A better comment would be:

  // DiscardNewMessage() closes outputStream.

That the code calls Close() if DiscardNewMessage() isn't called is quite obvious. Streams/files are not only closed to release resources but also to flush buffers and to unblock access. The coding standard calls for full sentences terminated by a full stop.

(In reply to Wayne Mery (:wsmwk) from comment #37)

Suggest not landing this until after this week's merge. So maybe wait until Thursday or Friday.

The merge is done. So no need to wait.

(In reply to Ben Campbell from comment #38)

Comment on attachment 9311851 [details]
DONT-USE-REUSABLE-part-1-Rev04.patch (take 5) Essential.

Looks good to me.

Oddly - like the previous versions - this patch doesn't cleanly hg import for me either:

$ hg import --no-commit "https://bugzilla.mozilla.org/attachment.cgi?id=9311851"
applying https://bugzilla.mozilla.org/attachment.cgi?id=9311851
patching file mailnews/base/src/nsMsgDBFolder.cpp
Hunk #1 FAILED at 732
abort: bad hunk #8 @@ -1686,45 +1722,72 @@ nsresult nsMsgDBFolder::EndNewOfflineMes
 (46 45 72 72)
(check that whitespace in the patch has not been mangled)

Any idea why?
To be honest, I have no idea.

? How did you export the patch?
I simply copied the local (still MQ) patch to a windows partition which is accessed from the firefox browser to
attach the patch to this bugzillla. (I run Debian GNU/Linux inside virtualbox under Windows and the guest linux is my main development environment at home. I read/write e-mails and browse websites including bugzillla on the host Windows.)

I can't think of a simple scenario of why a particular hunk for a particular hunk fails.
I updated M-C and C-C using "hg pull -u" and applied the MQ patch(es) to make sure it built locally and on tryserver as well.
There may be some whitespace handling issues somewhere. But the copying of local patch to a different partition
is not one of them. Also, since my try run built on tryserver. So, at that point the patch was OK.
I wonder if there were some whitespace modifications later to the tree (maybe by the pretty-printer?)

Anyway, manually downloading and applying to the working dir with patch seems to work fine.
This is another mystery to me. Is there special restrictive handling of diff by hg?

I've done that, fixed up one tiny typo in the nsIMsgPluggableStore.finishNewMessage() comment block and run clang-format over it.
I'll export and upload the tweaked patch and hopefully that will hg import cleanly.
(I was going to submit it via phabricator, but I couldn't upload a patch that had me as a reviewer, even though I'm not the author!)

And like Wayne said, we can hold off a couple of days to land it.

Thank you for that. Will you be able to handle the comment issue raised in comment 40?
I still have not moved off MQ and so I won't be able to use phabricator, etc.

It sounds as if you need still ANOTHER somebody who can move the patch to phabricator and do some changes there?

(In reply to ISHIKAWA, Chiaki from comment #42)

I can't think of a simple scenario of why a particular hunk for a particular hunk fails.

Ah well, no big problem.

Thank you for that. Will you be able to handle the comment issue raised in comment 40?

Done, and new patch uploaded.

It sounds as if you need still ANOTHER somebody who can move the patch to phabricator and do some changes there?

No, it's fine just uploading it here as an attachment. Phab is great for revising patches and chewing over all the detail that get a bit unwieldly here in the Bug, leaving it free for higher-level discussion. It's also a little bit simpler to land from phab I think, but we can still land patches from attachments here.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/bee6fb38d2d1
Remove 'reusable' flags from msgStore stream functions. r=benc

(In reply to Pulsebot from comment #45)

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/bee6fb38d2d1
Remove 'reusable' flags from msgStore stream functions. r=benc

Wow, thank you, finally!
My patch queue is now a bit shorter.