1278782 - TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_authentication.js | xpcshell return code: 0

Status: RESOLVED FIXED

(MailNews Core :: Security, defect)

Description

[aleth [:aleth]]

ERROR Component returned failure code: 0x805e0006 [nsIChannel.asyncOpen2]
test_noauth@/Users/helvellyn/buildhg/cc1/obj-mail/_tests/xpcshell/netwerk/test/unit/test_authentication.js:331:3
run_test@/Users/helvellyn/buildhg/cc1/obj-mail/_tests/xpcshell/netwerk/test/unit/test_authentication.js:324:3

Reproducible locally.

Comment 1

[aleth [:aleth]]

I can't find any information on that failure code. Dragana, any ideas on what could be causing this?

Comment 2

[Dragana Damjanovic [:dragana]]

(In reply to aleth [:aleth] from comment #1)
> I can't find any information on that failure code. Dragana, any ideas on
> what could be causing this?

The error code is for NS_ERROR_CONTENT_BLOCKED.
Has this started after bug 1278641 has landed? It sounds like it since the change in that bug has to do with load types.

Comment 3

[aleth [:aleth]]

(In reply to Dragana Damjanovic [:dragana] from comment #2)
> Has this started after bug 1278641 has landed? It sounds like it since the
> change in that bug has to do with load types.

No, this was first observed (on comm-central) on Tuesday June 7.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: console output of local run

To run this locally, we run
mozilla/mach xpcshell-test netwerk/test/unit/test_authentication.js

Output enclosed. I'm not sure which of the various warnings and errors are relevant. Maybe the
ERROR Component returned failure code: 0x805e0006 [nsIChannel.asyncOpen2]
as per comment #0

Comment 5

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #3)
> (In reply to Dragana Damjanovic [:dragana] from comment #2)
> > Has this started after bug 1278641 has landed? It sounds like it since the
> > change in that bug has to do with load types.
> 
> No, this was first observed (on comm-central) on Tuesday June 7.

Backing out bug 1230462 fixes this.

Comment 6

[Jorg K (CEST = GMT+2)]

Could you give us some further advice on that why the test might be failing in the TB environment after bug 1230462 landed.

Comment 7

[Dragana Damjanovic [:dragana]]

You can check what value pref network.auth.subresource-http-auth-allow has, and set it to 2 (allow all) (I have fix part of that logic in bug 1230462)

It is reproducible locally, can you make a http log I could take a look at, probably that is going to say more.

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks.
network.auth.subresource-http-auth-allow is already set to 2 in all.js
https://dxr.mozilla.org/comm-central/source/mozilla/modules/libpref/init/all.js#1914
which we seem to include somehow. In a "normal" TB session that preference is set and I assume it would be set in an xpcshell test as well. Adding it again to our all-thunderbird.js didn't make the test pass.

I noticed that TB overrides some of the preferences included from all.js here:
https://dxr.mozilla.org/comm-central/source/mail/app/profile/all-thunderbird.js#340
but nothing with network.auth.* gets changed.

I'll see how I can do a http log.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: http log

I followed
https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging
and set
$ export NSPR_LOG_MODULES=timestamp,nsHttp:5,nsSocketTransport:5,nsStreamPump:5,nsHostResolver:5
$ export NSPR_LOG_FILE=log-jk.txt

Here is the file. If you want something else, please let me know.

Comment 10

[Jorg K (CEST = GMT+2)]

Gentle ping: Dragana, could you please look at the log I provided so we can move forward on this. Our tree would be a lot greener if we could solve this one test failure which occurs on all platforms.

Comment 11

[aleth [:aleth]]

Maybe the default content policy differs? Not sure offhand where that is set.

Comment 12

[Dragana Damjanovic [:dragana]]

I do not see anything in the log: 
2016-06-10 07:19:43.100000 UTC - [Main Thread]: V/nsHttp Creating HttpBaseChannel @b042000
2016-06-10 07:19:43.100000 UTC - [Main Thread]: V/nsHttp Creating nsHttpChannel [this=b042000]
2016-06-10 07:19:43.106000 UTC - [Main Thread]: V/nsHttp HttpBaseChannel::Init [this=b042000]
2016-06-10 07:19:43.106000 UTC - [Main Thread]: V/nsHttp host=localhost port=50340
2016-06-10 07:19:43.106000 UTC - [Main Thread]: V/nsHttp uri=http://localhost:50340/auth
2016-06-10 07:19:43.106000 UTC - [Main Thread]: V/nsHttp nsHttpChannel::Init [this=b042000]
2016-06-10 07:19:43.143000 UTC - [Main Thread]: V/nsHttp Destroying nsHttpChannel [this=b042000]
2016-06-10 07:19:43.143000 UTC - [Main Thread]: V/nsHttp Destroying HttpBaseChannel @b042000

and probably has to do with default content policies.

The policy is set in netwerk/base/NetUtil.jsm in function makeChan can you please check what it is and set the same thing in the new version of the test.

Comment 13

[Jorg K (CEST = GMT+2)]

I'm not sure what to look for.
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm
has the string "make" twice in a comment, maybe you mean:
newChannel: function NetUtil_newChannel(aWhatToLoad, aOriginCharset, aBaseURI)

There is also makeChan here:
https://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_authentication.js#288

I dumped out contentPolicyType here
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#403
and I get 1, which is not a surprise since it's called with
Components.interfaces.nsIContentPolicy.TYPE_OTHER (which is 1, right?)

Comment 14

[Dragana Damjanovic [:dragana]]

If you change pref:
prefs.setBoolPref("network.loadinfo.skip_type_assertion", true);

it should fix this if it is a policy issue.

Comment 15

[Jorg K (CEST = GMT+2)]

I'm not sure where I should place this statement since the test is a M-C test that we're running as part of the C-C test suite.

I added
  pref("network.loadinfo.skip_type_assertion", true);
to our all-thunderbird.js file and it made no difference, still seeing
ERROR Component returned failure code: 0x805e0006 [nsIChannel.asyncOpen2]

Maybe I should let Aleth have a go, his comment #11 suggests that he knows more about that than I.

Comment 16

[Dragana Damjanovic [:dragana]]

You can add:
var prefs = Cc["@mozilla.org/preferences-service;1"].
              getService(Ci.nsIPrefBranch);
pref("network.loadinfo.skip_type_assertion", true);

to the test it self. 

Probably it is the best to debug it and see which check is failing and returning that error. From the log probably it is failing in asyncopen already.

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Dragana Damjanovic [:dragana] from comment #16)
> You can add:
> var prefs = Cc["@mozilla.org/preferences-service;1"].
>               getService(Ci.nsIPrefBranch);
> to the test it self. 
This is already present:
https://dxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_authentication.js#8
I added
prefs.setBoolPref("network.loadinfo.skip_type_assertion", true);
as you had suggested in comment #14 and it made no difference:
ERROR Component returned failure code: 0x805e0006 [nsIChannel.asyncOpen2]

Comment 18

[aleth [:aleth]]

Yes, the failure is on NetUtil.jsm::ioService.newChannelFromURI2, called from the first test, test_noauth.

The interesting thing is that all the other tests pass if you comment out the first.

Comment 19

[aleth [:aleth]]

Even more interestingly, if you add a function test_noop() {} to the test list at the beginning, all the tests pass ;)

So there seems to be some initialization race condition.

Comment 20

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #19)
> Even more interestingly, if you add a function test_noop() {} to the test
> list at the beginning, all the tests pass ;)
> 
> So there seems to be some initialization race condition.

Bah, I missed how these tests were being run (not with the usual add_test pattern). If you comment out a test, it disables all the following ones, so things trivially pass. Sorry the confusion.

Comment 21

[aleth [:aleth]]

OK, a little more detailed investigation shows that the failure stack is

nsHttpChannel.cpp:5426 (nsHttpChannel::AsyncOpen2)
nsContentSecurityManager.cpp:424 (nsContentSecurityManager::doContentSecurityCheck)

and the failing content policy is mailnews/base/src/nsMsgContentPolicy.cpp, which makes it clear why this test failure is c-c specific.

The failures in that file are, starting in nsMsgContentPolicy::ShouldLoad,

WARNING: NS_ENSURE_SUCCESS(rv, nullptr) failed with result 0x80070057: file mailnews/base/src/nsMsgContentPolicy.cpp, line 660
  nsMsgContentPolicy::GetMsgComposeForContext : fair enough, we're not in a compose window.
WARNING: NS_ENSURE_SUCCESS(rv, NS_OK) failed with result 0x80070057: file mailnews/base/src/nsMsgContentPolicy.cpp, line 281
WARNING: NS_ENSURE_TRUE(aRequestingContext) failed: file mailnews/base/src/nsMsgContentPolicy.cpp, line 786

nsMsgContentPolicy::GetOriginatingURIForContext fails out of the gate because aRequestingContext is not provided here.

Looks like this code long predates the aRequestPrincipal argument, which is not made use of.

Comment 22

[aleth [:aleth]]

Attachment: Use aRequestPrincipal as a fallback when available in the MsgContentPolicy

This uses the aRequestPrincipal, but only cautiously when there is no alternative, as mail code (unfortunately) seems to be doing without it, and instead implementing something similar by hand. I'm not too familiar with principals, nor with mailnews C++ style, so please check ;)

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 8763304 [details] [diff] [review]
Use aRequestPrincipal as a fallback when available in the MsgContentPolicy

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

Yeah I think this looks correct. r=mkmelin with the below addressed

::: mailnews/base/src/nsMsgContentPolicy.cpp
@@ +263,5 @@
>      return NS_OK;
>    }
>  
>    // Extract the windowtype to handle compose windows separately from mail
> +  if (!!aRequestingContext) {

remove the !!

and the style here seems to be to have { on a new line

@@ +277,5 @@
>  
>    // Find out the URI that originally initiated the set of requests for this
>    // context.
>    nsCOMPtr<nsIURI> originatorLocation;
> +  if (!aRequestingContext && !!aRequestPrincipal) {

remove the !!, move { to new line

@@ +279,5 @@
>    // context.
>    nsCOMPtr<nsIURI> originatorLocation;
> +  if (!aRequestingContext && !!aRequestPrincipal) {
> +    // Can get the URI directly from the principal.
> +    aRequestPrincipal->GetURI(getter_AddRefs(originatorLocation));

I think this should be 

 rv = ......
 NS_ENSURE_SUCCESS(rv, NS_OK);

@@ +281,5 @@
> +  if (!aRequestingContext && !!aRequestPrincipal) {
> +    // Can get the URI directly from the principal.
> +    aRequestPrincipal->GetURI(getter_AddRefs(originatorLocation));
> +  }
> +  else {

{ on new line

Comment 24

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/d0257f4d94ce63b6b3820320374b30585cbf1e16
Bug 1278782 - Use aRequestPrincipal as a fallback when available in the MsgContentPolicy. r=mkmelin

Comment 25

[aleth [:aleth]]

(In reply to Magnus Melin from comment #23)
> and the style here seems to be to have { on a new line

ah yeah, looks like someone didn't like K&R ;)

> > +    aRequestPrincipal->GetURI(getter_AddRefs(originatorLocation));
> 
> I think this should be 
> 
>  rv = ......
>  NS_ENSURE_SUCCESS(rv, NS_OK);

Good catch, thanks.