Closed Bug 860076 Opened 7 years ago Closed 6 years ago

MOZ_ASSERT(NS_IsMainThread()) fails when opening the certificate viewer

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: cviecco, Assigned: keeler)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
OS: Linux → All
Hardware: x86_64 → All
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.
Blocks: 891066
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8368231 - Flags: review?(brian)
(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.
Flags: needinfo?(cviecco)
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.
Attachment #8368231 - Flags: review?(brian) → review+
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.
Flags: needinfo?(cviecco)
Keywords: regression
Summary: default turns off ocsp when calling getchain → MOZ_ASSERT(NS_IsMainThread()) fails when opening the certificate viewer
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).
Attachment #8368231 - Flags: feedback-
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
Attached patch patch rebasedSplinter Review
Rebased patch and figured out why things were failing (see bug 967629).
https://tbpl.mozilla.org/?tree=Try&rev=47baae81f7a8
Attachment #8368231 - Attachment is obsolete: true
Attachment #8370430 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/23c20767b499
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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)
Attachment #8370430 - Flags: approval-mozilla-aurora?
Attachment #8370430 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.