1389789 - nsIPgpMimeProxy: Replace nsIStreamListener with direct call

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Patrick Brunschwig]

As nsIScriptableInputStream et.al. are now deprecated, I'd like to switch to a different API that allows JavaScript functionality to call nsIPgpMimeProxy without the hassle of going through streams.

Comment 1

[Patrick Brunschwig]

Attachment: patch v1

The attached patch replaces the nsIStreamListener interface by a new method readDecryptedData().

Enigmail master already supports this API.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8896621 [details] [diff] [review]
patch v1

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

::: mailnews/mime/public/nsIPgpMimeProxy.idl
@@ +10,5 @@
>  #define NS_PGPMIMEPROXY_CONTRACTID "@mozilla.org/mime/pgp-mime-decrypt;1"
>  #define NS_PGPMIMEPROXY_CID                     \
> +{ /* 815c4fbe-0e7c-45b6-8324-f7044c7252ac */    \
> +   0x815c4fbe, 0x0e7c, 0x45b6,                  \
> +{0x83, 0x24, 0xf0, 0x04x, 0x4C, 0x72, 0x52, 0xac } }

Please compile what you submit first :-(

That 0x04x gives:
invalid literal suffix 'x'; literal operator or literal operator template 'operator ""x' not found.

Comment 3

[Patrick Brunschwig]

Attachment: patch_v1.1

Oh, I'm sorry. I updated the CID at the very end and forgot to compile :-(

Here is a fixed patch. 

The one thing I wonder in ReadDecryptedData() is if it's OK to simply take the buffer and pass it on to mOutputFun() without creating a copy of the data or something like getter_addRef()? My tests all succeeded, but could this potentially lead to crashes?

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8896770 [details] [diff] [review]
patch_v1.1

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

Sorry about the slight delay, I was busily fighting bustage yesterday.

All this is of course a mystery to me, so I can just look at it from the point of view of plausibility.

You might want to protect against crashing on calling mOutputFun(); when that has been set to null previously.

I'd really like to see this output function to see what it does with the pointer. Can you please give me a reference.

::: mailnews/mime/cthandlers/pgpmime/nsPgpMimeProxy.cpp
@@ +429,5 @@
> +  NS_ENSURE_TRUE(mInitialized, NS_ERROR_NOT_INITIALIZED);
> +
> +  NS_ENSURE_ARG(buf);
> +
> +  int status = mOutputFun(buf, buf_size, mOutputClosure);

So this is how the PGP proxy interacts with MIME. I've browsed the MIME code and all the output functions I see have:
(const char *buf, int32_t size, void *closure)
So it's completely left to that function to do the right thing. Output function and buffer get set in nsPgpMimeProxy::SetMimeCallback().

status = mOutputFun(buf, buf_size, mOutputClosure);
Since you're setting it to null below, you can crash here next time that gets called and I can't see that there is any logic to prevent that. So maybe:
int status = -1;
if (mOutputFun)
  status = mOutputFun(buf, buf_size, mOutputClosure);
There's another spot in nsPgpMimeProxy::Finish() which has the same problem.

Next you asked about passing mOutputClosure to your output function. We have:
void* mOutputClosure;
Again, in nsPgpMimeProxy::Finish() it's passed like you propose to pass it here.

Since this is a void*, you can in fact not do anything with this, so there is no protection, nsPgpMimeProxy.cpp just handles this mOutputClosure pointer. All this appears to be your code from bug 768928. I looked through that bug and no one complained about the void*, most likely since this is how things are done in MIME :-((

@@ +432,5 @@
> +
> +  int status = mOutputFun(buf, buf_size, mOutputClosure);
> +  if (status < 0) {
> +    PR_SetError(status, 0);
> +    mOutputFun = nullptr;

Why set it to null? Same in nsPgpMimeProxy::Finish().

Comment 5

[Patrick Brunschwig]

Attachment: patch_v1.2

I added checks to ensure that mOutputFun() is not null whenever it's called.

The mime handler is a "subclass" of mimeEncryptedClass, which is implemented in mimecryp.cpp. 

mOutputFun is set to mimecryp.cpp:MimeHandleDecryptedOutput().

Comment 6

[Jorg K (CEST = GMT+2)]

I've looked at this for a while to understand how it all hangs together, so I'll document it here.

There are three components: MIME, a proxy object and Enigmail.

MIME creates and initialised the proxy object which in nsPgpMimeProxy::Init() creates a decryption object, which is EnigmailMimeDecrypt. When MIME wants to decode something, it calls the Write() method of the proxy, which in turn calls OnDataAvailable() on the decryptor. The decryptor, EnigmailMimeDecrypt, does its thing and passes the data back to the proxy, previously on the proxy's OnDataAvailable() and now on its ReadDecryptedData() data method. The proxy knows how to interface with MIME and passes the data on using some function pointers it got given via nsPgpMimeProxy::SetMimeCallback().

So after all that I'm asking myself whether 'ReadDecryptedData' is a good name. Unless I really misunderstood the whole thing, that method passes decoded data back to MIME via the proxy. Maybe the 'Read' is meant to match the 'Write' where MIME writes to the decoder. But here, MIME is not reading from the decoder, instead the decoder is writing to MIME. Can you motivate the choice of name. To me, '[PassBack|Store|Output]DecryptedData' would appear more natural.

The code is fine, BTW ;-)

Comment 7

[Patrick Brunschwig]

Attachment: patch_v1.3

So, here we go with the final patch. Can you commit it to Daily?

Your analysis of how this works is very nice. I added it more or less unchanged at the top of the .cpp file, such that others can directly profit from it.

You're right, readDecryptedData() is not ideal, I renamed it to outputDecryptedData().

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: patch_v1.4.patch

Tweaked the comments some more. Why does the proxy need to be a nsIInputStream? As far as I can see, no one calls Read() or Available(). Same question for nsIRequest. You wrote that stuff initially, so you must know.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #8)
> Why does the proxy need to be a nsIInputStream?
Oops, I dug down a little further. The decryptor gets called with
return mDecryptor->OnDataAvailable((nsIRequest*) this, nullptr, (nsIInputStream*) this, 0, buf_size);
and in turn does Read() on the stream to get the encoded data in the first place.

I'm a little confused how this
  var data = this.inStream.read(count);
can work since the Read() function has more parameters.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: patch_v1.4b.patch

Sorry about the noise, one more line of comment.

Comment 11

[Jorg K (CEST = GMT+2)]

Thinking about this a little more, we could easily maintain the existing interface and add your stream-less interface on top.

You are correct that some nsI*InputStreams will be removed, but the new interface would allow add-ons to get away "without the hassle of going through streams".

Comment 12

[Jorg K (CEST = GMT+2)]

If we go for the "maintain the old", my comment would change to:

* The decryptor decrypts the data and
* passes the result back to the proxy, using the OutputDecryptedData() method
* or by passing a stream to the proxy's OnDataAvailable() method, in which
* the proxy will read from that stream.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: patch_v2.0.patch

Same solution, but maintaining the existing interface.

Comment 14

[Patrick Brunschwig]

Comment on attachment 8897824 [details] [diff] [review]
patch_v2.0.patch

We can certainly keep the old interface and add the new one. If other add-ons depend on the old API then that's a perfectly valid decision.

I checked and tried the patch, it's fine with me - thanks a lot :-).

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8897824 [details] [diff] [review]
patch_v2.0.patch

OK, let's go with this.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/37c4745fb9f1
nsIPgpMimeProxy: Indroduce nsPgpMimeProxy::OutputDecryptedData. r=jorgk

Comment 17

[Jorg K (CEST = GMT+2)]

Sorry about the typo in the commit message.