768928 - PGP/MIME proxy implementation for mimelib

Status: VERIFIED FIXED

(Thunderbird :: General, enhancement)

Description

[Patrick Brunschwig]

Attachment: Patch v1

In order to change Enigmail into a pure JS implementation, some basic mimelib functionality written needs to be moved into Thunderbird.

The attached patch implements a new mimelib class (plus the corresponding hooks) that allow Enigmail to register to mimelib and process PGP/MIME encrypted data.

My implementation assumes that mimelib is single-threaded (and therefore only 1 instance of nsPgpMimeProxy is needed). I'm not sure if my assumption is correct; otherwise I'd have to change a few things.

Comment 1

[David :Bienvenu]

this does not build on windows:

nsMailModule.cpp
nsMailModule.cpp
c:/builds/tbirdhq/objdir-tb/mailnews/build/../../../mailnews/build/nsMailModule.
cpp(1023) : error C2440: 'initializing' : cannot convert from 'nsresult (__stdca
ll *)(nsISupports *,const nsIID &,void **)' to 'mozilla::Module::ConstructorProc
Ptr'
        None of the functions with this name in scope match the target type

In the directory  c:/builds/tbirdhq/objdir-tb/mailnews/build

Comment 2

[David :Bienvenu]

because you need to use static nsresult instead of static NS_IMETHODIMP...

Comment 3

[David :Bienvenu]

Comment on attachment 637153 [details] [diff] [review]
Patch v1

in general, I prefer nsnull to NULL.

+static NS_IMETHODIMP
+nsPgpMimeMimeContentTypeHandlerConstructor(nsISupports *aOuter,
+                                         REFNSIID aIID,
+                                         void **aResult)
+{

static nsresult...

use NS_ENSURE_ARG_POINTER(aResult);
NS_ENSURE_TRUE(aOuter, NS_ERROR_NO_AGGREGATION);

try using nsRefPtr to help with ref-counting:
nsRefPtr<nsMimeContentTypeHandler> inst(new nsMimeContentTypeHandler("mulitpart/encrypted",
+                                      &MIME_PgpMimeCreateContentTypeHandlerClass);)

NS_ENSURE_TRUE(inst, NS_ERROR_OUT_OF_MEMORY);

+  if (inst == NULL) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }

though I think new failing with oom aborts the app now...
space after ,:
+  rv = inst->QueryInterface(aIID,aResult);

I'd rather use NS_ASSERTION.
+  PR_ASSERT(!oclass->class_initialized);

return on its own line:
+  if (!(obj && obj->options && output_fn)) return NULL;

+  data = new MimePgpeData;
   use NS_ENSURE_TRUE...
+  if (!data)
+    return NULL;

did you try nsDependentCString(ct) here? :

+    nsCString contentType(ct, strlen(ct));
+    rv = data->mimeDecrypt->SetContentType(contentType);
+  }
+  else {
and EmptyCString() here:

+    nsCString noCt("");
+    rv = data->mimeDecrypt->SetContentType(noCt);
+  }

you can use PR_Free (PR_FREEIF nulls the local variable, which isn't useful). And PR_Free checks for null.

+  PR_FREEIF(ct);
+
+  return data;

return on its own line:
+  rv = data->mimeDecrypt->Write(buf, buf_size);
+  if (NS_FAILED(rv)) return -1;

no need for braces here:

+  if (!data || !data->output_fn) {
+    return -1;
+  }

or here:
+  if (msg) {
+    PL_strcpy(msg, htmlMsg);
+  }
+  return msg;

use false here: 
nsPgpMimeProxy::nsPgpMimeProxy()
+  : mInitialized(PR_FALSE),

and true here:
+  mOutputClosure = outputClosure;
+  mInitialized   = PR_TRUE;

no need for temp assignment here, just return OnStopRequest directly.
+
+  if (mDecryptor) {
+    rv = mDecryptor->OnStopRequest((nsIRequest*) this, nsnull, NS_OK);
+    return rv;
+  }

try avoiding PR_FREEIF by using :
nsCString tString.Adopt(Pgp...)

+    char *tString = PgpMimeGetStringByID(PGPMIME_STR_NOT_SUPPORTED_ID);
+    temp.Append(tString);
+    PR_FREEIF(tString);

I think you might want to add equivalent properties files for suite.

Comment 4

[David :Bienvenu]

Comment on attachment 637153 [details] [diff] [review]
Patch v1

minusing based on review comments, mostly style things...

Comment 5

[Patrick Brunschwig]

Attachment: Patch v2

New patch, integrated all review comments. Thanks!

Comment 6

[Patrick Brunschwig]

Attachment: Little add-on to for testing

The attached addon processes a message received from nsPgpMimeProxy (i.e. multipart/encrypted messages):
1. It creates a new MIME structure containing a plaintext body and an attachment
2 [review]. writes to the plaintext the content type 
3. adds all lines received prefixed by a line number
4. writes to the attachment the number of lines found.

Comment 7

[Patrick Brunschwig]

Attachment: 2 messages for testing (mbox file)

Comment 8

[David :Bienvenu]

Comment on attachment 637540 [details] [diff] [review]
Patch v2

extension and test messages seem to work fine. So r=me, as long as the following comments are addressed:

why not make this a class? 

+typedef struct MimePgpeData

then the comptr will automatically get init/cleared by the default constructor/destructor. I know libmime tends to use structs, but we've slowly been teaching it about c++.

if do_GetService fails, doesn't it return null?

+  data->mimeDecrypt = do_GetService(NS_PGPMIMEPROXY_CONTRACTID, &rv);
+  if (NS_FAILED(rv)) {
+    data->mimeDecrypt = nsnull;
+    return data;
+  }

this is really nitpicky, but I'd prefer no braces, and putting multiple args on the same line until 80 chars, and indenting the second line, if needed, at the (.
+  if (mDecryptor) {
+    rv = mDecryptor->OnDataAvailable((nsIRequest*) this,
+      nsnull,
+      (nsIInputStream*) this,
+      0,
+      buf_size);
+  }
+

only need one empty line here:
+  return 0;
+}
+
+
+typedef struct MimePgpeData

and here:
+  data->output_closure = output_closure;
+  data->mimeDecrypt = nsnull;
+
+
+  nsresult rv;
+  data->mimeDecrypt = do_GetService(NS_PGPMIMEPROXY_CONTRACTID, &rv);

and a few other places. Could you fix those?

don't need braces here:
+#ifdef PR_LOGGING
+  if (gPgpMimeProxyLog == nsnull) {
+    gPgpMimeProxyLog = PR_NewLogModule("nsPgpMimeProxy");
+  }
+#endif

personally, I wouldn't put the #ifdef PR_LOGGING in either. None of our other code, except for address book, does that.

here, you can move the decl of rv to where it's used:

+nsPgpMimeProxy::OnDataAvailable(nsIRequest* aRequest, nsISupports* aContext,
+                              nsIInputStream *aInputStream,
+                              PRUint32 aSourceOffset,
+                              PRUint32 aLength)
+{
+  nsresult rv;
+

and you'll need an sr since you're adding interfaces. I've requested Mark, but you could use Neil if you want to get a SeaMonkey blessing...

Comment 9

[Patrick Brunschwig]

Attachment: Patch v3

New patch, addressing (hopefully) all findings by David.

Is my understanding correct that if I want reference counting, the class MimePgpeData needs to be an implementation of nsISupports, or is it sufficient to simply change it to a class?

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 638190 [details] [diff] [review]
Patch v3

As David attracted my attention to this, I thought I'd add a few comments whilst its fresh. 

Can you attach git-based patches in future? It makes reviewing easier and I believe it generally works better with hg importing patches (see https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for more info).

>+[scriptable, uuid(c200d239-6dad-434f-a614-1b58a58c61ec)]
>+interface nsIPgpMimeProxy : nsIStreamListener
>+{
>+  /**
>+   * set the decoder callback into mimelib
>+   */
>+  [noscript] void setMimeCallback (
>+                       in MimeDecodeCallbackFun outputFun,
>+                       in voidPtr outputClosure);
>+

nit: no space after "setMimeCallback", and please use two-space indent on the subsequent lines.

>+++ b/mail/locales/en-US/chrome/messenger/pgpmime.properties	2012-06-11 17:51:51.000000000 +0200
>+## @name NS_MSG_UNABLE_TO_OPEN_FILE
>+## LOCALIZATION NOTE: the text can contain HTML tags.
>+1000=This is an encrypted OpenPGP message.<br>In order to decrypt this mail, you need to install an <a href="https://addons.mozilla.org/en-US/thunderbird/addon/enigmail/">OpenPGP add-on</a>.

There's a couple of issues here.

First I know the existing code uses IDs, but we should just give the string a text name as that's far better for localisers and if we need to change it in future. We've been migrating our code to not use IDs as we change things.

Secondly, I don't want the url hard-coded in the l10n files, I think it would be best split out as a pref, and have a parameter replacement for L10n. Also drop the en-US from the url and the browser will detect the right locale to hit. The main reason for doing this is if we ever did want to change the url (reorg of amo or something), then we wouldn't have to change 50+ locales.

>+++ b/mailnews/mime/cthandlers/pgpmime/Makefile.in	2012-06-27 17:52:11.000000000 +0200

nit: Makefiles aren't generally using tabs any more, unless needed for command lines. Where needed, just use two-space indent.

>+class nsPgpMimeProxy : public nsIPgpMimeProxy,
>+                         public nsIRequest,
>+                         public nsIInputStream
>+{
>+public:
>+    NS_DECL_ISUPPORTS
>+    NS_DECL_NSIPGPMIMEPROXY

nit: align the publics in the class definition inline, and use two-space indents.

>+++ b/mailnews/mime/cthandlers/pgpmime/pgpmime.def	2012-06-01 17:49:39.000000000 +0200

I don't think we use .def files any more, is this really needed? (I think the ones still in the source tree are legacy).

>+++ b/mailnews/mime/cthandlers/pgpmime/nsPgpMimeProxy.cpp	2012-07-01 16:57:46.000000000 +0200
>+#define PGPMIME_PROPERTIES_URL        "chrome://messenger/locale/pgpmime.properties"
>+#define PGPMIME_STR_NOT_SUPPORTED_ID  1000
>+
>+static char *PgpMimeGetStringByID(PRInt32 aMsgId)

Might as well just pass a nsCString reference argument and assign the result direct to that. This would avoid the need for a separate allocation & adoption.

>+NS_IMETHODIMP
>+nsPgpMimeProxy::Init()
>+{
>+  mByteBuf.Assign("");

mByteBuf.Truncate();

(also in SetMimeCallback and other places).

>+nsPgpMimeProxy::Write(const char *buf, PRUint32 buf_size)
...
>+  nsresult rv = NS_OK;
...
>+  if (mDecryptor)
>+    rv = mDecryptor->OnDataAvailable((nsIRequest*) this, nsnull, (nsIInputStream*) this,
>+                                     0, buf_size);
>+
>+  return rv;

If mDecryptor exists you could just return the result of mDecryptor->OnDataAvailable... and then return NS_OK if it doesn't.

>+nsPgpMimeProxy::Write(const char *buf, PRUint32 buf_size)

>+  mByteBuf.Assign(buf, buf_size);
>+  mByteCount = buf_size;

So if mByteCount = mByteBuf.Length() is there a point in storing the count separately?

Comment 11

[David :Bienvenu]

(In reply to Patrick Brunschwig from comment #9)
> Created attachment 638190 [details] [diff] [review]
> Patch v3
> 
> New patch, addressing (hopefully) all findings by David.
> 
> Is my understanding correct that if I want reference counting, the class
> MimePgpeData needs to be an implementation of nsISupports, or is it
> sufficient to simply change it to a class?

If you want refcounting, yes, that's what nsISupports provides.

My comment about using a class vs. a struct was probably specious. But, I think you don't need to have explicit constructors or destructors for the member vars that are comptrs - that should just happen for free.

Comment 12

[David :Bienvenu]

Comment on attachment 638190 [details] [diff] [review]
Patch v3

this doesn't appear to be used, and global comptrs cause problems, iirc.

+nsCOMPtr<nsIPgpMimeProxy> mMimeDecrypt;
+

and again, you shouldn't need a destructor to null out the comptr; the default destructor should do that:

+  ~MimePgpeData()
+  {
+    mimeDecrypt = nsnull;
+  }

might try ? operator here, or just use a local var, i.e., instead of:
+  if (ct) {
+    rv = data->mimeDecrypt->SetContentType(nsDependentCString(ct));
+  }
+  else {
+    rv = data->mimeDecrypt->SetContentType(EmptyCString());
+  }
  PR_Free(ct);

do 
  nsCString contentType;
  contentType.Adopt(ct);
  rv = data->mimeDecrypt->SetContentType(contentType);

you don't need a temp var here:

+  nsresult rv;
+  rv = data->mimeDecrypt->Write(buf, buf_size);
+  if (NS_FAILED(rv))
+    return -1;


you can just do

if (NS_FAILED(data->mimeDecrypt...)
  return -1;

or even
  return (NS_SUCCEEDED(data->mimeDecrypt...)) ? 0 : -1;

here, you should assign to rv on the same line you declare it:

+  nsresult rv;
+  rv = data->mimeDecrypt->Finish();
+  if (NS_FAILED(rv))
+    return -1;

i.e.,
  nsresult rv = data->mimeDecrypt->Finish();

other than that, r=me...

Comment 13

[Patrick Brunschwig]

Attachment: Patch v4

New patch addressing the comments by David and Mark:
- removed global nsCOMPtr
- changed property to string ID
- moved URL to preference
- fixed the various indentations, spaces and usage of rv.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 639375 [details] [diff] [review]
Patch v4

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

This looks great, sorry for the delay in reviewing this.

::: mail/locales/en-US/chrome/messenger/pgpmime.properties
@@ +5,5 @@
> +#
> +# The following are used by the pgpmime content type handler
> +#
> +
> +## @name NS_MSG_UNABLE_TO_OPEN_FILE

No need for this line now.

@@ +7,5 @@
> +#
> +
> +## @name NS_MSG_UNABLE_TO_OPEN_FILE
> +## LOCALIZATION NOTE: the text can contain HTML tags.
> +## LOCALIZATION NOTE: %URL% is the url to Enigmail on AMO.

nit: the normal form for these would be:

# LOCALIZATION NOTE(pgpMimeNeedsAddon): The text can contain HTML tags. %S is the url to Enigmail on AMO supplied from preferences.

@@ +8,5 @@
> +
> +## @name NS_MSG_UNABLE_TO_OPEN_FILE
> +## LOCALIZATION NOTE: the text can contain HTML tags.
> +## LOCALIZATION NOTE: %URL% is the url to Enigmail on AMO.
> +pgpMimeNeedsAddon=This is an encrypted OpenPGP message.<br>In order to decrypt this mail, you need to install an <a href="%URL%">OpenPGP add-on</a>.

Please use the %S form instead of %URL% as that's what localisers are more used to.

::: mailnews/mime/cthandlers/Makefile.in
@@ +15,5 @@
>  endif
>  
>  PARALLEL_DIRS += glue vcard
>  
> +DIRS += pgpmime

Can you add a note about the fact this depends on glue.

::: mailnews/mime/cthandlers/pgpmime/Makefile.in
@@ +21,5 @@
> +
> +EXTRA_DSO_LIBS	= mimecthglue_s
> +
> +CPPSRCS		= \
> +		nsPgpMimeProxy.cpp \

nit: we're generally going for 2-space indents in makefiles now.

::: mailnews/mime/cthandlers/pgpmime/nsPgpMimeProxy.cpp
@@ +54,5 @@
> +#define PGPMIME_PROPERTIES_URL        "chrome://messenger/locale/pgpmime.properties"
> +#define PGPMIME_STR_NOT_SUPPORTED_ID  "pgpMimeNeedsAddon"
> +#define PGPMIME_URL_PREF              "mail.pgpmime.addon_url"
> +
> +static char *PgpMimeGetStringByName(const char* aMsgId)

If you keep this function, please consider using nsCString & as a parameter which will allow to remove the extra string allocations.

@@ +65,5 @@
> +                                    getter_AddRefs(stringBundle));
> +  if (stringBundle)
> +  {
> +    nsString v;
> +    if (NS_SUCCEEDED(stringBundle->GetStringFromName(

Please use the formatStringFromName, then you can use %S directly as mentioned previously.

Comment 15

[Mark Banner (:standard8)]

I landed this on Patrick's behalf with my comments addressed, so that we could get this in for TB 16:

https://hg.mozilla.org/comm-central/rev/f0e7508fd586

Thanks Patrick.

Comment 16

[[e-mail]]

Attachment: External API fixes

#include fixes that allow this to compile when configuring with --enable-incomplete-external-linkage.

Comment 17

[Patrick Brunschwig]

Comment on attachment 642754 [details] [diff] [review]
External API fixes

This looks good. Sorry, I didn't try --enable-incomplete-external-linkage

Comment 18

[[e-mail]]

Comment on attachment 642754 [details] [diff] [review]
External API fixes

Pushed comm-central changeset 8bfd14466ea4.

Comment 19

[Patrick Brunschwig]

The patch is working as expected on comm-aurora.

Comment 20

[[e-mail]]

Attachment: External API fixes

Apparently I didn't fix external API builds hard enough, something must have been transitively including this before.