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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 1

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

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0f107a637e7b01b7419347d60e9da785102946b&group_state=expanded

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Bug 1353458 - Make test_cert_blocklist more flexible about order of lines in revocations.txt.

This allows patches to land that will change the hashtable enumeration
order, which in turn changes the ordering of the lines in
revocations.txt.

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

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, the lint opt source-test-mozlint-eslint test actually caught something, so revised patch coming...

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135990/diff/1-2/

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f9635458f9c873758d263bfa1085c618352f5c7&group_state=expanded

Comment 7

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

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d4225b099025