814391 - Permanent Orange: test_cspreports.js hangs for Thunderbird after bug 802905 landed (buildbot.slave.commands.TimeoutError)

Status: RESOLVED FIXED

(MailNews Core :: Security, defect)

Description

[Mark Banner (:standard8)]

Since bug 802905 landed, we've been seeing permanent hangs running test_cspreports.js against Thunderbird builds.

If I ctrl-c the test running locally on a debug build, I get:

TEST-UNEXPECTED-FAIL | /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/content/base/test/unit/test_cspreports.js | test failed (with xpcshell return code: -2), see following log:
>>>>>>>
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /var/folders/3r/4lhjf61d4fqdxjl7q5_yzz300000gp/T/tmp4Ku8MG/runxpcshelltests_leaks.log

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-INFO | (xpcshell/head.js) | test 2 pending
Created test 0 : allow 'none'; report-uri http://localhost:9000/test0


TEST-INFO | (xpcshell/head.js) | test 3 pending
Created test 1 : allow 'none'; report-uri http://localhost:9000/test1


TEST-INFO | (xpcshell/head.js) | test 4 pending
Created test 2 : allow 'none'; report-uri http://localhost:9000/test2


TEST-INFO | (xpcshell/head.js) | test 4 finished

TEST-INFO | (xpcshell/head.js) | running event loop
WARNING: NS_ENSURE_SUCCESS(rv, nsresult::NS_ERROR_UNEXPECTED) failed with result 0x80004005: file /Users/moztest/comm/main/src/mozilla/extensions/cookie/nsPermissionManager.cpp, line 377
WARNING: NS_ENSURE_SUCCESS(rv, nullptr) failed with result 0x80004003: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 614
WARNING: NS_ENSURE_TRUE(aRequestingContext) failed: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 728
WARNING: NS_ENSURE_SUCCESS(rv, nsresult::NS_OK) failed with result 0x80004003: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 291
WARNING: NS_ENSURE_SUCCESS(rv, nullptr) failed with result 0x80004003: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 614
WARNING: NS_ENSURE_TRUE(aRequestingContext) failed: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 728
WARNING: NS_ENSURE_SUCCESS(rv, nsresult::NS_OK) failed with result 0x80004003: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 291
WARNING: NS_ENSURE_SUCCESS(rv, nullptr) failed with result 0x80004003: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 614
WARNING: NS_ENSURE_TRUE(aRequestingContext) failed: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 728
WARNING: NS_ENSURE_SUCCESS(rv, nsresult::NS_OK) failed with result 0x80004003: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 291
<<<<<<<

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

So, I think we're failing this because mailnews's nsMsgContentPolicy is blocking the CSP report type, which we're not supposed to do.

Testing this patch on try:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=787096cf6bba

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 684751 [details] [diff] [review]
Patch v1

So this appears to fix it... am I doing the right thing here?

Comment 3

[Tanvi Vyas[:tanvi]]

(In reply to Mike Conley (:mconley) from comment #2)
> Comment on attachment 684751 [details] [diff] [review]
> Patch v1
> 
> So this appears to fix it... am I doing the right thing here?

Hi Mark,

Bug 802905 changed the content type for CSP reports to be more specific (TYPE_CSP_REPORT instead of TYPE_OTHER).  Adding code to allow for TYPE_CSP_REPORT should hopefully do the trick.  However, I don't see the code where the code to allow TYPE_OTHER is: http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgContentPolicy.cpp#208
It looks like you are just checking for TYPE_DOCUMENT, so I'm not sure where CSP reports were allowed before your patch.

Comment 4

[Mark Banner (:standard8)]

(In reply to Tanvi Vyas from comment #3)
> Bug 802905 changed the content type for CSP reports to be more specific
> (TYPE_CSP_REPORT instead of TYPE_OTHER).  Adding code to allow for
> TYPE_CSP_REPORT should hopefully do the trick.  However, I don't see the
> code where the code to allow TYPE_OTHER is:
> http://mxr.mozilla.org/comm-central/source/mailnews/base/src/
> nsMsgContentPolicy.cpp#208
> It looks like you are just checking for TYPE_DOCUMENT, so I'm not sure where
> CSP reports were allowed before your patch.

From bug 802905 comment 0, it looks like the CSP reports have a null requesting location, we'd have therefore returned early just after the type document. As at that stage the default is to accept, we'd have been accepting these reports.

As the bug has now separated them out, and given a requesting location, I think that's why it is now failing, so Mike's patch is correct.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 684751 [details] [diff] [review]
Patch v1

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

Thanks Mike.

Comment 6

[Mark Banner (:standard8)]

Checked in: https://hg.mozilla.org/comm-central/rev/56b11777b6cc