Closed Bug 1200567 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Security: PSM, defect)

Product:
Core
Component:
Security: PSM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox-esr38 --- fixed
b2g-v2.5 --- fixed
thunderbird_esr38 --- affected

People

(Reporter: Fallen, Assigned: mkmelin)

References

Details

(Keywords: intermittent-failure)

Attachments

(3 files, 2 obsolete files)

bug1200567_blocklist_test.patch
3.50 KB, patch
keeler
: review+
ritu
: approval-mozilla-beta+
lizzard
: approval-mozilla-esr38+
Details | Diff | Splinter Review
bug1200567_blocklist.patch (beta44 version)
2.32 KB, patch
Details | Diff | Splinter Review
bug1200567_blocklist.patch (esr38 version)
2.34 KB, patch
Details | Diff | Splinter Review 
Might be fallout from bug 1184381
Attached file failing test log with debug messages (obsolete) — 
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)
(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.
(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 ;)
Attachment #8658484 - Flags: feedback?(mgoodwin)
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)
./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 ;-)
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)
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)
(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?
I tried to catch you on developers, but no luck.
I'm running it on Windows, look: https://pastebin.mozilla.org/8853849
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.
OK, looks like Mark and Magnus are looking into it, so I will hold back (not that I'd have any bright idea).
Attached patch bug1200567_blocklist_test.patch (obsolete) — — Splinter Review 
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)
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
Great work Magnus, thanks for figuring this one out!
(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-
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+
https://hg.mozilla.org/mozilla-central/rev/807b0547abde
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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?
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)
Keywords: checkin-needed
Keywords: checkin-needed
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+
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
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 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+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: