Last Comment Bug 666516 - overriding ";1" causes a crash in the certManager.xul
: overriding ";1" causes a crash in the certM...
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: unspecified
: All All
: P1 critical (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: David Keeler [:keeler] (use needinfo?)
Depends on:
  Show dependency treegraph
Reported: 2011-06-23 00:43 PDT by passfree
Modified: 2011-09-19 23:26 PDT (History)
8 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fix (5.52 KB, patch)
2011-08-23 13:09 PDT, Boris Zbarsky [:bz] (still a bit busy)
kaie: review+
blizzard: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description passfree 2011-06-23 00:43:14 PDT
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;1 via the following chrome manifest

component      {6c367300-5a77-11df-a08a-0800200c9a61}                    components/CertOverrideService.js
contract;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;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] 2011-06-23 03:06:48 PDT
You mean bug 564584 which is marked fixed
Comment 2 passfree 2011-06-23 03:16:10 PDT
Unfortunately, not fixed.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-06-23 07:46:13 PDT
If you're getting a crash here, what's the stack look like?  Is there a breakpad incident id?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-06-23 07:51:38 PDT
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 2011-08-21 02:23:47 PDT
I can confirm that Xulrunner 6.0, FF6 still crash.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-08-23 13:09:07 PDT
Created attachment 555170 [details] [diff] [review]

This is ridiculous.  This code should never have been written....
Comment 7 Kai Engert (:kaie) (on vacation) 2011-08-23 13:51:57 PDT
Comment on attachment 555170 [details] [diff] [review]

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.

Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-08-23 14:04:44 PDT
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-08-23 14:05:20 PDT
Comment on attachment 555170 [details] [diff] [review]

Given that this is causing extension crashes, we should at least consider it for aurora.
Comment 10 Christopher Blizzard (:blizzard) 2011-08-23 14:33:06 PDT
Comment on attachment 555170 [details] [diff] [review]

Approved, please land on Aurora (Firefox Update 8.)
Comment 11 Marco Bonardo [::mak] 2011-08-24 01:46:13 PDT
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-08-25 20:56:45 PDT
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-19 15:58:37 PDT
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.

Comment 14 passfree 2011-09-19 23:26:51 PDT
I have verified that it works in 8. Though, I am not completely sure if this bug has been fully fixed everywhere.

Note You need to log in before you can comment on or make changes to this bug.