Last Comment Bug 210927 - mimeservice is supposed to be treated as a service
: mimeservice is supposed to be treated as a service
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: Trunk
: All All
-- normal (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks: 736766
  Show dependency treegraph
 
Reported: 2003-06-27 13:21 PDT by timeless
Modified: 2012-11-10 11:14 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
comptr/do_GetService, use nsresult... (14.37 KB, patch)
2003-06-27 13:37 PDT, timeless
neil: review-
Details | Diff | Splinter Review
thanks neil (14.35 KB, patch)
2003-06-30 17:04 PDT, timeless
neil: review-
Details | Diff | Splinter Review
updated patch (10.07 KB, patch)
2012-02-20 12:20 PST, :aceman
neil: review-
Details | Diff | Splinter Review
patch v3 (10.74 KB, patch)
2012-03-17 07:10 PDT, :aceman
neil: review+
Details | Diff | Splinter Review
patch v4 (10.78 KB, patch)
2012-03-17 09:33 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description User image timeless 2003-06-27 13:21:41 PDT
 
Comment 1 User image timeless 2003-06-27 13:37:39 PDT
Created attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...
Comment 2 User image timeless 2003-06-27 14:07:15 PDT
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 User image Tim 2003-06-27 14:15:46 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2003-06-28 20:16:45 PDT
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 User image neil@parkwaycc.co.uk 2003-06-30 05:14:32 PDT
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 User image timeless 2003-06-30 17:04:13 PDT
Created attachment 126792 [details] [diff] [review]
thanks neil
Comment 7 User image neil@parkwaycc.co.uk 2003-07-01 01:52:58 PDT
Comment on attachment 126792 [details] [diff] [review]
thanks neil

Looks good to me.
Comment 8 User image neil@parkwaycc.co.uk 2003-07-01 02:34:00 PDT
Comment on attachment 126792 [details] [diff] [review]
thanks neil

Spoke too soon :-(
No rv in MIME_EncoderWrite
Comment 9 User image Dan Mosedale (:dmose) 2005-06-07 08:46:06 PDT
Comment on attachment 126792 [details] [diff] [review]
thanks neil

Need new patch re Neil's comment before sr-ing.
Comment 10 User image :aceman 2012-01-23 08:36:15 PST
Is this still relevant? Is timeless working on it?
Comment 11 User image Ludovic Hirlimann [:Usul] 2012-01-25 00:54:43 PST
(In reply to :aceman from comment #10)
> Is this still relevant? Is timeless working on it?

No he ain't.
Comment 12 User image :aceman 2012-01-25 01:54:01 PST
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 User image neil@parkwaycc.co.uk 2012-02-20 04:46:38 PST
(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 User image :aceman 2012-02-20 04:56:42 PST
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 User image :aceman 2012-02-20 12:20:16 PST
Created attachment 598939 [details] [diff] [review]
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 User image neil@parkwaycc.co.uk 2012-03-11 11:26:05 PDT
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 User image :aceman 2012-03-17 07:10:54 PDT
Created attachment 606862 [details] [diff] [review]
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 User image neil@parkwaycc.co.uk 2012-03-17 08:55:34 PDT
(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 User image neil@parkwaycc.co.uk 2012-03-17 09:00:41 PDT
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 User image :aceman 2012-03-17 09:24:52 PDT
And why when it still compiles?
Comment 21 User image neil@parkwaycc.co.uk 2012-03-17 09:27:24 PDT
(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 User image :aceman 2012-03-17 09:33:35 PDT
Created attachment 606877 [details] [diff] [review]
patch v4

Ok.
Comment 23 User image David :Bienvenu 2012-03-17 10:57:06 PDT
(In reply to neil@parkwaycc.co.uk 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 User image Ryan VanderMeulen [:RyanVM] 2012-03-18 10:21:08 PDT
http://hg.mozilla.org/comm-central/rev/cd3cd74613f8

Note You need to log in before you can comment on or make changes to this bug.