As suggested in #1357391 comment 134, it would make sense to add a third slot to the test PKCS#11 module, so that the case of 'a slot without token exists' can be reliably tested. The current module has two slots; one always has a token, the other has one until a short time after the next C_WaitForSlotEvent() call. This makes it an unreliable option to test the 'no token found' case, which means the new browser.pkcs11 API introduced with #1357391 is not tested for that case. A third slot should be added, one that does not ever have a token, so that three testing for that API (and possibly others as well) can be improved.

anyone care to review that?

Comment on attachment 8919349 [details] Bug 1404421 - Add an empty slot to the test PKCS#11 module https://reviewboard.mozilla.org/r/190208/#review195628 Nice! This looks good, although I have some suggestions for improvements. In particular, it would be nice to have a case that ensures we get the status SLOT_NOT_PRESENT for that slot (see https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_pkcs11_slot.js - I would basically copy/paste the lines specific to slot 2 and update them for the empty slot you're adding). ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:84 (Diff revision 1) > return CKR_ARGUMENTS_BAD; > } > > - // Slot 2 is always present, while slot 1 may or may not be present. > + // Slot 3 is never present, slot 2 is always present, while slot 1 may or may > + // not be present. > CK_ULONG slotCount = (!limitToTokensPresent || tokenPresent ? 1 : 0) + 1; I think the ternary operator and the additional if statement make this a bit confusing now. How about we just break it out into 3 cases: * slotCount is always at least 1 because of slot 2 * we increase it by 1 if slot 1 is present (or if we want non-present tokens as well) * we again increase it by 1 if we want non-present tokens as well ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:85 (Diff revision 1) > } > > - // Slot 2 is always present, while slot 1 may or may not be present. > + // Slot 3 is never present, slot 2 is always present, while slot 1 may or may > + // not be present. > CK_ULONG slotCount = (!limitToTokensPresent || tokenPresent ? 1 : 0) + 1; > + if (!limitToTokensPresent) slotCount++; style nit - always use braces: ``` if (!limitToTokensPresent) { slotCount++; } ``` ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:112 (Diff revision 1) > - pSlotList[0] = 1; > + pSlotList[0] = 1; > - pSlotList[1] = 2; > + pSlotList[1] = 2; > + pSlotList[2] = 3; > + break; > + default: > + // how did we get here?!? `MOZ_ASSERT_UNREACHABLE("unexpected slot count in Test_C_GetSlotList");`

Comment on attachment 8919349 [details] Bug 1404421 - Add an empty slot to the test PKCS#11 module https://reviewboard.mozilla.org/r/190208/#review195628 > I think the ternary operator and the additional if statement make this a bit confusing now. How about we just break it out into 3 cases: > * slotCount is always at least 1 because of slot 2 > * we increase it by 1 if slot 1 is present (or if we want non-present tokens as well) > * we again increase it by 1 if we want non-present tokens as well That's essentially what we're doing with the ternary, but I agree that it's confusing I broke it out differently. I think that's easier to follow.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3) > In > particular, it would be nice to have a case that ensures we get the status > SLOT_NOT_PRESENT for that slot (see > https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/ > unit/test_pkcs11_slot.js - I would basically copy/paste the lines specific > to slot 2 and update them for the empty slot you're adding). Er, you meant the flag, not just the absense of the token. I guess it was a bit too early in the morning ;-) Let me try that again.

Comment on attachment 8919349 [details] Bug 1404421 - Add an empty slot to the test PKCS#11 module https://reviewboard.mozilla.org/r/190208/#review195964 Great - thanks! Unfortunately I told you to use the wrong assert macro, but that should be a quick fix. ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:25 (Diff revisions 1 - 3) > # include <unistd.h> // for usleep > #endif > > #include "pkcs11.h" > > +#include "mozilla/Assertions.h" Oh, shoot - sorry. I forgot we've avoided explicitly depending on other Mozilla things in this file. Let's just use <cassert> / assert("...") (put the include just above the one for string) ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:86 (Diff revisions 1 - 3) > return CKR_ARGUMENTS_BAD; > } > > - // Slot 3 is never present, slot 2 is always present, while slot 1 may or may > - // not be present. > - CK_ULONG slotCount = (!limitToTokensPresent || tokenPresent ? 1 : 0) + 1; > + // We always return slot 2 > + CK_ULONG slotCount = 1; > + // If we empty slots, we also return slots 1 and 3 nit: "If we want"? ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:92 (Diff revisions 1 - 3) > - if (!limitToTokensPresent) slotCount++; > + if (!limitToTokensPresent) { > + slotCount += 2; > + } else { > + // If we don't want empty slots, but token 1 is present, return that (but > + // not slot 3) > + if (tokenPresent) { style nit: since the else branch only contains the if, we can do an 'else if' here. ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:122 (Diff revisions 1 - 3) > pSlotList[0] = 1; > pSlotList[1] = 2; > pSlotList[2] = 3; > break; > default: > - // how did we get here?!? > + MOZ_ASSERT_UNREACHABLE("Unexpected slot count in Test_C_GetSlotList"); So this would just be 'assert("Unexpected slot...");'

Sorry, this had to be backed out for eslint failure at security/manager/ssl/tests/unit/test_pkcs11_slot.js:44 and static bustage at security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:119: https://hg.mozilla.org/integration/autoland/rev/b721cd90de90f188152728bc4005aa892fb659c9 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9ad123fb9ce5fd3211c6f675e8acbe452296880b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure eslint log: https://treeherder.mozilla.org/logviewer.html#?job_id=138001426&repo=autoland > TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/security/manager/ssl/tests/unit/test_pkcs11_slot.js:44:1 | Block must not be padded by blank lines. (padded-blocks) Failure log static: https://treeherder.mozilla.org/logviewer.html#?job_id=138001433&repo=autoland [task 2017-10-18T22:44:28.503Z] 22:44:28 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/security/manager/ssl/tests/unit/pkcs11testmodule/Unified_cpp_pkcs11testmodule0.cpp:2: [task 2017-10-18T22:44:28.503Z] 22:44:28 INFO - /builds/worker/workspace/build/src/security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:119:16: error: implicit conversion turns string literal into bool: 'const char [44]' to 'bool' [-Werror,-Wstring-conversion] [task 2017-10-18T22:44:28.503Z] 22:44:28 INFO - assert("Unexpected slot count in Test_C_GetSlotList"); [task 2017-10-18T22:44:28.503Z] 22:44:28 INFO - ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [task 2017-10-18T22:44:28.504Z] 22:44:28 INFO - /usr/include/assert.h:89:5: note: expanded from macro 'assert' [task 2017-10-18T22:44:28.504Z] 22:44:28 INFO - ((expr) \ [task 2017-10-18T22:44:28.504Z] 22:44:28 INFO - ^~~~ [task 2017-10-18T22:44:28.504Z] 22:44:28 INFO - 1 error generated.

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #10) > Sorry, this had to be backed out for eslint failure at > security/manager/ssl/tests/unit/test_pkcs11_slot.js:44 and static bustage at > security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:119: > > https://hg.mozilla.org/integration/autoland/rev/ > b721cd90de90f188152728bc4005aa892fb659c9 > > Push with failures: > https://treeherder.mozilla.org/#/ > jobs?repo=autoland&revision=9ad123fb9ce5fd3211c6f675e8acbe452296880b&filter- > resultStatus=testfailed&filter-resultStatus=busted&filter- > resultStatus=exception&filter-resultStatus=retry&filter- > resultStatus=usercancel&filter-resultStatus=runnable > > Failure eslint log: > https://treeherder.mozilla.org/logviewer.html#?job_id=138001426&repo=autoland > > TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/security/manager/ssl/tests/unit/test_pkcs11_slot.js:44:1 | Block must not be padded by blank lines. (padded-blocks) Whoops, forgot about running eslint. That was sloppy of me, sorry. > Failure log static: > https://treeherder.mozilla.org/logviewer.html#?job_id=138001433&repo=autoland > > [task 2017-10-18T22:44:28.503Z] 22:44:28 INFO - In file included from > /builds/worker/workspace/build/src/obj-firefox/security/manager/ssl/tests/ > unit/pkcs11testmodule/Unified_cpp_pkcs11testmodule0.cpp:2: > [task 2017-10-18T22:44:28.503Z] 22:44:28 INFO - > /builds/worker/workspace/build/src/security/manager/ssl/tests/unit/ > pkcs11testmodule/pkcs11testmodule.cpp:119:16: error: implicit conversion > turns string literal into bool: 'const char [44]' to 'bool' > [-Werror,-Wstring-conversion] > [task 2017-10-18T22:44:28.503Z] 22:44:28 INFO - > assert("Unexpected slot count in Test_C_GetSlotList"); Hrm. I think that's a bit overzealous; this is an "assert that this code is not reached" statement. Yes, this is a C++ source file, which has strong typing; but assert() is a macro, which does not. Anyway, I've added "== NULL" at the end, which will fix that, will retain the message, and should work (i.e., fail when reached) as expected. Started a try build to be sure this time around[1], but that revealed some errors on Windows. Don't have time to fix those now, but will do that later. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=708d9e06f615d5fb3dde54572c9bb2ea13550bfc

(In reply to David Keeler [:keeler] (use needinfo?) from comment #8) > ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp:122 > (Diff revisions 1 - 3) > > pSlotList[0] = 1; > > pSlotList[1] = 2; > > pSlotList[2] = 3; > > break; > > default: > > - // how did we get here?!? > > + MOZ_ASSERT_UNREACHABLE("Unexpected slot count in Test_C_GetSlotList"); > > So this would just be 'assert("Unexpected slot...");' Whoops - heh. The vanilla assert just takes an expression that it expects to be true, so this should be `assert(false);` If we ever encounter it, we'll get the source line and it'll be pretty obvious what's up (particularly if we add a comment).

(In reply to David Keeler [:keeler] (use needinfo?) from comment #13) > (In reply to David Keeler [:keeler] (use needinfo?) from comment #8) > > So this would just be 'assert("Unexpected slot...");' > > Whoops - heh. The vanilla assert just takes an expression that it expects to > be true, so this should be `assert(false);` If we ever encounter it, we'll > get the source line and it'll be pretty obvious what's up (particularly if > we add a comment). Yeah, I'd noticed that. I had wanted to do assert("Unexpected..." == NULL), which would have been syntactically valid (and still would've given the string), but then forgot to add the == NULL bit. Did that now. eslint errors were fixed now, too. Please check in again.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/70d574308366 Add an empty slot to the test PKCS#11 module r=keeler