Lately, I refreshed comm-central thunderbird code and tested my local modification to enable buffering of writing downloaded message to a local mail store.
(This is about when one uses POP3. Please bear this in mind. Imap testing is further along.) I noticed a couple of things: (1) File API semantic change? : unexpected file pointer position. First, this could be my mixup of refreshing the source tree and merging the local modification to create a new set of hg patches, but has there been a change to file-handling function code so that a newly opened file's seek position (or file pointer to write or read for the next operation in other words) be placed at the beginning (i.e., 0 offset) even if the said file existed and has non-zero value and the intention is to append? I didn't write the original code, so am unsure, but the intention was to append to the file. I don't think I touched that part of the code. It seems to me that the file pointer was at the end of the file for writing to Berkely style mbox-format Inbox (so we append to it) previously before the refresh of source tree. But after the refresh, I realized it is no longer positioned at the end, but at 0-th offset, and so a call to |Seek()| before appending is now indeed necessary. I have been attempting to remove a call to |seek()| in a busy loop to write to the Inbox: I don't think this |seek()| is needed. [1] The offending |Seek()| nullifies our attempt to write to Inbox with buffering. This slows down I/O. After a couple of months testing and debugging on and off, even monitoring the checking of the file positions before and after |seek|, I thought I can remove the offending |seek()| safely at least from my limited testing. Well what about other people's environment? Screwing up one's mailbox is something that needs to be avoided at all cost. Based on the suggestion of aceman, I was about to upload a proposed patch for Beta to test that it is true that we can remove the |seek()| and still be safe in other people's environment: the patch is meant to check that the |seek()| has no effect, i.e., the file pointer position does not change before and after the offending |seek()|. There are a few preferences that can change the behavior of TB and so I wanted to make sure. Once we know for sure that it is safe to remove |Seek()|, I intend to upload the final patch to remove the |Seek()|. Above slight change in the file pointer position after opening mbox-style Inbox, I now need to insert a |Seek| once after we begin writing to it. Just once. All is well now. But I am curious if anyone is aware of such file API change. I tried to read through the changes to M-C tree for the last week, but it was long and I could not figure out if the change is in the last week or not. Thus I have to ask experts here. (2) This is more like C++ semantic issue and the way XPCOM is built. (After I wrote down the following, I found out from system trace, that maybe my change does not buffer the writing to temporary file still :-(. I will follow up when the dust settles.) With a user preference setting of "mailnews.downloadToTempFile" to true, thunderbird mail client can be taught to download a POP3 message first to a temporary file and close it, and then read the content of this file and append it to mbox-style Inbox. I think this is meant to deal with arcane very spartan anti-virus software that would REMOVE/QUARANTINE a file when a virus or malware is detected. If you receive a spam e-mail with malware, and your whole Inbox is removed/quarantined, it is a disaster. I think today's AV software is more benign and handle the situation more elegantly (I don't know the details.) Although "mailnews.downloadToTempFile" does not even exist by default, and you need to manually set it to test the feature, the code path to handle this feature DOES exist, and I needed to test the path in my quest for better I/O error handling in TB.[2][3][4][5]. When I enabled buffering of writing to a local file during downloading as in [1], and the local file is temporary (by setting mailnews.downloadToTempFile to true), I hit a snug. There is a code in the "mailnews.downloadToTempFile is true" path. http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Sink.cpp#787 * 787 nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream); 788 rv = MsgReopenFileStream(m_tmpDownloadFile, inboxInputStream); Before, as in the current release, m_outFileStream is not buffered. And the code on line 787 produces non-null inboxInputStream. However, once m_outFileStream is turned into a buffered output stream using, say, m_outFileStream = NS_BufferOutputStream(m_outFileStream, 64 * 1024 ); the code on line 787 produces nullptr. Is this to be expected? Up until now, I thought of do_QueryInterface() as mere sugar-coating for certain type-mutation or something. But I now know I am wrong. I read a page about do_QueryInterface() but it does not explain the principle very much. Is the reason of failure something like as follows. I am using a very general class hierarchy. A base class | +---+---+ B C B and C are derived from base class A | --+--+ | D D is derived further from Class D. Let's say Class B and C are derived from Class A. Class D is further derived from Class C. Let us assume there are corresponding XPCOM class/object A', B', C', D'. By using do_QueryInterface() on objects, we can follow the path of direct "derives" relation B' <= do_QueryInterface (A') (or is it the other way round?) and maybe between B' and C' (? Not sure about this.) but we can NOT follow the direction of B' <= do_QueryInterface (D') That is X = do_QeuryInterface(Y) is possible only when X is the direct or indirect descendant of Y? Thank you for clarification. It is a pity that the original authors of nsPop3*.{cpp,h} code did not define a derived class that would make it possible to execute the following code successfully nsCOMPtr <this_derived_class> inboxInputStream = do_QueryInterface(m_outFileStream); and also prepare the file APIs that accept |this_derived_class| in the position of |nsIInputFile|. TIA CI [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1116055 Bug 1116055 - Performance issue: Failure to use buffered write (comm-central thunderbird) [2] Bug 1121842 [META] RFC: C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. [3] Bug 1122698 C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (Step-1) 2015-04-08 [4] Bug 1134527 C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (Step-2) [5] Bug 1134529 C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (Step-3) [6]Bug 1159777 - Strange Failure of inboxInputStream = do_QueryInterface(m_outFileStream) in nsPop3Sink.cpp _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
