666516 - overriding "@mozilla.org/security/certoverride;1" causes a crash in the certManager.xul

Status: RESOLVED FIXED

Description

[passfree]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_7) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/14.0.797.0 Safari/535.1
Build Identifier: xulrunner (all versions)

This is an old bug and it was reported like 2 years ago. I thought it was fixed but that is not the case in xul2.0 and perhaps even xul5.0

In summary, if we override @mozilla.org/security/certoverride;1 via the following chrome manifest

component      {6c367300-5a77-11df-a08a-0800200c9a61}                    components/CertOverrideService.js
contract       @mozilla.org/security/certoverride;1                      {6c367300-5a77-11df-a08a-0800200c9a61}

A crash will occur within certManager.xul when the following line is invoked 

serverTreeView.loadCertsFromCache(certcache, nsIX509Cert.SERVER_CERT);

As far as I remember the issue was somehow related to the way the XPCOM instance was handled in the C code (old code conventions?).

This service @mozilla.org/security/certoverride;1 is actually overridden by several popular extensions and products such as Selenium. A legitimate reason for overriding it is to hide bad ssl certificate problems.

Reproducible: Always

Steps to Reproduce:
See above.

Actual Results:  
The app crashes when certManager.xul is opened.

Expected Results:  
It should not crash.

Comment 1

[Matthias Versen [:Matti]]

You mean bug 564584 which is marked fixed

Comment 2

[passfree]

Unfortunately, not fixed.

Comment 3

[Boris Zbarsky [:bzbarsky, bz on IRC]]

If you're getting a crash here, what's the stack look like?  Is there a breakpad incident id?

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Actually, nevermind.  nsCertTree::GetCertsByTypeFromCertList has exactly the sort of code that was causing crashes in bug 564584, and the thing it's working with was gotten by contract id...  <sigh>.

Comment 5

[passfree]

I can confirm that Xulrunner 6.0, FF6 still crash.

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Created attachment 555170 [details] [diff] [review]
Fix

This is ridiculous.  This code should never have been written....

Comment 7

[Kai Engert (:kaie:)]

Comment on attachment 555170 [details] [diff] [review]
Fix

Yes, I agree this code wasn't written as future-proof as it should had.

And yes, we need more people caring for PSM and helping out with it, in particular with writing/reviewing patches and fixing correctness issues.

Thanks a lot for your fix, I appreciate it.

r=kaie

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9c84cf6b4d2c

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 555170 [details] [diff] [review]
Fix

Given that this is causing extension crashes, we should at least consider it for aurora.

Comment 10

[Christopher Blizzard (:blizzard)]

Comment on attachment 555170 [details] [diff] [review]
Fix

Approved, please land on Aurora (Firefox Update 8.)

Comment 11

[Marco Bonardo [::mak]]

http://hg.mozilla.org/mozilla-central/rev/9c84cf6b4d2c

Comment 12

[Boris Zbarsky [:bzbarsky, bz on IRC]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/9546f9e05e7a

Comment 13

[Anthony Hughes (:ashughes) [QA]]

Can QA get some help verifying this bug fix? Either passfree, if you can verify this on Firefox 8 Aurora, or if you can provide some more details on how to use the testcase you provided. It's not completely clear to QA how to utilize your testcase.

Thanks

Comment 14

[passfree]

I have verified that it works in 8. Though, I am not completely sure if this bug has been fully fixed everywhere.