Closed Bug 1389789 Opened 2 years ago Closed 2 years ago

nsIPgpMimeProxy: Replace nsIStreamListener with direct call

Categories

(Thunderbird :: General, enhancement)

57 Branch
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: patrick, Assigned: patrick)

Details

Attachments

(1 file, 6 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
The attached patch replaces the nsIStreamListener interface by a new method readDecryptedData().

Enigmail master already supports this API.
Attachment #8896621 - Flags: review?(jorgk)
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.
Attachment #8896621 - Flags: review?(jorgk)
Attached patch patch_v1.1 (obsolete) — Splinter Review
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?
Attachment #8896621 - Attachment is obsolete: true
Attachment #8896770 - Flags: review?(jorgk)
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().
Attachment #8896770 - Flags: review?(jorgk) → review+
Attached patch patch_v1.2 (obsolete) — Splinter Review
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().
Attachment #8896770 - Attachment is obsolete: true
Attachment #8897463 - Flags: review?(jorgk)
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 ;-)
Attached patch patch_v1.3 (obsolete) — Splinter Review
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().
Attachment #8897463 - Attachment is obsolete: true
Attachment #8897463 - Flags: review?(jorgk)
Attachment #8897766 - Flags: review?(jorgk)
Attached patch patch_v1.4.patch (obsolete) — Splinter Review
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.
Attachment #8897791 - Flags: feedback?(patrick)
(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.
Attached patch patch_v1.4b.patch (obsolete) — Splinter Review
Sorry about the noise, one more line of comment.
Attachment #8897791 - Attachment is obsolete: true
Attachment #8897791 - Flags: feedback?(patrick)
Attachment #8897810 - Flags: feedback?(patrick)
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".
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.
Attached patch patch_v2.0.patchSplinter Review
Same solution, but maintaining the existing interface.
Attachment #8897824 - Flags: feedback?(patrick)
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 :-).
Attachment #8897824 - Flags: feedback?(patrick) → feedback+
Attachment #8897766 - Attachment is obsolete: true
Attachment #8897766 - Flags: review?(jorgk)
Attachment #8897810 - Attachment is obsolete: true
Attachment #8897810 - Flags: feedback?(patrick)
Comment on attachment 8897824 [details] [diff] [review]
patch_v2.0.patch

OK, let's go with this.
Attachment #8897824 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/37c4745fb9f1
nsIPgpMimeProxy: Indroduce nsPgpMimeProxy::OutputDecryptedData. r=jorgk
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Sorry about the typo in the commit message.
Target Milestone: --- → Thunderbird 57.0
You need to log in before you can comment on or make changes to this bug.