A doc comment on bug 1322748 regarding the validity timestamps clued me into the error I created in the test for bug 1470516. This fixes that test, and may as well make the timestamp milliseconds to be consistent (and directly usable with js Date) with other timestamps in webext apis.

Comment on attachment 8991045 [details] Bug 1474626 - fix timestamp test and values, https://reviewboard.mozilla.org/r/256032/#review262896 ::: toolkit/modules/addons/SecurityInfo.jsm:217 (Diff revision 1) > > let certData = { > subject: cert.subjectName, > issuer: cert.issuerName, > validity: { > - start: cert.validity.notBefore, > + start: cert.validity.notBefore && cert.validity.notBefore / 1000, Should we also ensure that the result is still an integer number? (e.g. using Math.round, Math.trunc or Math.floor) (and if we should, it may be also worth to make a related assertion in the test case).

Comment on attachment 8991045 [details] Bug 1474626 - fix timestamp test and values, https://reviewboard.mozilla.org/r/256032/#review263148 ::: toolkit/modules/addons/SecurityInfo.jsm:217 (Diff revisions 1 - 2) > > let certData = { > subject: cert.subjectName, > issuer: cert.issuerName, > validity: { > - start: cert.validity.notBefore && cert.validity.notBefore / 1000, > + start: cert.validity.notBefore ? Math.floor(cert.validity.notBefore / 1000) : 0, Not sure if it would really make a huge difference, but it may worth to mention that with `Math.floor`: ``` Math.floor(100.1) => 100 Math.floor(100.6) => 100 Math.floor(100.8) => 100 Math.floor(-100.8) => -101 Math.floor(-100.1) => -101 ``` On the contrary with `Math.round`: ``` Math.round(100.1) => 100 Math.round(100.6) => 101 Math.round(100.8) => 101 Math.round(-100.8) => -101 Math.round(-100.1) => -100 ``` How about adding an explicit assertion to verify that these properties are going to be integer as expected? I guess that something like `browser.test.assertEq(Math.trunc(cert.validity.start), cert.validity.start, "Got an integer cert.validity.start number as expected");` should be enough.

Comment on attachment 8991045 [details] Bug 1474626 - fix timestamp test and values, https://reviewboard.mozilla.org/r/256032/#review263152 Thanks Shane, it looks good, there is only a small issue with one of the assertion that it seems to be actually copied twice by mistake. I'm setting r+ it in the meantime (with the above comment), because it is a super small issue in the test case and a new review round doesn't really seem necessary. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_hsts.html:44 (Diff revisions 2 - 3) > } > let cert = securityInfo.certificates[0]; > let now = Date.now(); > + browser.test.assertTrue(Number.isInteger(cert.validity.start), "cert start is integer"); > + browser.test.assertTrue(Number.isInteger(cert.validity.end), "cert end is integer"); > + browser.test.assertTrue(cert.validity.start < now, "cert start validity is correct"); is reviewboard that is messing with this interdiff or `browser.test.assertTrue(cert.validity.start < now, "cert start validity is correct");` is actually done twice here by mistake?

Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8a4ba4e8e53e fix timestamp test and values, r=rpl

Comment on attachment 8991045 [details] Bug 1474626 - fix timestamp test and values, Approval Request Comment [Feature/Bug causing the regression]: We didn't catch the logic error in the tests for bug 1470516 which resulted in the api being slightly wrong. [User impact if declined]: Incorrect time stamp values (microseconds in place of milliseconds) in ssl api [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: minimal change with fixed tests [String changes made/needed]: none

Comment on attachment 8991045 [details] Bug 1474626 - fix timestamp test and values, Fix for upcoming new feature, let's get this onto beta 8.

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag. Thanks!

The change here is api level so you'd have to write something using the api to validate. Given that it has a unit test I don't think further qa is necessary. April has written an extensive cert viewer extension using this api, so I'll just ask for external dev validation of this change based on that work.

I use the dates inside the DER, not the dates sent by the API. But I would be happy to take a look at it and verify that it looks correct. Is it pushed into Nightly?