security/manager/ssl/tests/unit/test_cert_blocklist.js depends on hashtable enumeration order

RESOLVED FIXED in Firefox 55

Status

()

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

People

(Reporter: dbaron, Unassigned)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf-])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
I was trying to land bug 1352889, one of whose effects is to change the hashing used in PLDHashTable.  This changes hashtable enumeration order.

This change led to a single test failure, in security/manager/ssl/tests/unit/test_cert_blocklist.js .  This test failure is easier to understand if I change kTruncateLength from 128 to 4096 here:
https://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/testing/modules/Assert.jsm#54

The failure (with the larger kTruncateLength) is:

 0:02.25 TEST_STATUS: Thread-1  FAIL revocations.txt should be as expected - "# Auto generated contents. Do not edit.\\nMCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5\\n\\tVCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=\\nMBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E=\\n exJUIJpq50jgqOwQluhVrAzTF74=\\nMBIxEDAOBgNVBAMMB1Rlc3QgQ0E=\\n BVio/iQ21GCi2iUven8oJ/gae74=\\nYW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy\\n YW5vdGhlciBzZXJpYWwu\\n c2VyaWFsMi4=" == "# Auto generated contents. Do not edit.\\nMCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5\\n\\tVCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=\\nMBIxEDAOBgNVBAMMB1Rlc3QgQ0E=\\n BVio/iQ21GCi2iUven8oJ/gae74=\\nMBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E=\\n exJUIJpq50jgqOwQluhVrAzTF74=\\nYW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy\\n YW5vdGhlciBzZXJpYWwu\\n c2VyaWFsMi4="
/home/dbaron/builds/mozilla-central/obj/firefox-debugopt/_tests/xpcshell/security/manager/ssl/tests/unit/test_cert_blocklist.js:check_revocations_txt_contents:208
/home/dbaron/builds/mozilla-central/obj/firefox-debugopt/_tests/xpcshell/security/manager/ssl/tests/unit/test_cert_blocklist.js:run_test/<:303
/home/dbaron/builds/mozilla-central/mozilla/testing/xpcshell/head.js:_run_next_test:1569
/home/dbaron/builds/mozilla-central/mozilla/testing/xpcshell/head.js:run:703
/home/dbaron/builds/mozilla-central/mozilla/testing/xpcshell/head.js:_do_main:211
/home/dbaron/builds/mozilla-central/mozilla/testing/xpcshell/head.js:_execute_test:546
-e:null:1
 0:02.25 LOG: Thread-1 INFO exiting test
 0:02.25 LOG: Thread-1 ERROR 2147500036


This shows the lines in the expected output being reordered.



What's the best way to proceed here?  Is it actually important that the hashtable enumeration order remain unchanged, or does the test just happen to depend on it?  If so, is there a better way to fix the test than to just change the test expectation?

(Note that I may need to do this *again* for bug 1352890.)
Flags: needinfo?(mgoodwin)
Whiteboard: [qf]
(Reporter)

Updated

a year ago
Flags: needinfo?(dkeeler)
There is some structure to the file that we're testing there, but we can ensure that it is as expected without depending on the hashtable enumeration order. As far as I understand, these are the requirements we need to ensure:

1. The file starts with "# Auto generated contents. Do not edit."
2. The file has four sets of related lines:

MCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5
\tVCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=

MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=
 BVio/iQ21GCi2iUven8oJ/gae74=

MBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E=
 exJUIJpq50jgqOwQluhVrAzTF74=

YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy
 YW5vdGhlciBzZXJpYWwu
 c2VyaWFsMi4=

where these sets may appear in any order, but the contents of each set should remain together (that is, two sets shouldn't be interleaved). For the last set in this list (the one that has two indented lines after the main line), the order of the indented lines shouldn't matter either (but to be clear, the heading line of each set should always be first).

3. The file shouldn't contain anything else.

Let me know if I should elaborate on anything here. If that's enough detail to fix this test with respect to bug 1352889 and if you have the time, feel free to write a patch - I can review it. If not, I'll find someone to address this as soon as I can.
Flags: needinfo?(dkeeler)

Updated

a year ago
Whiteboard: [qf] → [qf-]
Flags: needinfo?(mgoodwin)
Comment hidden (mozreview-request)
(Reporter)

Comment 4

a year ago
Er, the lint opt source-test-mozlint-eslint test actually caught something, so revised patch coming...
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8864344 [details]
Bug 1353458 - Make test_cert_blocklist more flexible about order of lines in revocations.txt.

https://reviewboard.mozilla.org/r/135990/#review139284

Nice - I like this solution. Thank you for taking the time to do this.
Attachment #8864344 - Flags: review?(dkeeler) → review+
(Reporter)

Comment 8

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4225b0990255df9f75157a74c249878a2f47103
Bug 1353458 - Make test_cert_blocklist more flexible about order of lines in revocations.txt.  r=keeler

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d4225b099025
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.