210927 - mimeservice is supposed to be treated as a service

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[timeless]

Comment 1

[timeless]

Attachment: comptr/do_GetService, use nsresult...

Comment 2

[timeless]

Comment on attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...

For some unknown reason this patch triggers:
###!!! ASSERTION: This function was called on two disconnected nodes!: 'Error',
file
mozilla/content/base/src/nsContentUtils.cpp, line 1110
Break: at file mozilla/content/base/src/nsContentUtils.cpp, line 1110

I need to figure out why :(

Comment 3

[Tim]

Comment on attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...

I made a silly mistake and gave timeless bad information (the assertion occurs
regardless of his patch). Undoing his changes per his request.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...

This needs review from someone who actually knows this code.. in particular,
nsMimeConverter is NOT the MIME service; I don't know whether it's supposed to
be a service at all... and neither does Neil, I'm sure.

Please look up cvsblame for this code and ask for appropriate review.

Comment 5

[[e-mail]]

Comment on attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...

>+  nsCOMPtr<nsIMimeConverter> converter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);
>+  if (!converter)
>+    return nsnull;
Except nsnull isn't an nsresult.
> 
>-  return NS_SUCCEEDED(res) ? 0 : -1;
>+  nsresult res = converter->EncoderDestroy(data, abort_p);
>+  return res;
Just return converter->EncoderDestroy(data, abort_p);
You aren't consistent, sometimes you do get these right.

Comment 6

[timeless]

Attachment: thanks neil

Comment 7

[[e-mail]]

Comment on attachment 126792 [details] [diff] [review]
thanks neil

Looks good to me.

Comment 8

[[e-mail]]

Comment on attachment 126792 [details] [diff] [review]
thanks neil

Spoke too soon :-(
No rv in MIME_EncoderWrite

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 126792 [details] [diff] [review]
thanks neil

Need new patch re Neil's comment before sr-ing.

Comment 10

[:aceman]

Is this still relevant? Is timeless working on it?

Comment 11

[Ludovic Hirlimann [:Usul]]

(In reply to :aceman from comment #10)
> Is this still relevant? Is timeless working on it?

No he ain't.

Comment 12

[:aceman]

It seems that today the line(in several functions):
nsIMimeConverter *converter;
nsresult res = nsComponentManager::CreateInstance(NS_MIME_CONVERTER_CONTRACTID, nsnull, 
                                   NS_GET_IID(nsIMimeConverter), (void **)&converter);

is converted to:

nsIMimeConverter *converter;
nsresult res = CallCreateInstance(NS_MIME_CONVERTER_CONTRACTID, &converter);

The patch wants to convert the original line to this:
nsCOMPtr<nsIMimeConverter> converter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);

Neil, should this still be done today?

Comment 13

[[e-mail]]

(In reply to aceman from comment #12)
> nsIMimeConverter *converter;
> nsresult res =
> nsComponentManager::CreateInstance(NS_MIME_CONVERTER_CONTRACTID, nsnull, 
>                                    NS_GET_IID(nsIMimeConverter), (void **)&converter);
I don't actually recognise this way of creating instances, but I'm so used to using CallCreateInstance etc. that I don't actually know what predated them.

CallGetService is the service version of CallCreateInstance, thus
> nsresult res = CallCreateInstance(NS_MIME_CONVERTER_CONTRACTID, &converter);
becomes
> nsresult res = CallGetService(NS_MIME_CONVERTER_CONTRACTID, &converter);

do_CreateInstance is the nsCOMPtr version of CallCreateInstance, thus
> nsIMimeConverter *converter;
> nsresult res = CallCreateInstance(NS_MIME_CONVERTER_CONTRACTID, &converter);
becomes
> nsresult res;
> nsCOMPtr<nsIMimeConverter> converter = do_CreateInstance(NS_MIME_CONVERTER_CONTRACTID, &res);

do_GetService is just making both changes at once, i.e.
> nsresult res;
> nsCOMPtr<nsIMimeConverter> converter = do_GetService(NS_MIME_CONVERTER_CONTRACTID, &res);

Note that the &res is optional, the equivalent is calling CallCreateInstance without saving the result code.

Comment 14

[:aceman]

Thanks, according to that answer it looks like the change the patch wants to do is wanted.

I'll try to refresh it.

Comment 15

[:aceman]

Attachment: updated patch

Updated patch. I skipped this change, because I think it changes the logic (status will not be set):

@@ -758,9 +760,9 @@ nsMsgSendPart::Write()
   
   if (m_encoder_data) 
   {
-    status = MIME_EncoderDestroy(m_encoder_data, PR_FALSE);
+    nsresult rv = MIME_EncoderDestroy(m_encoder_data, PR_FALSE);
     m_encoder_data = nsnull;
-    if (status < 0)
+    if (NS_FAILED(rv))
       goto FAIL;
   }

And the parts in MimeEmitters seem to be already applied.

Comment 16

[[e-mail]]

Comment on attachment 598939 [details] [diff] [review]
updated patch

Sorry for taking so long to get back to you on this.

> #include "nsComponentManagerUtils.h"
> 
> extern "C" MimeEncoderData *
>+MIME_B64EncoderInit(nsresult (*output_fn) (const char *buf, PRInt32 size, void *closure), void *closure)
> {
>   MimeEncoderData *returnEncoderData = nsnull;
>+  nsCOMPtr<nsIMimeConverter> converter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);
Unfortunately do_GetService is in nsServiceManagerUtils.h, which means that the build will break under certain configurations. (nsComponentManagerUtils.h is however no longer necessary with these changes.)

>+    nsresult rv = MIME_EncoderWrite(m_encoder_data, encoded_data, length);
>+    if (NS_FAILED(rv))
>+      status = -1;
[See below.]

>-    if (!buffer) return NS_ERROR_OUT_OF_MEMORY;
>+    NS_ENSURE_TRUE(buffer, NS_ERROR_OUT_OF_MEMORY);
[Bah, this method returns an int, not an nsresult...]

>     status = MIME_EncoderDestroy(m_encoder_data, false);
>     m_encoder_data = nsnull;
>     needToWriteCRLFAfterEncodedBody = !m_parent;
>     if (status < 0)
>       goto FAIL;
It would be nice to have some way of treating MIME_EncoderDestroy as an nsresult, so as to be consistent with SMIME which does it correctly. I notice you worked around it for MIME_EncoderWrite above, maybe you could copy that?

Comment 17

[:aceman]

Attachment: patch v3

There are more spots in nsMsgSendPart.cpp where a function is declared int but sometimes returns some status and sometimes NS_ERROR_*. Can I do something about that in a new bug? Convert it all to nsresult? Would it be hard?

Comment 18

[[e-mail]]

(In reply to aceman from comment #17)
> There are more spots in nsMsgSendPart.cpp where a function is declared int
> but sometimes returns some status and sometimes NS_ERROR_*. Can I do something
> about that in a new bug? Convert it all to nsresult? Would it be hard?
I imagine a new bug to convert to nsresult would be the best option, but I don't really know how hard it would be, maybe bienvenu could weigh in?

Comment 19

[[e-mail]]

Comment on attachment 606862 [details] [diff] [review]
patch v3

>-#include "nsComponentManagerUtils.h"
Looks good but I meant for you to replace this with nsServiceManagerUtils.h
r=me with that fixed.

Comment 20

[:aceman]

And why when it still compiles?

Comment 21

[[e-mail]]

(In reply to comment #16)
> the build will break under certain configurations.
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

(In reply to aceman from comment #20)
> And why when it still compiles?
The old code would have compiled in the configuration you were using without that line too, but that was no reason to remove it.

Comment 22

[:aceman]

Attachment: patch v4

Ok.

Comment 23

[David :Bienvenu]

(In reply to [e-mail] from comment #18)
> (In reply to aceman from comment #17)
> > There are more spots in nsMsgSendPart.cpp where a function is declared int
> > but sometimes returns some status and sometimes NS_ERROR_*. Can I do something
> > about that in a new bug? Convert it all to nsresult? Would it be hard?
> I imagine a new bug to convert to nsresult would be the best option, but I
> don't really know how hard it would be, maybe bienvenu could weigh in?

Probably fairly easy.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/cd3cd74613f8