654183 - Cleanup null out parameters in nsIMsgMessageService

Status: RESOLVED DUPLICATE of bug 430716

(MailNews Core :: Backend, defect)

Description

[Kent James (:rkent)]

As part of my efforts to make core mailnews interfaces more scriptable, remove references to null out parameters in nsIMsgMessageService. (XPCOM will cause a crash if a javascript-based component is used, that is expecting an out parameter, but a C++ function calls that method with a null for the out parameter).

Comment 1

[Kent James (:rkent)]

Attachment: Mostly eliminates out nsIURI

This attachment mostly removes the ability of message service callers to get a copy of the created nsIURI, which as far as I can tell is not used anywhere in core code. If this capability is important to maintain, then I could propose several options:

1) leave the interface as it is, and pass a dummy nsCOMPtr to each of the calls that currently give a nsnull value for the out parameter, or
2) overload the component that implements nsIUrlListener with a second interface, that would have a read-only parameter to get the nsIURI, or
3) do nothing, and force SkinkGlue to override each of these methods and insert the same dummy nsCOMPtrs that option 1 would do in core.

Currently, SkinkGlue does this override only for DisplayMessage as that seems to be the only method that I am overriding in my existing JavaScript new account types (See http://hg.mozilla.org/users/kent_caspia.com/skinkglue/file/fd799127dbb0/src/msqSgService.cpp#l106 to see what this entails). But if SkinkGlue were to have complete capability to allow override of any nsIMsgMessageService method, I would need to do this for all of the methods where a C++ caller is calling the method with a null out parameter.

Comment 2

[David :Bienvenu]

Comment on attachment 529544 [details] [diff] [review]
Mostly eliminates out nsIURI

my gut reaction is no; there are many times where being able to get that out uri has been very useful. I think making it be a return value instead of an out is a good thing to do.

Comment 3

[David :Bienvenu]

does making the nsIURI the return value in the idl help you?

Comment 4

[Kent James (:rkent)]

Making it a return value does not really solve the underlying problem, if the C++ callers just pass nsnull as the pointer for the return value.

The solution without changing the interface (keeping the capability) is to replace lines such as:

rv = messageService->DisplayMessage(fullMessageUri.get(), convertedListener, mMsgWindow, nsnull, nsnull, nsnull);

with:

nsCOMPtr<nsIURI> dummyNull;
rv = messageService->DisplayMessage(fullMessageUri.get(), convertedListener, mMsgWindow, nsnull, nsnull, getter_AddRefs(dummyNull));

Comment 5

[David :Bienvenu]

(In reply to comment #4)

> The solution without changing the interface (keeping the capability) is to
> replace lines such as:
> 
> rv = messageService->DisplayMessage(fullMessageUri.get(), convertedListener,
> mMsgWindow, nsnull, nsnull, nsnull);
> 
> with:
> 
> nsCOMPtr<nsIURI> dummyNull;
> rv = messageService->DisplayMessage(fullMessageUri.get(), convertedListener,
> mMsgWindow, nsnull, nsnull, getter_AddRefs(dummyNull));

sigh, well, that's not pretty, but I'd prefer that to losing the uri.

Comment 6

[Kent James (:rkent)]

The prettier solution that I proposed as option 2 was to create a new interface, say nsIUrlListener2, that adds:

attribute nsIURI url

and store the uri there. So in each of the routines that are currently returning aUrl, you would need to add code like:

nsCOMPtr<nsIUrlListener2> listener2(do_QueryInterface(aUrlListener));
if (listener2)
  listener2->SetUrl(....

This would add still more code in addition to all of the code in the current attachment, but probably should have been the correct original implementation for a rarely used out parameter. I would prefer my implementation in comment 4 though because it is less work for me.

I don't really have to have this at all, as I can do the overrides myself in SkinkGlue. The additional annoyance factor with doing this in SkinkGlue instead of in core is that I cannot use the standard forwarding macro NS_FORWARD_NSIMSGMESSAGESERVICE but instead must manually copy that and modify it each time the underlying interface changes (see for example http://hg.mozilla.org/users/kent_caspia.com/skinkglue/file/fd799127dbb0/src/msqSgService.h#l79 )

This is not just an issue with a skinkGlue approach though. I believe that an attempt to implement these interfaces directly using JavaScript components, such as jcranmer was trying to do, will fail for the same reasons, and he would not have the option to fallback to a C++ override like I can do with my C++-based SkinkGlue approach.

From my perspective, I don't mind continuing to do these overrides in SkinkGlue instead of accepting core patches. So I guess an important question is, is there a interest in changing the core code to make the core interfaces more script friendly?

Comment 7

[David :Bienvenu]

I'm fine with the implementation in #c4. I'd like to change the interfaces to return the nsURI's while we're at it, though I won't insist on it...

Comment 8

[David :Bienvenu]

Comment on attachment 529544 [details] [diff] [review]
Mostly eliminates out nsIURI

minusing because it can be handy to get the url created for operations.

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to David :Bienvenu from comment #8)
> Comment on attachment 529544 [details] [diff] [review]
> Mostly eliminates out nsIURI
> 
> minusing because it can be handy to get the url created for operations.

Kent, what's your thinking on future for this bug?

Comment 10

[Kent James (:rkent)]

I need to review Bienevenu's comments, but generally I still hope to do cleanup such as this that will eliminate a lot of the pain of my binary addons. I still don't think we should allow null output parameters like this that cause an immediate crash when used in JS.

Comment 11

[Kent James (:rkent)]

I will do this as part of the general integration of SkinkGlue into core.