A question about do_QueryInterface()

[ISHIKAWA, Chiaki]

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

[Boris Zbarsky]

On 4/30/15 2:25 PM, ISHIKAWA, Chiaki wrote:
> Is this to be expected?

Sure. You're taking an _output_ stream and QIing it to
nsI_Input_Stream.

It might happen that some objects implement both interfaces (and looks
like nsMsgFileStream does). The object returned by
NS_BufferOutputStream does not: it's an output stream, but not an input
stream.

I recommend keeping track of both the input _and_ output stream in
members, and buffering only the output, or buffering them separately, as
you prefer.

> I read a page about do_QueryInterface() but it does not
> explain the principle very much.

It's simple: You pass the object a 128-bit IID, it returns a pointer to
something that implements that interface if that's an IID it implements,
and null otherwise.

> X = do_QeuryInterface(Y) is possible only when X is the direct or
> indirect descendant of Y?

No, that has nothing to do with it. It's all about what the object
claims to implement.

-Boris

[ISHIKAWA, Chiaki]

Thank you for the clarification.

On 2015/05/01 3:38, Boris Zbarsky wrote:
> On 4/30/15 2:25 PM, ISHIKAWA, Chiaki wrote:
>> Is this to be expected?
>
> Sure. You're taking an _output_ stream and QIing it to
> nsI_Input_Stream.
>

Yes, that is how the original code was written.

> It might happen that some objects implement both interfaces (and looks
> like nsMsgFileStream does). The object returned by
> NS_BufferOutputStream does not: it's an output stream, but not an input
> stream.

Oh, I see. So non-buffered version was OK, but now that I introduce
buffering, it is no longer possible.

> I recommend keeping track of both the input _and_ output stream in
> members, and buffering only the output, or buffering them separately, as
> you prefer.
>

I will try to do something sensible, but I do not want to meddle with
the original code as much as possible, too.

>> I read a page about do_QueryInterface() but it does not
>> explain the principle very much.
>
> It's simple: You pass the object a 128-bit IID, it returns a pointer to
> something that implements that interface if that's an IID it implements,
> and null otherwise.
>
>> X = do_QeuryInterface(Y) is possible only when X is the direct or
>> indirect descendant of Y?
>
> No, that has nothing to do with it. It's all about what the object
> claims to implement.

Now I see. I thought of do_QueryInterface() has something to do with
class hierarchy, but it has nothing to do with.
It is about the object's claim that it implements certain interface.

Thank you again.

BTW, the seeming lack of buffering was that I failed to
remove the offencing |Seek()| that negates buffering.
Once I disabled it in my local builds, everything seems AOK!



> -Boris

Chiaki Ishikawa

>

[Joshua Cranmer 🐧]

On 4/30/2015 1:25 PM, ISHIKAWA, Chiaki wrote:
> * 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?

In short, yes. What happens is that the original m_outFileStream happens
to be of type nsFileStreams (or something like that), which inherits
from both nsIInputStream and nsIOutputStream. When you wrap that in a
buffered output stream, the resulting type of m_outFileStream is of
nsBufferedOutputStream, which does not inherit nsIInputStream; therefore
the cast to nsIInputStream fails.

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

do_QueryInterface is the equivalent of a type-checked downcast, e.g.
(ClassName)foo in Java. (Regular C++ downcasts are not dynamically
type-checked).

>
> 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?

No, you are incorrect. The issue is the dynamic type of the object (if
you have A *x = new B, the static type of x is A whereas the dynamic
type is B). In the pre-modified code, the dynamic type of
m_outFileStream supported the interface in question, but your
modification changed the dynamic type to one that did not support the
interface.

--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

[Seth Fowler]

> On Apr 30, 2015, at 12:09 PM, Joshua Cranmer 🐧 <Pidg...@gmail.com> wrote:
>
> do_QueryInterface is the equivalent of a type-checked downcast, e.g. (ClassName)foo in Java. (Regular C++ downcasts are not dynamically type-checked).

do_QueryInterface is, in other words, essentially equivalent to dynamic_cast in C++, except that because it’s implemented manually people can do strange things if they want to. They almost never do, though, so dynamic_cast is a pretty good mental model.

- Seth

[ishikawa]

Thank you. I am learning bit by bit.

I did not realize that the do_QueryInterface has something to do with
"dynamic type".

Hmm, a lot to learn just to be able to use less buggy mail client :-)

Thank you again.

CI

[ishikawa]

Quoting Joshua,

>> if you have A *x = new B, the static type of x is A whereas the dynamic type is B).

do_QueryInterface() handles the XPCOM interface issues, though.
I need to investigate a little more about how similar class/type objects can
produce
the difference between
the set of XPCOM interfaces supported by A and the set of interfaes
supported by B.

I think my main question after the knowledge gained would be:
Does inheritance of class inherits the XPCOM interfaces supported by the
base class automatically?
[Come to think of it, no I don't thinkso. XPCOM is an artificial framework
tacked on C++. Or XPCOM interface in C++ is written in such a manner that
they are automatically inherited?]
Or do we have to manually and explicitly state that
a set of interfaces are inherited (and of course, implemented, too)?
Or considering the implementation issue, may the answer not be quite crystal
clear (???)


TIA