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 timeless 2003-06-27 13:21:41 PDT
 
Comment 1 timeless 2003-06-27 13:37:39 PDT
Created attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...
Comment 2 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 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 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 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 timeless 2003-06-30 17:04:13 PDT
Created attachment 126792 [details] [diff] [review]
thanks neil
Comment 7 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 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 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 :aceman 2012-01-23 08:36:15 PST
Is this still relevant? Is timeless working on it?
Comment 11 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 :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 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 :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 :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 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 :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 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 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 :aceman 2012-03-17 09:24:52 PDT
And why when it still compiles?
Comment 21 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 :aceman 2012-03-17 09:33:35 PDT
Created attachment 606877 [details] [diff] [review]
patch v4

Ok.
Comment 23 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 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.