When using the default configuration the getchain call disables ALL ocsp fetches to prevent network connections. This not even addresses all connections as crls could be enabled too.

This now fails MOZ_ASSERT(NS_IsMainThread()); in SkipOcspOff added by bug 891066, meaning that looking at a certificate in the certificate manager crashes debug builds.

I think we should just remove this entirely. My impression is it's intended that this prevent OCSP lookups that could block the main thread (ironically I found this from a verification that isn't on the main thread), but it disables OCSP for the entire browser, which is wrong. The real solution would be to have a certificate verification library that can be told not to perform network traffic on a case-by-case basis.

(In reply to Camilo Viecco (:cviecco) from comment #0) > When using the default configuration the getchain call disables ALL ocsp > fetches to prevent network connections. This not even addresses all > connections as crls could be enabled too. Which GetChain call are you referring to? I don't think I added a call to GetChain in bug 891066.

Comment on attachment 8368231 [details] [diff] [review] patch Review of attachment 8368231 [details] [diff] [review]: ----------------------------------------------------------------- I agree with this change but I don't understand how I introduced a bug here.

The certificate viewer calls requestUsagesArrayAsync which eventually calls nsUsageArrayHelper::GetUsagesArray, which is where the SkipOcsp/SkipOcspOff calls happen. https://hg.mozilla.org/mozilla-central/rev/95f848f55c90 added an assertion to SkipOcspOff that checks for being on the main thread.

Thanks. I agree with this change. Almost nobody uses the certificate viewer. I believe this change to avoid OCSP checks in the certificate viewer was done because originally the certificate validation was done on the main thread, and we were causing jank by blocking the main thread. Now the certificate verification is asynchronous and happens off the main thread, so jank isn't a factor. In fact, if this is the only use of the LOCAL_ONLY flag then I'd support removing that flag too.

keeler answered by question in comment 5.

Comment on attachment 8368231 [details] [diff] [review] patch Review of attachment 8368231 [details] [diff] [review]: ----------------------------------------------------------------- Things I like about this: 1. We have a better test for the certmanager ui 2. solves the debug crash but there are things I dont like about this: 1. The regression (and patches) should be in another bug. 1. We can potentially be blocking on the network for this ui issue. 2. I would really like to remove this, but only after certverifier friends actually support the LOCAL_ONLY_FLAG (this was the purpose of this bug).

15:43 cviecco | I am looking at bug 860076 15:44 cviecco | what happens if you just remove the assertion? 15:45 keeler | preferences would be accessed off the main thread, which is apparently not allowed 15:48 cviecco | are we ok with going to the network? 15:51 keeler | well, in the case of that one, it's actually fine since it isn't on the main thread 15:51 keeler | (when asyncGetUsagesArray or whatever is called) 15:52 cviecco | ok. 15:54 keeler | it's true that other calls can now cause network traffic and might block the main thread, but | there's already plenty of cases where that happens, so it's something that needs a more | wide-spread fix anyway 15:55 cviecco | yes, that bug was for that fix, just got suprised there was no new bug number with regression | associated with it. 15:56 keeler | I see. Looks like I misunderstood and ended up morphing that bug a bit - would you rather I just | file a new bug? 15:56 cviecco | that is ok. with your fix my bug makes no sense. 15:56 keeler | ok 16:17 keeler | ping - so, are you cool with the patch as it is to land? 16:19 cviecco | yes 16:20 keeler | cool - thanks

Thanks for the quick review. (In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #6) > In fact, if this is the only use of the LOCAL_ONLY flag then I'd support > removing that flag too. Doesn't look like it - nsNSSCertificate::hasValidEVOidTag uses it too :( (as well as some tests) https://hg.mozilla.org/integration/mozilla-inbound/rev/f693f6c91b23

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3635f2f0c4f6 for breaking browser_certViewer.js: https://tbpl.mozilla.org/php/getParsedLog.php?id=33848147&tree=Mozilla-Inbound

Now with extra try: https://tbpl.mozilla.org/?tree=Try&rev=7fa8afb92333

Rebased patch and figured out why things were failing (see bug 967629). https://tbpl.mozilla.org/?tree=Try&rev=47baae81f7a8

Pushed with the patch for bug 967629. https://hg.mozilla.org/integration/mozilla-inbound/rev/23c20767b499

Comment on attachment 8370430 [details] [diff] [review] patch rebased [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 891066 User impact if declined: debug builds assert when viewing a certificate Testing completed (on m-c, etc.): on m-c, has an automated test Risk to taking this patch (and alternatives if risky): none String or IDL/UUID changes made by this patch: nsINSSComponent UUID rev'd (I can prepare a patch where this isn't necessary if need be)

Pushed with the patch for bug 967629. https://hg.mozilla.org/releases/mozilla-aurora/rev/9f6c81e93e37