1404577 - TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_cert_blocklist.js

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

First seen: Sat Sep 30, 2017, 1:48:57

TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_cert_blocklist.js | xpcshell return code: 0 [log…]
 
TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_cert_blocklist.js | - MIGKMQswCQYDVQQGEwJDSDEQMA4GA1UEChMHV0lTZUtleTEmMCQGA1UECxMdQ29weXJpZ2h0IChjKSAyMDA1IFdJU2VLZXkgU0ExFjAUBgNVBAsTDUludGVybmF0aW9uYWwxKTAnBgNVBAMTIFdJU2VLZXkgQ2VydGlmeUlEIEFkdmFuY2VkIEcxIENB should be an expected top-level line in revocations.txt - false == true [log…]

M-C last good: cd9c8c48e4b3ded47a776f757008f3dcf5
M-C first bad: 57f68296c350469d73d788eb3695a89894

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cd9c8c48e4b3ded47a776f757008f3dcf5&tochange=57f68296c350469d73d788eb3695a89894

Seems to be related to bug 1359428.

Mark, David, Mathieu, Robert, could you please advise me what I need to do so this test doesn't fail in the Thunderbird Xpcshell suite.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

It looks like the test is picking up the real revocations.txt file? (Although I don't understand how it could do that since I thought tests weren't allowed to connect to external resources, although maybe if we're serving it from the same data center it wouldn't consider it external?)
Maybe the test should set the "services.settings.server"/"extensions.blocklist.url" prefs earlier than it does?

Comment 2

[Jorg K (CEST = GMT+2)]

David, thanks for the reply, but this is really far removed from my area of expertise. Can the test be changed to pass for TB or should we disable it?

Comment 3

[Mathieu Leplatre [:leplatrem]]

This failing assertion does not tell us if the resulting revocations.txt is empty or contains other values. You can check around that, and I'll make my best to help you...

Basically, in Bug 1359428 we removed the pref `security.onecrl.via.amo` which is now always false.

The tests consists in:

- triggering a blocklist sync from a fake remote kinto server (which executes a specific blocklist callback that notifies the nsICertBlocklist of the changes)
- making sure the revocation.txt generated by nsICertBlocklist service matches the fake entries

I don't have any strong idea about what could be causing this :( 

Marc, any guess?

Comment 4

[Jorg K (CEST = GMT+2)]

I can run that test locally
mozilla/mach xpcshell-test security/manager/ssl/tests/unit/test_cert_blocklist.js
and I get:

 0:13.52 TEST_STATUS: Thread-1  FAIL MIGKMQswCQYDVQQGEwJDSDEQMA4GA1UEChMHV0lTZUtleTEmMCQGA1UECxMdQ29weXJpZ2h0IChjKSAyMDA1IFdJU2VLZXkgU0ExFjAUBgNVBAsTDUludGVybmF0aW9uYWwxKTAnBgNVBAMTIFdJU2VLZXkgQ2VydGlmeUlEIEFkdmFuY2VkIEcxIENB should be an expected top-level line in revocations.txt - false == true

and later on:

 0:14.30 PROCESS_OUTPUT: Thread-1 (pid:5572) "JavaScript strict warning: resource://testing-common/httpd.js, line 800: ReferenceError: reference to undefined property "_stopCallback""
 0:14.30 PROCESS_OUTPUT: Thread-1 (pid:5572) "!!! error running onStopped callback: TypeError: callback is not a function"
 0:13.55 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "_stopCallback"" {file: "resource://testing-common/httpd.js" line: 800}]"

Since this is the first time I've run this test manually, I assume this file:
 Directory of C:\Users\jorgk\AppData\Local\Temp\firefox\xpcshellprofile
03/10/2017  19:31            20,466 revocations.txt
(Windows, European time format) originates from the run I've just done.

I'll attach it in the next comment.

Looking at the message:
MIGKMQswCQYDVQQGEwJDSDEQMA4GA1UEChMHV0lTZUtleTEmMCQGA1UECxMdQ29weXJpZ2h0IChjKSAyMDA1IFdJU2VLZXkgU0ExFjAUBgNVBAsTDUludGVybmF0aW9uYWwxKTAnBgNVBAMTIFdJU2VLZXkgQ2VydGlmeUlEIEFkdmFuY2VkIEcxIENB should be an expected top-level line in revocations.txt

and at the file, I see:
MIGKMQswCQYDVQQGEwJDSDEQMA4GA1UEChMHV0lTZUtleTEmMCQGA1UECxMdQ29weXJpZ2h0IChjKSAyMDA1IFdJU2VLZXkgU0ExFjAUBgNVBAsTDUludGVybmF0aW9uYWwxKTAnBgNVBAMTIFdJU2VLZXkgQ2VydGlmeUlEIEFkdmFuY2VkIEcxIENB

as the fourth line of the file.

Looking at the test, 'expected' doesn't appear to be set up with that line, so that's why the test fails.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: revocations.txt

Comment 6

[Mark Goodwin [:mgoodwin]]

Looking at the revocations.txt file you've provided (thanks for that), we appear to have actual blocklist values (i.e. loaded from services/blocklists/certificates.json), plus some of the values that are added by the test. My recollection is that these aren't typically loaded when we run this test (for Fx, at least).

Is there some difference between how this data ends up in the profile data for thunderbird and FX?

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Mark Goodwin [:mgoodwin] from comment #6)
> Is there some difference between how this data ends up in the profile data
> for thunderbird and FX?
Apparently so ;-)

What determines how this data is loaded and how can I debug that? Otherwise the only option I see is to switch off that check for TB.

Comment 8

[Mark Goodwin [:mgoodwin]]

Mat, is there some magic that stops kinto loading initial data for collections in test profiles for Fx?

Comment 9

[Jorg K (CEST = GMT+2)]

While trying to follow comment #1 (quote):
> Maybe the test should set the "services.settings.server"/"extensions.blocklist.url"
> prefs earlier than it does?
I ran the test locally and noticed that things were going wrong before the test failure:

 0:09.25 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsICertBlocklist.revokeCertByIssuerAndSerial]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://services-common/blocklist-clients.js :: updateCertBlocklist :: line 358"  data: no]"]
updateCertBlocklist@resource://services-common/blocklist-clients.js:358:9
async*maybeSync/<@resource://services-common/blocklist-clients.js:310:17
async*openCollection@resource://services-common/blocklist-clients.js:140:20
async*maybeSync@resource://services-common/blocklist-clients.js:233:20
async*this.checkVersions@resource://services-common/blocklist-updater.js:164:15
async*notify@file:///c:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/components/nsBlocklistService.js:638:7
fetch_blocklist/<@c:/mozilla-source/comm-central/obj-i686-pc-mingw32/_tests/xpcshell/security/manager/ssl/tests/unit/test_cert_blocklist.js:225:5
Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:390:5
fetch_blocklist@c:/mozilla-source/comm-central/obj-i686-pc-mingw32/_tests/xpcshell/security/manager/ssl/tests/unit/test_cert_blocklist.js:215:10
run_test/<@c:/mozilla-source/comm-central/obj-i686-pc-mingw32/_tests/xpcshell/security/manager/ssl/tests/unit/test_cert_blocklist.js:357:11
TaskImpl_run@resource://gre/modules/Task.jsm:331:42
TaskImpl@resource://gre/modules/Task.jsm:280:3
asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
_run_next_test@c:\\mozilla-source\\comm-central\\mozilla\\testing\\xpcshell\\head.js:1488:9
run@c:\\mozilla-source\\comm-central\\mozilla\\testing\\xpcshell\\head.js:701:9
_do_main@c:\\mozilla-source\\comm-central\\mozilla\\testing\\xpcshell\\head.js:221:3
_execute_test@c:\\mozilla-source\\comm-central\\mozilla\\testing\\xpcshell\\head.js:544:5
@-e:1:1
"

That repeats three times. Perhaps that gives a clue.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

What's the value that's causing the error? (see https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/CertBlocklist.cpp#309 - it looks like setting the environment variable "MOZ_LOG=CertBlock:5" should say what it is).

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1404577.txt

With that logging it got quite wordy. I also added print statements into the function. If you look for "failed on", you see:
"=== failed on |MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=| |oops! more nonsense.|"
and three more.

But that's right in the middle and appears unrelated to the
0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsICertBlocklist.revokeCertByIssuerAndSerial]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://services-common/blocklist-clients.js :: updateCertBlocklist :: line 358"  data: no]"]

Or maybe it's related since there are four "failed on" and also four JS errors. And the whole thing is async, right?

Full log enclosed.

P.S.: Thanks for the help.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1404577-disable-part-of-test_cert_blocklist.patch (v1)

Looks like the

LOG: Thread-1 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsICertBlocklist.revokeCertByIssuerAndSerial]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://services-common/blocklist-clients.js :: updateCertBlocklist :: line 358"  data: no]"]

are expected since they originate from
"=== failed on |MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=| |oops! more nonsense.|"
and friends.

We're not into the third week of this test busting every single Thunderbird Xpcshell test run which makes sheriffing harder than it should be.

In this patch I'm suggesting to just switch off the part that fails since sadly I have received very little support to get to the bottom of the problem.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8918986 [details] [diff] [review]
1404577-disable-part-of-test_cert_blocklist.patch (v1)

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

If we have to, I'd rather just disable the test on thunderbird. I've been looking into this, and I think the unexpected entries are coming from objdir/dist/bin/defaults/blocklists/certificates.json, which exists when running on comm-central but not mozilla-central. I don't know why that is, though - perhaps Mark does.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Ah - it does exist on mozilla-central, but as objdir/dist/bin/browser/defaults/blocklists/certificates.json (note the extra "browser" directory). I believe this is a result of these lines:

if CONFIG['MOZ_BUILD_APP'] == 'browser':
    DIST_SUBDIR = 'browser'

in https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/services/blocklists/moz.build#17

Removing the conditional and recompiling (and removing the objdir/dist/bin/defaults/blocklists/certificates.json file first, just in case) results in this test succeeding.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8918986 [details] [diff] [review]
1404577-disable-part-of-test_cert_blocklist.patch (v1)

This patch wasn't really meant seriously, more to express the fact the ongoing failure is rather, well, how to say it nicely, ... unfortunate :-(

(In reply to David Keeler [:keeler] (use needinfo?) from comment #14)
> Removing the conditional and recompiling (and removing the
> objdir/dist/bin/defaults/blocklists/certificates.json file first, just in
> case) results in this test succeeding.
Wow, \o/, thanks! Could you do a patch for this?

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1404577-blocklist-test.patch

Try run with the enclosed patch based on David's idea/observation:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=76c27356e450037e226ee2542cdce03b18e62b1a

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Jorg K (GMT+2) from comment #15)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #14)
> > Removing the conditional and recompiling (and removing the
> > objdir/dist/bin/defaults/blocklists/certificates.json file first, just in
> > case) results in this test succeeding.
> Wow, \o/, thanks! Could you do a patch for this?

Looks like you beat me to it. In any case, I don't really know the effect of doing this - we should get some input from kinto folks.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1404577-blocklist-test.patch (v2)

The previous patch didn't work, but this one does. The JSON file gets put into
  dist\bin\mail\defaults\blocklists\certificates.json
and the test passes.

Here's another try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1bf7d8faf18d2dd22b48d7b437835d46f66fb6dc

David, please pass on the review as you deem fit. We're into the third week of having our xpcshell runs busted due to this, so quick action would be appreciated.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1404577-blocklists-packaging.patch (v1)

This also needs a C-C change. New try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6d02df0f32a4a472c3cf5c06aa925ab72474b4b6

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8920310 [details] [diff] [review]
1404577-blocklist-test.patch (v2)

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

Redirecting to original author.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1404577-blocklists-packaging.patch (v2)

Again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=039e0620dd10e22b82b889d86b596d4d8792a399

Or maybe Tom has a better idea.

Comment 22

[Tom Prince [:tomprince]]

This looks reasonable. I don't understand how the blocklist code works, or what the implications of putting the files in a different location (particularly as this is security sensitive code).

The m-c patch will need to be ajusted to take into account building c-c with m-c as topdir: MOZ_BUILD_APP will be `comm/mail` in that case. (so maybe `CONFIG["MOZ_BUILD_APP"] in ("../mail", "comm/mail")`?).

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1404577-blocklist-test.patch (v3)

OK, taking Tom's suggestions on board.

Comment 24

[Tom Prince [:tomprince]]

I was talking with people in #build and I think this is the wrong approach. firefox has two omni.ja bundles, on in the root and one in browser/. They can be accessed as resource://gre/ and resource://app/ respectively. Thunderbird, on the other hand, only has one. I think this change may "work" because the loading code can no longer find the bundled blocklist, but this break the actual desired behavior while making the test pass.

Comment 25

[Jorg K (CEST = GMT+2)]

I'd say it doesn't matter where the files are put as long as they are packaged. This time my try
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=039e0620dd10e22b82b889d86b596d4d8792a399
at least built, the Xpcshell test hasn't started yet.

I'm happy for someone else to take the bug and fix it. I've tried for three(!!) weeks so far to get a bustage fixed which affects every single xpcshell test we run on all platforms. So far I've received little support apart from David's who observed that the test fails if the file is placed into
  dist\bin\defaults\blocklists\certificates.json
since FF puts it into
  dist\bin\browser\defaults\blocklists\certificates.json
so I'm trying to imitate that and put it into
  dist\bin\mail\defaults\blocklists\certificates.json.

I don't understand why the test suddenly started failing after https://hg.mozilla.org/mozilla-central/rev/a889835e4003 which didn't change any make/build files.

I also don't understand why the test tries to pick up a file that in the case of FF isn't there. Instead of moving the file out of the way, IMHO it would be preferable not to go looking for it in the first place.

Comment 26

[Tom Prince [:tomprince]]

(In reply to Jorg K (GMT+2) from comment #25)
> I'd say it doesn't matter where the files are put as long as they are
> packaged.

Of course it matters. Unless files are where the code expects them to be, they can't be accessed by that code. And if they can't be accessed, they might as well not be packaed.

> I don't understand why the test suddenly started failing after
> https://hg.mozilla.org/mozilla-central/rev/a889835e4003 which didn't change
> any make/build files.

Looking at that commit, it changes that test file. Before that commit, the test was using a deprecated method of updating the blocklist that had been disabled for sometime. That commit got rid of the old method of updating the blocklist and adjusted the test to use the new method.
 
> I also don't understand why the test tries to pick up a file that in the
> case of FF isn't there. Instead of moving the file out of the way, IMHO it
> would be preferable not to go looking for it in the first place.

It turns out this hinges on a difference between firefox vs. xpcshell and thunderbird:
- In firefox, dist/bin/browser/ is available as resource://app/
- In xpcshell and firefox, dist/bin/ is available as resource://app/
So, in a firefox build, resource:://app/defaults/blocklists/certificates.json exists when running the app, but doesn't in tests. Whereas in thunderbird builds, it exists in both the app and the tests.

It looks like there was some[1][2] attempt to avoid loading the blocklist dump, but since it didn't cause the firefox tests to fail, it never ended up being implemented. (Though there was a comment[3] asking if that was leftover code)

[1] https://hg.mozilla.org/mozilla-central/rev/a889835e4003#l5.12
[2] https://hg.mozilla.org/mozilla-central/rev/a889835e4003#l3.254
[3] https://reviewboard.mozilla.org/r/176422/#review181706

I think the correct solution is to honor that preference.

Comment 27

[Tom Prince [:tomprince]]

Attachment: Bug 1404577: Actually disable loading blocklist dump in test_cert_blocklist.js.

The dump file isn't in the expected location in firefox tests, but is in
thunderbird tests, so the preference to disable loading wasn't originally
implemented.

Review commit: https://reviewboard.mozilla.org/r/191442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191442/

Comment 28

[Tom Prince [:tomprince]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=26262374b030671b9d13bbdb65beea54232d7f98
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82459dcbf6239298c6fafb329897f329dce267fe

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8920363 [details] [diff] [review]
1404577-blocklists-packaging.patch (v2)

Thanks Tom!

Comment 30

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8920456 [details]
Bug 1404577: Actually disable loading blocklist dump in test_cert_blocklist.js.

https://reviewboard.mozilla.org/r/191442/#review196704

Comment 31

[Jorg K (CEST = GMT+2)]

Ryan, could you please land this.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b2f0f688fe3
Actually disable loading blocklist dump in test_cert_blocklist.js. r=leplatrem

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0b2f0f688fe3