TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_cert_blocklist.js | - revocations.txt should be as expected

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
Security: PSM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Fallen, Assigned: Magnus Melin)

Tracking

({intermittent-failure})

Trunk
mozilla45
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed, firefox-esr38 fixed, b2g-v2.5 fixed, thunderbird_esr38 affected)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Might be fallout from bug 1184381
(Reporter)

Updated

2 years ago
Blocks: 1184381

Updated

2 years ago
Keywords: intermittent-failure
(Reporter)

Comment 1

2 years ago
Created attachment 8658484 [details]
failing test log with debug messages

I've been able to reproduce this now. It seems items from blocklist.xml are included in the persisted revocations.txt, which the test does not expect. We've just recently updated blocklist.xml to include the certItems, after above mentioned bug:

http://mxr.mozilla.org/comm-central/source/mozilla/browser/app/blocklist.xml#3249

I'm attaching the failing test log with debugging messages for convenience. It also contains a diff between the actual/expected lines at the end, which I have re-sorted to better show the added lines.

Mark, since you wrote the test, do you think you could help us out in figuring out why the blocklist.xml entries are being (wrongly) added?
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Flags: needinfo?(mgoodwin)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(In reply to Philipp Kewisch [:Fallen] from comment #1)
> Mark, since you wrote the test, do you think you could help us out in
> figuring out why the blocklist.xml entries are being (wrongly) added?

Certainly. I'll take a look now.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)

Comment 35

2 years ago
6 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* comm-central: 6

Platform breakdown:
* windowsxp: 2
* windows7-32: 2
* linux64: 2

For more details, see:
http://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1200567&startday=2015-09-28&endday=2015-10-04&tree=all

Comment 36

2 years ago
9 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* comm-beta: 9

Platform breakdown:
* windowsxp: 2
* osx-10-10: 2
* linux64: 2
* windows7-32: 1
* osx-10-6: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1200567&startday=2015-10-05&endday=2015-10-11&tree=all

Comment 37

2 years ago
22 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 22

Platform breakdown:
* android-4-3-armv7-api11: 15
* windowsxp: 2
* windows8-64: 2
* osx-10-10: 2
* android-4-2-x86: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1200567&startday=2015-10-20&endday=2015-10-20&tree=all

Comment 38

2 years ago
22 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 22

Platform breakdown:
* android-4-3-armv7-api11: 15
* windowsxp: 2
* windows8-64: 2
* osx-10-10: 2
* android-4-2-x86: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1200567&startday=2015-10-19&endday=2015-10-25&tree=all

Updated

2 years ago
status-thunderbird41: --- → affected
status-thunderbird42: --- → affected
status-thunderbird43: --- → affected
status-thunderbird44: --- → affected
status-thunderbird45: --- → affected
status-thunderbird_esr38: --- → affected

Comment 39

2 years ago
(In reply to Mark Goodwin [:mgoodwin] from comment #9)
> (In reply to Philipp Kewisch [:Fallen] from comment #1)
> > Mark, since you wrote the test, do you think you could help us out in
> > figuring out why the blocklist.xml entries are being (wrongly) added?
> 
> Certainly. I'll take a look now.

Gentle ping ;)

Updated

2 years ago
Attachment #8658484 - Flags: feedback?(mgoodwin)

Comment 40

2 years ago
David and Dave, I can see you've done some reviewing in the area. Perhaps you can give us a hint since Mark doesn't seem to be available.
Flags: needinfo?(dtownsend)
Flags: needinfo?(dkeeler)
I don't really know anything about this.
Flags: needinfo?(dtownsend)
Sounds like a couple of things might be going on:

Can the comm-central test infrastructure reach external URLs? I'm fairly sure we prevent this in general for mozilla-central, but maybe the same thing never happened for comm-central.

Is the pref "extensions.blocklist.url" set to the right thing? (Presumably a non-existent URL or a stub endpoint that just returns an empty blocklist.) Either the entire test infrastructure or any specific test that deals with the blocklist needs to set this. Since there appears to be a difference between mozilla-central and comm-central, my guess is the comm-central test setup isn't correct.
Flags: needinfo?(dkeeler)
Apologies, I've been out for a few days.

I'm trying to work out how to run these tests locally; are there docs on how to do this?

I tried:
$./mozilla/mach xpcshell-test mozilla/security/manager/ssl/tests/unit/test_cert_blocklist.js

To no avail
Flags: needinfo?(mgoodwin)

Comment 44

2 years ago
./mozilla/mach xpcshell-test security/manager/ssl/tests/unit/test_cert_blocklist.js

Thanks for looking at it!!

I was going to see whether I can follow up on David's hints, but knowing nothing about all this, I let you go first ;-)

Comment 45

2 years ago
Oops, English derailed: I let you have a go first. Thanks again.
(In reply to Jorg K (GMT+1) from comment #44)
> ./mozilla/mach xpcshell-test
> security/manager/ssl/tests/unit/test_cert_blocklist.js

Jorg, are there docs for running the platform xpcshell tests with comm-central?
Flags: needinfo?(mozilla)

Comment 47

2 years ago
Sorry, not that I'd know. Someone showed me how to run some C-C tests and that's all I've been doing. I didn't even realise I didn't type "mozilla" to make the path complete.

Hopefully Aleth or Philipp know.
Flags: needinfo?(philipp)
Flags: needinfo?(mozilla)
Flags: needinfo?(aleth)
Ah, don't worry. I sorted it: I don't need the leading mozilla/ from the test path.

But already I'm getting a hint of the problem; the test works in isolation which likely means that CertBlocklist is initialized before we get to the test.

/me will turn on some debug and look for clues
Flags: needinfo?(philipp)
Flags: needinfo?(aleth)

Comment 49

2 years ago
(In reply to Mark Goodwin [:mgoodwin] from comment #48)
> Ah, don't worry. I sorted it: I don't need the leading mozilla/ from the
> test path.
Umm, that's what I wrote in comment #44 ;-)

> But already I'm getting a hint of the problem; the test works in isolation
> which likely means that CertBlocklist is initialized before we get to the
> test.
Well, when I run it locally (in isolation?) I get:
0:08.99 TEST_STATUS: Thread-1  FAIL revocations.txt should be as expected.
(In reply to Jorg K (GMT+1) from comment #49)
> Umm, that's what I wrote in comment #44 ;-)

Ah, sorry. I missed that.

> Well, when I run it locally (in isolation?) I get:
> 0:08.99 TEST_STATUS: Thread-1  FAIL revocations.txt should be as expected.

Weird. What platform?

Comment 51

2 years ago
I tried to catch you on developers, but no luck.
I'm running it on Windows, look: https://pastebin.mozilla.org/8853849
(Assignee)

Comment 52

2 years ago
If I wipe all content from all-thunderbird.js I get a packaging error. But then the test works!! 
If I add back "#filter substitution" (only), I don't get the packaging error, but the test starts failing again. 

NO idea what to make of that.

Comment 53

2 years ago
OK, looks like Mark and Magnus are looking into it, so I will hold back (not that I'd have any bright idea).
(Assignee)

Comment 54

2 years ago
Created attachment 8696489 [details] [diff] [review]
bug1200567_blocklist_test.patch

Finally figured this out. It's another bust due to bug 1168178 - the test assumes no shipped blocklist.xml...
Assignee: philipp → mkmelin+mozilla
Attachment #8658484 - Attachment is obsolete: true
Attachment #8658484 - Flags: feedback?(mgoodwin)
Attachment #8696489 - Flags: review?(mgoodwin)
(Assignee)

Updated

2 years ago
status-thunderbird41: affected → ---
status-thunderbird42: affected → ---
status-thunderbird43: affected → ---
status-thunderbird44: affected → ---
status-thunderbird45: affected → ---
Component: General → Security: PSM
Product: Thunderbird → Core
Version: 39 → Trunk
(Reporter)

Comment 55

2 years ago
Great work Magnus, thanks for figuring this one out!

Comment 56

2 years ago
(In reply to Magnus Melin from comment #54)
> Finally figured this out. It's another bust due to bug 1168178 - the test
> assumes no shipped blocklist.xml...

Awesome, thanks!
Comment on attachment 8696489 [details] [diff] [review]
bug1200567_blocklist_test.patch

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

This looks good to me - passing on to keeler for the review
Attachment #8696489 - Flags: review?(mgoodwin)
Attachment #8696489 - Flags: review?(dkeeler)
Attachment #8696489 - Flags: feedback+
Comment on attachment 8696489 [details] [diff] [review]
bug1200567_blocklist_test.patch

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

I think it's worth investigating why there's a blocklist.xml file on comm-central but not mozilla-central rather than just overwriting it here. Does the test not get its own clean profile directory? What is the value of the XPCSHELL_TEST_PROFILE_DIR environment variable when the test runs?

::: security/manager/ssl/tests/unit/test_cert_blocklist.js
@@ +12,5 @@
>  //   unmodified
>  
>  var { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
>  
> +Components.utils.import("resource://gre/modules/FileUtils.jsm");

This isn't necessary - FileUtils.jsm is already imported in head_psm.js.

@@ +74,5 @@
> +  .createInstance(Ci.nsIFileOutputStream);
> +stream.init(blockFile, FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE | FileUtils.MODE_TRUNCATE,
> +            FileUtils.PERMS_FILE, 0);
> +
> +var data = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +

I must be missing something. Where is this used? It doesn't look like this is actually written out to the blocklist file.
Attachment #8696489 - Flags: review?(dkeeler) → review-
(Assignee)

Comment 59

2 years ago
Created attachment 8697098 [details] [diff] [review]
bug1200567_blocklist_test.patch

Whops, seems like an empty file was ok to but yes, the xml data was supposed to be written out.

The reason there's a blocklist.xml for comm-central is that Thunderbird does use a Thunderbird profile when running the xpcshell-tests so that we know when things aren't working (may need package changes etc.) Now, Firefox just runs with the "toolkit" profile so any browser/ prefs and packaging doesn't come into play. This is bug 1168178 which is crazy if you ask me...
Attachment #8696489 - Attachment is obsolete: true
Attachment #8697098 - Flags: review?(dkeeler)
Comment on attachment 8697098 [details] [diff] [review]
bug1200567_blocklist_test.patch

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

Ok - seems reasonable.

::: security/manager/ssl/tests/unit/test_cert_blocklist.js
@@ +68,5 @@
> +// is blocklisted by default
> +var blockFile = profile.clone();
> +blockFile.append("blocklist.xml");
> +var stream = Cc["@mozilla.org/network/file-output-stream;1"]
> +  .createInstance(Ci.nsIFileOutputStream);

nit: looks like the style in this file is to align the leading '.' of this line with the '[' in the previous line

@@ +69,5 @@
> +var blockFile = profile.clone();
> +blockFile.append("blocklist.xml");
> +var stream = Cc["@mozilla.org/network/file-output-stream;1"]
> +  .createInstance(Ci.nsIFileOutputStream);
> +stream.init(blockFile, FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE | FileUtils.MODE_TRUNCATE,

nit: keep lines to < 80 characters if possible
Attachment #8697098 - Flags: review?(dkeeler) → review+

Comment 61

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/807b0547abde
(Assignee)

Comment 62

2 years ago
Good (small) try on https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a13d154ced6

Comment 63

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/807b0547abde
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 64

2 years ago
Comment on attachment 8697098 [details] [diff] [review]
bug1200567_blocklist_test.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: This fixes an xpcshell test failure on comm-*.
Fix Landed on Version: 45
Risk to taking this patch (and alternatives if risky): Patch only affects tests
String or UUID changes made by this patch: none
Attachment #8697098 - Flags: approval-mozilla-esr38?
Attachment #8697098 - Flags: approval-mozilla-beta?
Attachment #8697098 - Flags: approval-mozilla-aurora?

Updated

a year ago
status-firefox44: --- → affected
Philor, tomcat, KWierso: Do we know whether this fixed the test failure on central or not? I would like to approve uplift once we have confirmation that this is "the fix". Please let me know.
Flags: needinfo?(wkocher)
Flags: needinfo?(philringnalda)
Flags: needinfo?(cbook)
Wrong central: this was never a failure on mozilla-central, which we sheriff, it was a failure on comm-central, which we do not. But, yes, this fixed this particular problem for them, without breaking Firefox.
Flags: needinfo?(wkocher)
Flags: needinfo?(philringnalda)
Flags: needinfo?(cbook)
Comment on attachment 8697098 [details] [diff] [review]
bug1200567_blocklist_test.patch

Test only changes don't need uplift approval. Beta44+, Aurora45+
Attachment #8697098 - Flags: approval-mozilla-beta?
Attachment #8697098 - Flags: approval-mozilla-beta+
Attachment #8697098 - Flags: approval-mozilla-aurora?
Attachment #8697098 - Flags: approval-mozilla-aurora+
I'm hitting conflicts trying to uplift this to beta (haven't yet tried aurora as it's still closed). Can we get a rebased patch if this needs to be uplifted?
Flags: needinfo?(mkmelin+mozilla)

Updated

a year ago
Keywords: checkin-needed

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 69

a year ago
Comment on attachment 8697098 [details] [diff] [review]
bug1200567_blocklist_test.patch

Originally landed on 45, so no uplift to aurora45 needed.
Flags: needinfo?(mkmelin+mozilla)
Attachment #8697098 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 70

a year ago
Created attachment 8700414 [details] [diff] [review]
bug1200567_blocklist.patch (beta44 version)
(Assignee)

Comment 71

a year ago
Created attachment 8700415 [details] [diff] [review]
bug1200567_blocklist.patch (esr38 version)

Updated

a year ago
Keywords: checkin-needed
has problems to apply to inbound - renamed 1200567 -> bug1200567_blocklist_test.patch
applying bug1200567_blocklist_test.patch
patching file security/manager/ssl/tests/unit/test_cert_blocklist.js
Hunk #1 FAILED at 54
Hunk #2 FAILED at 129
Hunk #3 FAILED at 214
3 out of 3 hunks FAILED -- saving rejects to file security/manager/ssl/tests/unit/test_cert_blocklist.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1200567_blocklist_test.patch
Flags: needinfo?(mkmelin+mozilla)
Keywords: checkin-needed
(Assignee)

Comment 73

a year ago
I think you tried to apply the trunk version, not the beta44 version of the patch. For beta44 it should be attachment 8700414 [details] [diff] [review].
Flags: needinfo?(mkmelin+mozilla)

Comment 74

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/1af2fbfc24a4
status-firefox44: affected → fixed

Comment 75

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1af2fbfc24a4
status-b2g-v2.5: --- → fixed
Comment on attachment 8697098 [details] [diff] [review]
bug1200567_blocklist_test.patch

Looks like a test only change so it shouldn't need approval.
Attachment #8697098 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+

Updated

a year ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-esr38/rev/c31c8a5a92b8
status-firefox-esr38: --- → fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.