1084481 - Build error: 'NS_NewInputStreamChannel' : no overloaded function takes 4 arguments

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Stefan Sitter [:ssitter]]

I got the following errors during build:

> mailnews/compose/src/nsMsgAttachmentHandler.cpp(556) : error C2661: 'NS_NewInputStreamChannel' : no overloaded function takes 3 arguments
> mailnews/addrbook/src/nsAddbookProtocolHandler.cpp(100) : error C2661: 'NS_NewInputStreamChannel' : no overloaded function takes 4 arguments
> mailnews/addrbook/src/nsAddbookProtocolHandler.cpp(145) : error C2661: 'NS_NewInputStreamChannel' : no overloaded function takes 4 arguments

The method signature was changed with Bug 1063197 / https://hg.mozilla.org/mozilla-central/rev/7e905781c715

Places that might need update: https://mxr.mozilla.org/comm-central/search?string=NS_NewInputStreamChannel&find=mail

Comment 2

[[e-mail]]

Attachment: Possible patch

This compiles but is untested. Some of the callers didn't actually pass in an input stream, so I switched those to NS_NewChannel instead.

Comment 3

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8507381 [details] [diff] [review]
Possible patch

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

I wish you would have spoken up on #maildev while you were working on this, I was working in parallel...

::: mailnews/addrbook/src/nsAddbookProtocolHandler.cpp
@@ +98,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIPrincipal> nullPrincipal =
> +    do_CreateInstance("@mozilla/nullprincipal;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_SUCCESS() is deprecated in favour of

if (NS_FAILED(rv)) {
  return rv;
}

(or, if you want a warning, )
if (NS_WARN_IF(NS_FAILED(rv)) {
   return rv;
}

https://groups.google.com/forum/#!msg/mozilla.dev.platform/1clMLuuhtWQ/8MxLDZN28Y4J

(though the code that landed on M-C didn't follow this advice...)

::: mailnews/base/src/nsMessenger.cpp
@@ +1044,5 @@
>          goto done;
>  
> +      nsCOMPtr<nsIPrincipal> nullPrincipal =
> +        do_CreateInstance("@mozilla/nullprincipal;1", &rv);
> +      NS_ASSERTION(NS_SUCCEEDED(rv), "CreateInstance failed");

I'm nervous about using the null principal here; we may want the principal from the message window to make sure we do the right checks on anything that gets loaded as we try to process the HTML.

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ +560,5 @@
> +        do_CreateInstance("@mozilla/nullprincipal;1", &rv);
> +      if (NS_FAILED(rv))
> +        goto done;
> +
> +      rv = NS_NewChannel(getter_AddRefs(m_converter_channel), aURL, nullPrincipal, nsILoadInfo::SEC_NORMAL, nsIContentPolicy::TYPE_OTHER);

Trying to convince myself that the null principal is OK here - if we're only loading MIME attachments from our own messages, yes, but if there's any chance this URL goes to the network we'll need a real principal.

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +1603,2 @@
>    nsCOMPtr<nsIChannel> channel;
> +  rv = NS_NewChannel(getter_AddRefs(channel), url, nullPrincipal, nsILoadInfo::SEC_NORMAL, nsIContentPolicy::TYPE_OTHER);

Again, if pulling the MIME message in here could cause us to dereference any HTTP-ish links, we probably want a real principal.

::: mailnews/compose/src/nsSmtpService.cpp
@@ +335,5 @@
>    pipeOut->Close();
>  
> +  nsCOMPtr<nsIPrincipal> nullPrincipal =
> +    do_CreateInstance("@mozilla/nullprincipal;1", &rv);
> +  if (NS_FAILED(rv)) 

White space (and NS_WARN_IF())

Comment 4

[ISHIKAWA, Chiaki]

I tested the patch:

I think all the references to @mozilla/nullprincipal;1 ought to be
@mozilla.org/nullprincipal;1

but I still get the error in NS_NewChannel call in
mailnews/compose/src/nsMsgComposeService.cpp
below: reindented and I have .org in the CreateInstance call.
After the addition of .org do_CreateInstance now works.
But the call to NS_NewChannel() fails with
[4423] ###!!! ASSERTION: NS_NewChannel failed.: 'NS_SUCCEEDED(rv)', file /REF-COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgComposeService.cpp, line 1611


  nsCOMPtr<nsIPrincipal> nullPrincipal =
    do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);
  NS_ASSERTION(NS_SUCCEEDED(rv), "CreateInstance of nullprincipal failed");
  if (NS_FAILED(rv))
    return rv;

  nsCOMPtr<nsIChannel> channel;
  rv = NS_NewChannel(getter_AddRefs(channel),
                     url,
                     nullPrincipal,
                     nsILoadInfo::SEC_NORMAL,
                     nsIContentPolicy::TYPE_OTHER);
  NS_ASSERTION(NS_SUCCEEDED(rv), "NS_NewChannel failed.");
  if (NS_FAILED(rv))
    return rv;

Oh, I just noticed that there is another failure that precedes this error in the log.

[4423] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B0012: file ../../../dist/include/nsNetUtil.h, line 218
[4423] ###!!! ASSERTION: NS_NewChannel failed.: 'NS_SUCCEEDED(rv)', file /REF-COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgComposeService.cpp, line 1611

The error in nsNetUtilh is from line 218.

  200	inline nsresult
  201	NS_NewChannelInternal(nsIChannel**           outChannel,
  202	                      nsIURI*                aUri,
  203	                      nsILoadInfo*           aLoadInfo,
  204	                      nsILoadGroup*          aLoadGroup = nullptr,
  205	                      nsIInterfaceRequestor* aCallbacks = nullptr,
  206	                      nsLoadFlags            aLoadFlags = nsIRequest::LOAD_NORMAL,
  207	                      nsIIOService*          aIoService = nullptr)
  208	{
  209	  NS_ASSERTION(aLoadInfo, "Can not create channel without aLoadInfo!");
  210	  NS_ENSURE_ARG_POINTER(outChannel);
  211
  212	  nsCOMPtr<nsIIOService> grip;
  213	  nsresult rv = net_EnsureIOService(&aIoService, grip);
  214	  NS_ENSURE_SUCCESS(rv, rv);
  215
  216	  nsCOMPtr<nsIChannel> channel;
  217	  rv = aIoService->NewChannelFromURI(aUri, getter_AddRefs(channel));
* 218	  NS_ENSURE_SUCCESS(rv, rv);
  219
  220	  if (aLoadGroup) {

Hmm... I wonder why it fails there. The arguments ought to be fine.

Oh, in your patch

   nsCOMPtr<nsIChannel> channel;
-  rv = NS_NewInputStreamChannel(getter_AddRefs(channel), url, nullptr);
+  rv = NS_NewChannel(getter_AddRefs(channel), url, nullPrincipal, nsILoadInfo::SEC_NORMAL, nsIContentPolicy::TYPE_OTHER);
   NS_ENSURE_SUCCESS(rv, rv);

Is the change to NS_NewChannel () from NS_NewInputStreamChannel intentional?

There is NS_NewInputStreamChannel() defined in
mozilla/netwerk/base/public/nsNetUtil.h
and that takes principal, loadinfo and friends, also

TIA

Comment 5

[ISHIKAWA, Chiaki]

Attachment: Possible patch modified (locally tested) See comment.

Hi,

After a few trials and errors, the uploaded modified patch
produced no errors on its own (I ran |make mozmill|
and xpcshell-tests and they did not seem to generate errors [added glaring new errors, that is.]

But, the choice of principal et al needs careful attention.

Hope this helps.

TIA

Comment 6

[Stefan [:stefanh]]

Comment on attachment 8507487 [details] [diff] [review]
Possible patch modified (locally tested) See comment.

jftr, I tried building with this patch and it worked fine (debug, mac os x).

Comment 7

[Joshua Cranmer [:jcranmer]]

Right, so before I start reviewing this patch, let me explain the background as I understand it:

Principals are primarily used to indicate cross-origin stuff, for CORS or security checks. The bottom of principals is a null principal (can't do anything), and the top is the system principal (can do anything).

The goal, AIUI, is that principals get attached during channel creation, and that CORS happens internally to necko/protocol handlers instead of at consumer site. Since using null principals means we can't load anything, it may be dangerous to use null principals to request a channel. But that doesn't happen yet, and it's not fully clear what the future implementation strategy will look like.

So, to summarize, null principals are sufficient for now. This does raise interesting questions of what policy for "cross-origin" should be for messages, and I will admit that I don't fully know. I would raise it with the HTML-in-email W3C community group, but I doubt that there are sufficient people in there to discuss this kind of question meaningfully.

Comment 8

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8507487 [details] [diff] [review]
Possible patch modified (locally tested) See comment.

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

Since this is the patch I tested, this is the patch I reviewed.

Comment 9

[Joshua Cranmer [:jcranmer]]

(In reply to :Irving Reid from comment #3)
> NS_ENSURE_SUCCESS() is deprecated in favour of

There is, shall we say, strong disagreement on this point. Some people heavily dislike the NS_ENSURE_* macros because "they hide control flow" while other people object that said maxim doesn't apply because the control flow isn't relevant, and that current coding standards require effectively four lines of code per function call. People basically use or don't use the NS_ENSURE_SUCCESS largely on their own whim under the guise of "follow prevailing style."

I personally prefer to use these macros and will probably keep using them unless people actually remove the macros.

> I'm nervous about using the null principal here; we may want the principal
> from the message window to make sure we do the right checks on anything that
> gets loaded as we try to process the HTML.

I've looked over the principal usage and asked bz for some feedback about this from IRC. In short, all of the null principals here seem safe.

Comment 10

[Joshua Cranmer [:jcranmer]]

Since this is making the tree red, I fixed the whitespace issues locally (hence I didn't bother to note them) and landed them:
https://hg.mozilla.org/comm-central/rev/c3f9d6ad3086

Comment 11

[[e-mail]]

(In reply to ISHIKAWA, Chiaki from comment #4)
> I think all the references to @mozilla/nullprincipal;1 ought to be
> @mozilla.org/nullprincipal;1
Oops sorry.

> Is the change to NS_NewChannel () from NS_NewInputStreamChannel intentional?
I was wondering why we were calling NS_NewInputStreamChannel without a stream. Obviously creating the channel the "correct" way doesn't work for some reason.

Comment 12

[ISHIKAWA, Chiaki]

(In reply to [e-mail] from comment #11)
> (In reply to ISHIKAWA, Chiaki from comment #4)
> > I think all the references to @mozilla/nullprincipal;1 ought to be
> > @mozilla.org/nullprincipal;1
> Oops sorry.

No problem. I was looking at the failure of creation, and looking at the diff
and added, ".org", and TB went past this creation without no problem then.

> 
> > Is the change to NS_NewChannel () from NS_NewInputStreamChannel intentional?
> I was wondering why we were calling NS_NewInputStreamChannel without a
> stream. Obviously creating the channel the "correct" way doesn't work for
> some reason.

Well the world is full of mysteries, isn't it? (I am sorry I don't have time to look 
into the NS_NewInputStreamChannel issue. I can only scratch the surface. But as Comment 9 notes, future security model *MAY* affect the I/O inside TB, too. I don't like HTML mail for various reasons, but more so when I think about the security implications...)

Comment 13

[ISHIKAWA, Chiaki]

(In reply to Joshua Cranmer [:jcranmer] from comment #10)
> Since this is making the tree red, I fixed the whitespace issues locally
> (hence I didn't bother to note them) and landed them:
> https://hg.mozilla.org/comm-central/rev/c3f9d6ad3086

Thank you.
FYI,
To make C-C TB compile in a more or less useable manner,
I needed these patches.

Major ones first:

The patch in 
Bug 1084481 - Build error: 'NS_NewInputStreamChannel' : no overloaded function takes 4 arguments (this bugzilla entry)


Bug 1032767 - Removed setUpdateUrl function used [Error: listManager.setUpdateUrl is not a function]
    This has prevented to create phishing warden.

This needs to be followed by a patch in

Bug 1085382 - unable to create the phishing warden: [Exception... "Not enough arguments [nsIUrlListManager.registerTable]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: file:///REF-OBJ-DIR/objdir-tb3/dist/bin/components/nsPhishingProtec 

Next one caused a hung of a newly installed thunderbird. It cannot go past
system integration screen.
Bug 1085003 - JavaScript error: chrome://messenger/content/systemIntegrationDialog.xul, line 1: ReferenceError: gSystemIntegrationDialog is not defined
    
And more, |make mozmill| test do not proceed further without the patch mentioned in the
following.

Bug 1083793 - Make nsITreeBoxObject and nsIBoxObject scriptable again
    |make mozmill| test of TB requires certain XPCOM objcts to scriptable,
    and so restore scriptable again.

But then this would uncover the following new bug:

Bug 1085050 - Assertion failure: mTopRowIndex == mozilla::clamped(mTopRowIndex, 0, maxTopRowIndex), at .../layout/xul/tree/nsTreeBodyFrame.cpp 

You need the fix in the above bug (disable over-eager MOZ_ASSERT()).

Medium:

Bug 733535 - fix 'cards[i] is null' by making GetSelectedAbCards ...
    This needs to be applied to suppress some run-time errors.

The following needs to be added to shutup warning/errors for
seeming important three variables.
 Bug 1085014 - JavaScript strict check: mail/base/content/mail3PaneWindowCommands.js needs three variable declarations

There are a few more niceties, but they can wait until C-C TB is in
more or less a shap that can run |make mozmill|.

With the above patches, |make mozmill| on local PC
ran slightly over 1000 tests successfully and 8 failures.

I needed to figure out the above just to make sure my humble desire of
changing context menu layout ( Bug 837205 ) did not cause all the regressions

TB may pose too high a hurdle for ordinary users...

TIA

PS: not all the fixes mentioned are proposed as patch proposals yet, but
in hindsight, they are all necessary for C-C TB to be testable by |make mozmill|, |make xpcshell-tests|, etc.