1733849 - nsIMsgPluggableStore.getMsgInputStream() should return a stream for a single message, not entire mbox

Status: RESOLVED FIXED

(MailNews Core :: Backend, task)

Description

[Ben Campbell]

The mbox implementation of nsIMsgPluggableStore.getMsgInputStream() currently returns an nsIInputStream, offset to point to the start of the message.
It relies on the caller to figure out how long the message is (either by parsing the message, or by using the messageSize stashed in the nsIMsgDBHdr).

It should return an inputstream which contains only that single message. Starting at offset zero, and returning an EOF at the end.
This is what happens for maildir, and it should happen for mbox too.
(Ping did this in https://bugzilla.mozilla.org/show_bug.cgi?id=1719413#c15, using SlicedInputStream, and I'm planning to do something similar here)

The mbox situation is a little tricky. We know the start offset of the message (it's used as the store token), but the mbox code will need to parse the message itself: stepping past any leading "From ", spitting out the headers and body verbatim, EOFing when the end of the body is detected (either the real mbox EOF, or another "From " separator).

It sounds bad to have to parse the message like this, but we'll have to do it here anyway, because we have to perform "From "-escaping upon the message body (see Bug 1717137, for a start, but there will be more).
There are mbox format variants which avoid "From "-escaping entirely (by adding a Content-Length header), but even if we support those, we still need to support old formats in the wild.

Comment 1

[Ben Campbell]

Some places use nsIMsgFolder.getMsgInputStream() directly, and some go via nsIMsgFolder.getOfflineFileStream() (which is mostly just a thin wrapper).

I've gone through all the places that call these functions. It looks mostly straightforward and doable, with some minor exceptions.

Here are my notes:

Users of nsIMsgFolder.getOfflineFileStream():

• nsMailboxService::StreamHeaders()

  • ineffective? should be zapped?

• nsNNTPProtocol::ReadFromLocalCache()

  • gets stream, slices it, adds a pump
  • a good example for others to follow?

• nsMsgDBFolder::GetSlicedOfflineFileStream()

  • what GetOfflineFileStream() should be
  • merge

• comm/mailnews/imap/test/unit/test_largeOfflineStore.js

• comm/mailnews/imap/test/unit/test_downloadOffline.js

• comm/mailnews/imap/test/unit/test_compactOfflineStore.js

• nsImapMailFolder::FetchMsgPreviewText()

  • straightforward

• nsImapMailFolder::GetOfflineFileStream()

  • some X-GM_MSGID shenanigans - can fetch message from other folder.

• nsImapMockChannel::ReadFromLocalCache()

  • mostly same as nNNTPProtocol version?

• nsImapService::StreamHeaders()

  • as per nsMailboxService version.

Users of nsIMsgFolder.getMsgInputStream():

• nsParseNewMailState::MoveIncorporatedMessage()

  • Passes the inputstream to nsParseNewMailState::AppendMsgFromStream(), which is essentiall just a copy (to a msgstore outstream).

• nsMailboxProtocol::Initialize(nsIURI* aURL)

  • if instream is reusable and url is for multiple messages (copy or move with more than one message):
    • clones the stream and saves it in m_multipleMsgMoveCopyStream for next operation.
  • creates a sliced version and creates an input transport for it m_transport.

• nsMailboxProtocol::OnStopRequest()

  • Used only if m_multipleMsgMoveCopyStream is null.

  question: who uses the nsMailboxProtocol URLs for copy/move?
  That's nsIMailboxUrl::ActionCopyMessage and nsIMailboxUrl::ActionMoveMessage

• nsMsgLocalMailFolder::GetUidlFromFolder(nsLocalFolderScanState* aState, nsIMsgDBHdr* aMsgDBHdr)

  • If instream is reusable, it's stashed in nsLocalFolderScanState.m_inputStream and m_seekableStream.

  GetUidlFromFolder() looks for netscape X-UIDL header. Is that ever even used these days?

• nsMsgLocalMailFolder::FetchMsgPreviewText()

  • passes stream down to nsMsgDBFolder::GetMsgTextFromStream(), which is sync.

  FetchMsgPreviewText() implemented by local and IMAP. Should make it always async!

  nsMsgDBFolder::GetMsgTextFromStream() has some header parsing using nsIMimeHeaders - could be a useful example.

• MailTestUtils.jsm loadMessageToString()

  • simple, no funny business

• nsMsgDBFolder::GetOfflineFileStream()

  • Performs mbox header-sniffing shenanigans. Those should be moved into mbox store.
  • Is otherwise a very thin wrapper around GetMsgInputStream().

• nsImapMailFolder::CopyMessagesOffline()

  • all local, no funny business.

• nsImapOfflineSync::ProcessAppendMsgOperation()

  TODO: remove messageOffset seek in nsImapOfflineSync::ProcessAppendMsgOperation(). GetMsgInputStream should be fine.

• nsMsgSearchScopeTerm::GetInputStream()

  • redundant? Users should just call GetMsgInputStream() directly?
  • only user seems to be nsMsgBodyHandler::OpenLocalFolder()

• openpgp filters.jsm

  • simple just slurps into string.

Comment 2

[Ben Campbell]

Attachment: Bug 1733849 - Use nsIMsgFolder.getSlicedOfflineFileStream() instead of getOfflineFileStream(). r=mkmelin

This is an intermediate step. We want to get all the message-reading code
working with streams that contain just a single message, rather than with an
unwieldly mbox stream + offset/size combo.
Then we'll remove getOfflineFileStream() entirely and rename
getSlicedOfflineFileStream() to replace it.

Comment 3

[Ben Campbell]

Attachment: Bug 1733849 - Make nsIMsgPluggableStore.getMsgInputStream() return stream positioned at start of message (rather than start of mbox file). r=mkmelin

Depends on D127372

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/94ffdd55a9b2
Use nsIMsgFolder.getSlicedOfflineFileStream() instead of getOfflineFileStream(). r=mkmelin

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/59bec5562937
Make nsIMsgPluggableStore.getMsgInputStream() return stream positioned at start of message (rather than start of mbox file). r=mkmelin

Comment 6

[Ben Campbell]

Attachment: Bug 1733849 - Correct the byte counting in MailTestUtils.loadMessageToString(). r=mkmelin

A bit of a stop-gap. loadMessageToString() could be mostly replacd by NetUtil.readInputStreamToString()
but that function drains the inputstream, stopping at EOF. Our mbox inputstreams don't yet support that.
So for now, loadMessageToString() relies on the message length in the nsIMsgDBHdr, stopping before EOF.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2d54a1f20760
Correct the byte counting in MailTestUtils.loadMessageToString(). r=mkmelin

Comment 8

[Ben Campbell]

Attachment: Bug 1733849 - Remove unused nsISeekable QI in nsMsgDBFolder::WriteStartOfNewLocalMessage(). r=mkmelin

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/75b66e20fc7a
Remove unused nsISeekable QI in nsMsgDBFolder::WriteStartOfNewLocalMessage(). r=mkmelin

Comment 10

[Ben Campbell]

The correct thing to do here is for input streams produced by the msgStore to EOF at the end of the message. But for now, the mbox msgStore can't to that - the returned inputstream will let you happily keep reading in all the subsequent messages...
Ultimately, the mbox msgStore will have an inputStream that parses the message properly, stopping with a proper EOF at the end of the message (and handling "From " escaping behind the scenes).

But for an intermediate step we can move to using a Sliced input stream to provide just the single message.
To do that we need to know the length of the message in advance. We can get that from the msgHdr. But it depends on the folder type.
For local folders, all messages are stored locally and the size is in .messageSize.
For IMAP and News folders, only Offline messages are stored locally, and the size is in .offlineMessageSize.

For more background, see Bug 1764857.

Comment 11

[Ben Campbell]

Attachment: Bug 1733849 - Add nsIMsgFolder.getLocalMsgStream() to unify local/offline message reading. r=mkmelin

This patch makes some changes to the nsIMsgFolder interface. It:

• Replaces getSlicedOfflineFileStream() with getLocalMsgStream().
  The old fn was specific to IMAP/News folders.
  The new fn also handles local folders.
• Removes getOfflineFileStream() from the public interface (although it's
  still used behind the scenes to handle the IMAP gmail hack to let
  messages appear to be in multiple folders).
• Deprecates getMsgInputStream(), which returns the whole mbox file, rather
  than just a single message.

Comment 12

[Ben Campbell]

Just a couple of heads-ups (don't need any feedback, so feel free to just clear the NI flag without reply if you want):

Ping: you added getSlicedOfflineFileStream() for Bug 1719413, and I'm just extended the technique to cover local folders too. Don't think this'll affect you much, but thought you should know!

Chiaki: I know you're doing work on stream buffering, and this might cause you some merging (sorry).

See comment 10 for where I'm going with this work.
My next step is to replace nsIMsgDBHdr.getMsgInputStream() calls with getLocalMsgStream().
That'll get all the code accustomed to seeing just a single message in a stream, rather than (potentially) a whole mbox.
After that, I can move all the mbox handling code (i.e. "From " separators and all the complication they add all over the codebase) into the mbox msgStore (Bug 1719121).

Comment 13

[Ping Chen [:rnons] (away)]

Thanks, good work

Comment 14

[ISHIKAWA, Chiaki]

(In reply to Ben Campbell from comment #12)

Chiaki: I know you're doing work on stream buffering, and this might cause you some merging (sorry).

Thank you for the heads-up.
I am doing merging work since early this year. My hardware issue (an ECC memory system going awry) was a blow.
Now the hardware has been replaced, but I am hampered by the really verbose debug build test log.
The merging has been done as far as source code is concerned and the source tree compiles.
However, I am not sure if some test failures I observe locally is caused by my patches or not right now.
I believe some are, but I have not been able to find out how the errors occur. :-(

I hope I can sync with your on-going mods very soon and post finished patches, but I am not holding my breath yet.
(We may have to clean up verbose debug log somehow first.)

Again, keep up the good work. I also found a few points that might interest you to improve your patches once the merge is complete.

Comment 15

[Magnus Melin [:mkmelin]]

Regarding message size: how do we get it in the first place? Be aware servers can lie about the size so we can't trust anything non-local.
The other thing about message size is probably the need to consider partially downloaded (pop and imap) messages.

Comment 16

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #15)

Regarding message size: how do we get it in the first place? Be aware servers can lie about the size so we can't trust anything non-local.
The other thing about message size is probably the need to consider partially downloaded (pop and imap) messages.

It's set in a few places - couldn't tell you exact details off the top of my head, but in general I think most code counts the number of bytes it writes to the outputstream and then sets .messageSize or .offlineMessageSize.
Lying servers should be OK. That value will end up in .messageSize (and the actual number of bytes written locally will be put into .offlineMessageSize). For local folders, the bytes written locally is stored in .messageSize (and I think .offlineMessageSize is zero).
It'd be nice to make this more consistent - .offlineMessageSize would always hold the size of the message in the local msgStore, and .messageSize would either be a copy of that (for local folders), or the server-supplied size. But I need to think about that a little more (the IMAP gmail multi-folder hack needs some investigation, for one).

In any case, the exact message size is only an issue at the moment because the mbox msgStore doesn't know when to stop reading - it just continues on into the next message.
Ultimately, the msgStore will return inputstreams for reading which really do EOF at the end of the message. For now, this patch fakes that by using the message size from the DB and a SlicedinputStream, but the aim is to not have to rely on that - the msgStore will just handle it (along with "From " quoting etc).
At that point, the message size is mainly to show to the user, and for making retention/compaction/expunge decisions, and not required for reading the message from the msgStore.
(sorry, that was a bit of a rambling description, but I've had a lot of these complicated little details kicking about in my head for a while looking to get out ;- )

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/21bbb5c18ad0
Add nsIMsgFolder.getLocalMsgStream() to unify local/offline message reading. r=mkmelin

Comment 18

[ISHIKAWA, Chiaki]

Ben,

This sounds a very strange question, but might have these patches change the
semantics of GetNewMsgOutputStream
as in

rv = m_msgStore->GetNewMsgOutputStream(m_folder, getter_AddRefs(newHdr),
                                         getter_AddRefs(m_outFileStream));

ever so slightly?

You know I am working on the BUFFERED output of messages and
I work on the m_outFileStream returned by GetNewMsgOutputStream(...),
first enabling buffering of m_outFileStream
and then making sure we get to the EOF position since the buffering operation turns out to
rewind the stream to its beginning (!).

I am removing the reusable flag feature from stream handling because

• as it turned out, it is not so much of a performance win in modern hardware/software (I believe I posted the performance comparison result requested before.) The result is the win due to buffering outweighs any negative impact caused by NOT using reusable feature.
  The buffering makes the copying of 1000 messages from one buffer to the other happen in less than 5 seconds under Windows while the old code
  takes 90 seconds or more. (Based on real messages from gdb-mailing list, etc.)
• the reusable feature makes file descriptor handling (including caching necessitated by reusability) too complex and hairy (in the meaning of MIT hackers of the old days),
  so I removed it.

So the handling of downloading a pop3 message is
IncorporateMessage begins
GetNewMsgOutputStream is called to obtain a output stream.
Buffering is enabled on that stream.
Seek to the end of the stream is performed.
Write downloaded message to that stream, <--- not sure if this is correct or not these days.
and THEN ALWAYS close the stream.
End Incorporate Message.

Problem I found in the last couple of days is that this scenario works for the first message, but
for the second message, it fails.
Even the simple test of

mach xpcshell-test --verbose comm/mailnews/local/test/unit/test_pop3Download.js

I can download the first message OK. So I am puzzled.
In the second run for the second message,
the seek fails. (The error is that the underlying buffer stream has already been CLOSED?! So somehow file stream that should have been associated with it is gone.)
The write also fails in the end (I think when the buffered output is written and the reason is the same. Underlying file stream has been closed.)

Any idea?

I know this bugzilla is mainly for INPUT, but since there is a mention
Bug 1733849 - Correct the byte counting in MailTestUtils.loadMessageToString(). r=mkmelin
so there is a remote chance something in the output function might have changed slightly. (Actually that particular patch does not
look responsible.)
You may be doing other works related to message store (including output phase) so you may have a hunch here.

NB.: I am not ruling out a subtle copy & paste error, but
the new problems I noticed only in the last couple of days (suddenly I got so many errors), seem to be related to this
and other patches possibly.

Oh, I should mention, that I have not updated my local source tree to focus on fixed problems for a few weeks.
I did not want to change the tree so often so that I can focus on known bugs.
Another reason I did not update the code is the hardware problem of my PC.
My host windows 10 pro OS failed to boot. I run linux as a guest OS inside VirtualBox that runs under windows10.
It took me almost a couple of weeks to fully recover to the point I can use VirtualBox again. OS recovery essentially wipes out registry and as it turns out VirtualBox stores the virtual disk [mapped to Windows 10 file] in registry. Agha.

Anyway, I updated the source tree three days ago or so and suddenly I noticed these new errors.
And the reason for the error is related to the seeking/writing to the stream returned by GetNewMsgOutputStream () for the second message.
The first message is handled OK.

Given that you seem to have changed the default download to use a temporary file (last year?) by default,
the legacy patch I have been working may have missed the subtle change it may require to cope with this temporary file.
But like I said, the latest problems I have not seen before a few days ago is related to this
strange seek fails on the second message incorporation processing and the first message handling does not fail.
(I don't believe this was the case before. But I may have to go back and check the old local logs.
But the error I saw was not that clear cut if I recall correctly. I could be wrong. I changed to a CPU to a slightly newer one and my build time is about 1.7 times faster than before and so I can work on patches more efficiently. That is why I could focus on the issue this weekend.)

In the meantime, if you have any hunch, for example, like we should not close the stream returned by
GetNewMsgOutputStream() these days until the whole one pop3 download is complete or something like that
I would love to hear that.

TIA
Chiaki

Comment 19

[ISHIKAWA, Chiaki]

I think my patch for buffering is not working very well with quarantine handling.
nsMsgBrkMBoxStore::GetNewMsgOutputStream( ......) calls
InternalGetNewMsgOutputStream(aFolder, aNewMsgHdr,
aResult)
or
nsQuarantinedOutputStream(mboxStream)

depending on whether quarantine is on.

Do you know what is the test for checking TB with quarantine turned on and off for pop3 download?
I want to check the file handling in those tests.
Oh, I think I should be writing to Maildev mailing list about this.
The patch for buffered writing looks innocent enough, but somewhere I have made some implicit assumptions how the
file streams behave and changing due to quarantine might have broken the assumptions, and the recent
changes in the tree have made them clear.

Comment 20

[Magnus Melin [:mkmelin]]

See the tests affected: https://searchfox.org/comm-central/search?q=mailnews.downloadToTempFile&path=&case=true&regexp=false

Comment 21

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin [:mkmelin] from comment #20)

See the tests affected: https://searchfox.org/comm-central/search?q=mailnews.downloadToTempFile&path=&case=true&regexp=false

Thank you very much.
I think my existing local patch closes the Inbox file a bit too early when merged with the current C-C tree.
(It seems to close the file before the incorporation of the second message happens. I am now trying to find out where that close() comes from.
I suspect the interaction with download-to-tempfile interaction. I will report back.)

Comment 22

[Ben Campbell]

(sorry about slow reply - I've been away ill)

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

Ben,

This sounds a very strange question, but might have these patches change the
semantics of GetNewMsgOutputStream

I don't /think/ so - none of the patches here have touched any of the output.

Replying more generally here - if there are any specific points I don't hit, let me know!

The sequence for writing a message is:

• call nsIMsgPluggableStore.getNewMsgOutputStream() to get the output stream
• write the message
• call nsIMsgPluggableStore.finishNewMessage() to commit the write (or nsIMsgPluggableStore.discardNewMessage() to abort the write)

The calling code should not call .close() on the output stream (but it wouldn't surprise me if there were are a few places where this might still happen... they'd require fixing!).
Under normal operation, calling .close() probably won't affect things, as long as finishNewMessage() is also called. If message quarantining is on, calling .close() will abort the message.

[snip]

I am removing the reusable flag feature from stream handling because

Great! I've started removing the reusable flag it from the reading side. it should be gone by the time I close this bug.

For writing, I could imagine that it might be worthwhile for the mbox to cache outputstreams (i.e. keeping the file open), especially if the buffered stream implementation ends up reading in the beginning chunk of the file every time (you mention it resets to the beginning of the file, which suggests that it might be reading the start of the file into the buffer every time...)
But such caching should be handled entirely within the mailstore code, and the caller should never know about it. It just calls GetNewMsgOutputStream() to get an outputstream to write to, does it's stuff, then calls finishNewMessage() when it's done (which may one day choose to leave the file open behind the scenes).

So the handling of downloading a pop3 message is
IncorporateMessage begins
GetNewMsgOutputStream is called to obtain a output stream.
Buffering is enabled on that stream.
Seek to the end of the stream is performed.
Write downloaded message to that stream, <--- not sure if this is correct or not these days.
and THEN ALWAYS close the stream.
^----- this step, I think, might be the problem? Where is FinishNewMessage() invoked?
End Incorporate Message.

Given that you seem to have changed the default download to use a temporary file (last year?) by default,

I don't think this is the case... I'm pretty sure it's only if message quarantining is on, and I don't think that's by default...

In the meantime, if you have any hunch, for example, like we should not close the stream returned by
GetNewMsgOutputStream() these days until the whole one pop3 download is complete or something like that
I would love to hear that.

Yes, definitely don't close() the stream :-)

Comment 23

[Ben Campbell]

Attachment: Bug 1733849 - Remove use of deprecated nsIMsgFolder.getMsgInputStream() in nsParseNewMailState. r=mkmelin

Use nsIMsgFolder.getLocalMsgStream() instead, which returns a single message rather than entire mbox.

Comment 24

[Ben Campbell]

Attachment: Bug 1733849 - Extract stream copy from nsParseNewMailState::AppendMsgFromStream() into nsMsgUtils SyncCopyStream(). r=mkmelin

The source stream in AppendMsgFromStream() is now bounded to a single message,
so we can just copy until EOF, rather than counting bytes.

Depends on D147606

Comment 25

[Ben Campbell]

try run covering those last two patches:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=2b0ab2be6b0779fe464321df7373a03930351e22

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/86ea7effd7b8
Remove use of deprecated nsIMsgFolder.getMsgInputStream() in nsParseNewMailState. r=mkmelin
https://hg.mozilla.org/comm-central/rev/e0eb4bb286d8
Extract stream copy from nsParseNewMailState::AppendMsgFromStream() into nsMsgUtils SyncCopyStream(). r=mkmelin

Comment 27

[Magnus Melin [:mkmelin]]

(In reply to Pulsebot from comment #17)

https://hg.mozilla.org/comm-central/rev/21bbb5c18ad0

imap-js needs some adjustments for this - https://searchfox.org/comm-central/rev/8513d87a80bff2e4eee8115e3c18c8a5dc26e3dd/mailnews/imap/src/ImapChannel.jsm#103

Comment 28

[[e-mail]]

deprecated nsIMsgFolder.getMsgInputStream()

Are you planning to remove this API before or after Thunderbird 102?

Also, you've added an API getLocalMsgStream() to nsIFolder but there's no obvious way for a third party experiment that extends JsAccount to implement it in JS (no access to SlicedInputStream). Is this intended to be a public API used by other Thunderbird components?

Comment 29

[Ben Campbell]

(In reply to [e-mail] from comment #28)

deprecated nsIMsgFolder.getMsgInputStream()

Are you planning to remove this API before or after Thunderbird 102?

Also, you've added an API getLocalMsgStream() to nsIFolder but there's no obvious way for a third party experiment that extends JsAccount to implement it in JS (no access to SlicedInputStream). Is this intended to be a public API used by other Thunderbird components?

Ahh - wasn't aware that SlicedInputStream isn't available to JS :-( But then I wasn't intending that a JS folder would ever have to implement this hack...
The plan is that nsIMsgFolder.getLocalMsgStream() will be a more-or-less direct pass-through to nsIMsgPluggableStore.getMsgInputStream(), which will return an nsIInputStream which serves up just a single message (and deals with storage-specific quirks internally, like mbox "From " separator lines and quoting). So no slicing needed.
But right now, the nsIMsgPluggableStore doesn't have the required information (message size determination is different depending on folder type, sigh), so the folder deals with it and uses slicing to restrict the stream to the single message.

Looks like I might need to add a hack to nsIMsgPluggableStore which can do the slicing (given the message size). That gives a JS folder an avenue, even if not ideal.

nsIMsgFolder.getMsgInputStream() will be removed as soon as I can. It just shouldn't be by new code - it returns (potentially) multiple messages, and I'm trying to move the codebase to assuming it can rely on an EOF at the end of a message.

Comment 30

[Ben Campbell]

Attachment: Bug 1733849 - Expose SlicedInputStream to JS to allow JsAccount folders to implement getLocalMsgStream(). r=darktrojan

Comment 31

[Ben Campbell]

@neil:
That patch just slaps a SlicedInputStream construction method onto nIMsgPluggableStore. Not ideal, but should at least let you implement .getLocalMsgStream() in JS.
Your JS-implemented getLocalMsgStream() would have to:

1. get the messsage size (usualy .messageSize or .offlineMessageSize in the msgHdr, depending on folder type - see Bug 1764857).
2. get the raw stream using nIMsgPluggableStore.getMsgInputStream().
3. use nIMsgPluggableStore.sliceStream(instream, 0, msgSize) to confine the stream to just that message (getMsgInputStream positions the stream at the start of the message, hence the 0 offset here).

Eventually this will be all handled inside nIMsgPluggableStore.getMsgInputStream() (but using a custom inputstream which handles mbox "From " quoting etc etc), but for now this should let you do what you need I think.

Would this patch work for you?

Comment 32

[[e-mail]]

(In reply to Ben Campbell from comment #29)

The plan is that nsIMsgFolder.getLocalMsgStream() will be a more-or-less direct pass-through to nsIMsgPluggableStore.getMsgInputStream(), which will return an nsIInputStream which serves up just a single message (and deals with storage-specific quirks internally, like mbox "From " separator lines and quoting). So no slicing needed.
But right now, the nsIMsgPluggableStore doesn't have the required information (message size determination is different depending on folder type, sigh), so the folder deals with it and uses slicing to restrict the stream to the single message.

Can't the folder pass the message size to nsIMsgPluggableStore.getMsgInputStream()? Just thinking aloud here, without knowing the details of what your plan was.

(In reply to Ben Campbell from comment #31)

1. get the message size (usually .messageSize or .offlineMessageSize in the msgHdr, depending on folder type - see Bug 1764857).

This is its own folder type, which is sort of the point here. But if you're interested, we're storing the server's idea of the message size in .messageSize, and then if the message gets downloaded for offline use (which is currently automatic as unfortunately we can't properly integrate with Thunderbird's offline support right now) then we set the offline flag and store the local message size in .offlineMessageSize.

3. use nIMsgPluggableStore.sliceStream(instream, 0, msgSize) to confine the stream to just that message (getMsgInputStream positions the stream at the start of the message, hence the 0 offset here).

Any particular reason to require a value which will always be 0?

Would this patch work for you?

  RefPtr<mozilla::SlicedInputStream> slicedStream =
      new mozilla::SlicedInputStream(already_AddRefed(inStream), start,
                                     uint64_t(length));

I don't know why SlicedInputStream has to be a special snowflake (other stream-wrapping streams such as BinaryInputStream and ScriptableInputStream just take a raw nsIInputStream*), but you'll need to create a local nsCOMPtr to add a reference to inStream which you can then give to SlicedInputStream via forget().

Comment 33

[Ben Campbell]

(In reply to [e-mail] from comment #32)

(In reply to Ben Campbell from comment #29)

The plan is that nsIMsgFolder.getLocalMsgStream() will be a more-or-less direct pass-through to nsIMsgPluggableStore.getMsgInputStream(), which will return an nsIInputStream which serves up just a single message (and deals with storage-specific quirks internally, like mbox "From " separator lines and quoting). So no slicing needed.
But right now, the nsIMsgPluggableStore doesn't have the required information (message size determination is different depending on folder type, sigh), so the folder deals with it and uses slicing to restrict the stream to the single message.

Can't the folder pass the message size to nsIMsgPluggableStore.getMsgInputStream()? Just thinking aloud here, without knowing the details of what your plan was.

It could, but that'd be a more intrusive change at this point. This patch is an icky hack, yes, but it's an icky hack which is completely separate to any existing C++ code paths.
The ultimate goal is that we're not using SlicedStreams for this at all, and that nsIMsgPluggableStore.getMsgInputStream() returns a nsIInputStream which produces one and only one message (then EOF) and doesn't need any size info to be passed in from the message hdr (the nsIMsgPluggableStore implementation should know when to stop reading).
We're just not quite there yet, hence the folder's getLocalMsgStream() having to do this fudging about with SlicedStreams to approximate the eventual working of be nsIMsgPluggableStore.getMsgInputStream() :-(

(In reply to Ben Campbell from comment #31)

1. get the message size (usually .messageSize or .offlineMessageSize in the msgHdr, depending on folder type - see Bug 1764857).

This is its own folder type, which is sort of the point here. But if you're interested, we're storing the server's idea of the message size in .messageSize, and then if the message gets downloaded for offline use (which is currently automatic as unfortunately we can't properly integrate with Thunderbird's offline support right now) then we set the offline flag and store the local message size in .offlineMessageSize.

Yup, that's how IMAP and News do it.
Local folders store the offline size in .messageSize.
So getting the size of a locally-stored message is currently dependent on folder type. ugh.
I have an idea that local folders don't set the offline flag on the message either...

3. use nIMsgPluggableStore.sliceStream(instream, 0, msgSize) to confine the stream to just that message (getMsgInputStream positions the stream at the start of the message, hence the 0 offset here).
   Any particular reason to require a value which will always be 0?

Not really - just exposing the standard SlicedStream construction params.

  RefPtr<mozilla::SlicedInputStream> slicedStream =
      new mozilla::SlicedInputStream(already_AddRefed(inStream), start,
                                     uint64_t(length));

I don't know why SlicedInputStream has to be a special snowflake (other stream-wrapping streams such as BinaryInputStream and ScriptableInputStream just take a raw nsIInputStream*), but you'll need to create a local nsCOMPtr to add a reference to inStream which you can then give to SlicedInputStream via forget().

I'm pretty sure that passing in the already_AddRefed(inStream) is equivalent to creating a temporary nsCOMPtr to pass in, then calling .forget(). (But not absolutely 100% sure :-)

Comment 34

[Ping Chen [:rnons] (away)]

(In reply to Magnus Melin [:mkmelin] from comment #27)

(In reply to Pulsebot from comment #17)

https://hg.mozilla.org/comm-central/rev/21bbb5c18ad0

imap-js needs some adjustments for this - https://searchfox.org/comm-central/rev/8513d87a80bff2e4eee8115e3c18c8a5dc26e3dd/mailnews/imap/src/ImapChannel.jsm#103

Thanks, fixed in https://phabricator.services.mozilla.com/D149848

Comment 35

[Ben Campbell]

Neil - time is getting a little short for 102. Will this patch tide you over? I'm pretty sure it's enough to implement .getLocalMsgStream() in JS.
Not the perfect solution, but it is separate to any other code paths, so landing it isn't likely break anything.

Comment 36

[[e-mail]]

(In reply to Ben Campbell from comment #35)

Will this patch tide you over?

I double-checked and the patch crashes as written. Adding the local nsCOMPtr variable fixed it for me, and I was able to use getLocalMsgStream to replace getOfflineFileStream.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/825433407b3b
Expose SlicedInputStream to JS to allow JsAccount folders to implement getLocalMsgStream(). r=darktrojan

Comment 38

[Worcester12345]

Looks like a checkin was requested, a "push", and even a dependency was fixed. Still nothing here? Did this fall of some calendar?

Comment 39

[Ben Campbell]

Oops, I think it was just a case of forgetting to close the bug...
Or is there still something missing that I don't know about? If not, I'll close it.

To recap:
This bug uses slicedstreams to confine reading to a single message (rather than open access to, say, the whole mbox).
This caused a problem for jsaccount folders, as there's no way to create sliced streams from JS. As an interim hack, I added a method to the nsIMsgPluggableStore to create sliced streams.

But the main show is going to be Bug 1719121, which aims to ditch the slicedstream workarounds altogether and confine all the mbox awfulness in one place. It's mostly there, but it'll be a big change to the plumbing - mbox assumptions are everywhere.

Comment 40

[Ben Campbell]

Bug 1719121 is done now, so I'm closing this one.