Last Comment Bug 426555 - nsClientAuthRememberService must be refcounted and implement nsISupportsWeakReference
: nsClientAuthRememberService must be refcounted and implement nsISupportsWeakR...
Status: REOPENED
: assertion, fixed1.8.1.16, fixed1.9.0.2
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Kai Engert (:kaie)
:
Mentors:
https://addons.mozilla.org/en-US/fire...
Depends on:
Blocks: 431819
  Show dependency treegraph
 
Reported: 2008-04-02 10:37 PDT by Carsten Book [:Tomcat]
Modified: 2011-07-18 09:48 PDT (History)
17 users (show)
dveditz: wanted1.9.0.x+
samuel.sidler+old: blocking1.8.1.15-
samuel.sidler+old: blocking1.8.1.16+
shaver: in‑testsuite?
cbook: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack (5.44 KB, text/plain)
2008-04-02 10:37 PDT, Carsten Book [:Tomcat]
no flags Details
stack trace for the assertion (without the extension) (6.54 KB, text/plain)
2008-06-20 02:50 PDT, Jesse Ruderman
no flags Details
Proposed fix (3.00 KB, patch)
2008-06-25 04:56 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
kaie: review-
Details | Diff | Review
Patch v2 (5.06 KB, patch)
2008-06-27 04:24 PDT, Kai Engert (:kaie)
no flags Details | Diff | Review
Patch v3 (6.94 KB, patch)
2008-06-27 05:24 PDT, Kai Engert (:kaie)
mrbkap: review+
mrbkap: superreview+
Details | Diff | Review
Patch v3 merged to 1.8 branch (7.64 KB, patch)
2008-06-27 06:55 PDT, Kai Engert (:kaie)
no flags Details | Diff | Review
Patch v4 (6.94 KB, patch)
2008-06-27 07:05 PDT, Kai Engert (:kaie)
kaie: review+
kaie: superreview+
mbeltzner: approval1.9.0.1-
shaver: approval1.9.0.2+
Details | Diff | Review
Patch v4 merged to 1.8 branch (7.64 KB, patch)
2008-06-27 07:06 PDT, Kai Engert (:kaie)
samuel.sidler+old: approval1.8.1.16+
Details | Diff | Review

Description Carsten Book [:Tomcat] 2008-04-02 10:37:00 PDT
Created attachment 313115 [details]
stack

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040215 Minefield/3.0pre ID:2008040215

Steps to reproduce:
-> Install the DejaClick Extension (https://addons.mozilla.org/en-US/firefox/addons/versions/3262)
-> Make this Extension Compatible to Firefox Nightly (as example with the Nightly tester tool)
-> Start Firefox with this Extension enabled 
-> Quit Firefox

###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that doesn't support that.: 'factoryPtr', file c:/debug/mozilla/firefox-

debug/xpcom/build/nsWeakReference.cpp, line 109
Comment 1 Daniel Holbert [:dholbert] 2008-06-19 16:47:21 PDT
kschwarz (on Windows Vista) and I (on Ubuntu Linux) both hit this assertion whenever we start fresh mozilla-central checkouts.

OS / Hardware --> All/All
Comment 2 Daniel Holbert [:dholbert] 2008-06-19 16:58:04 PDT
(In reply to comment #1)
> kschwarz (on Windows Vista) and I (on Ubuntu Linux) both hit this assertion
> whenever we start fresh mozilla-central checkouts.

Meant to say -- we both hit it _without any addons installed_.
Comment 3 Jesse Ruderman 2008-06-20 02:47:37 PDT
I think that's a more recent regression.
Comment 4 Jesse Ruderman 2008-06-20 02:50:44 PDT
Created attachment 325954 [details]
stack trace for the assertion (without the extension)
Comment 5 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-06-25 04:55:45 PDT
I just went through the same path that Pelle did in bug 431819, comment 137, but took it further: The reason that adding nsISupportsWeakReference to nsClientAuthRememberService crashes is that the only instantiation of that class is not actually refcounted. Therefore, successfully grabbing a weak reference to it adds and subtracts 1 reference, causing it be freed (but it isn't malloced, which in itself is bad) and runs the destructor. Then later, we pass a member to PR_EnterMonitor and die.

I don't see why nsClientAuthRememberService is an observer at all: nsNSSComponent listens to the same topic as it and notifies it when it receives it. I have a patch to rip all of the observer stuff out and make nsClientAuthRememberService not an XPCOM object, which gets rid of the assertion and doesn't crash and still works.
Comment 6 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-06-25 04:56:15 PDT
Created attachment 326666 [details] [diff] [review]
Proposed fix
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-06-25 05:01:08 PDT
(In reply to comment #5)
> I don't see why nsClientAuthRememberService is an observer at all:
> nsNSSComponent listens to the same topic as it and notifies it when it receives
> it.

As a note: this happens through nsNSSComponent::ShutdownNSS. If we need to do what was in the observe function even if NSS initialization failed, then my patch won't work.
Comment 8 Kai Engert (:kaie) 2008-06-27 03:59:01 PDT
Comment on attachment 326666 [details] [diff] [review]
Proposed fix

Blake, thanks a lot for working on this patch!

Unfortunately I have to give this r- because we do have apps (like seamonkey or embedding apps) that support profile switching on runtime.

We must clear that list on switching the profile.
Comment 9 Kai Engert (:kaie) 2008-06-27 04:24:07 PDT
Created attachment 327090 [details] [diff] [review]
Patch v2

Blake, does this make sense?
Comment 10 Kai Engert (:kaie) 2008-06-27 04:35:15 PDT
I think originally this bug report must have been related to a different problem, because the code we are trying to fix with the patch has been introduced much later, with bug 431819.
Comment 11 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-06-27 05:00:05 PDT
Comment on attachment 327090 [details] [diff] [review]
Patch v2

> NS_IMETHODIMP
> nsNSSComponent::GetClientAuthRememberService(nsClientAuthRememberService **cars)
> {
>   NS_ENSURE_ARG_POINTER(cars);
>-  *cars = &mClientAuthRememberService;
>+  NS_IF_ADDREF(mClientAuthRememberService);
>+  *cars = mClientAuthRememberService;

This could be one line: NS_IF_ADDREF(*cars = mClientAuthRememberService);
Also, you need to return NS_OK from the function.

This is fine, but won't fix the assertion because you still need to make nsClientAuthRememberService implement nsWeakReference.
Comment 12 Kai Engert (:kaie) 2008-06-27 05:24:29 PDT
Created attachment 327098 [details] [diff] [review]
Patch v3

is this sufficient?
Comment 13 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-06-27 05:29:11 PDT
Comment on attachment 327098 [details] [diff] [review]
Patch v3

>+++ b/security/manager/ssl/src/nsClientAuthRemember.cpp
>+NS_IMPL_THREADSAFE_ISUPPORTS2(nsClientAuthRememberService, 
>+                              nsIObserver,
>+                              nsSupportsWeakReference)

This wants to be nsISupportsWeakReference.

r+sr=mrbkap with that.
Comment 14 Kai Engert (:kaie) 2008-06-27 06:53:39 PDT
(In reply to comment #13)
> This wants to be nsISupportsWeakReference.

Are you sure?

- PSM already has other code that uses the version without "I"

- when I add the "I", I get:
  
security/manager/ssl/src/nsNSSComponent.cpp:293: error: cannot allocate an object of abstract type 'nsClientAuthRememberService'
security/manager/ssl/src/nsClientAuthRemember.h:144: note:   because the following virtual functions are pure within 'nsClientAuthRememberService':
nsIWeakReference.h:142: note:    virtual nsresult nsISupportsWeakReference::GetWeakReference(nsIWeakReference**)
Comment 15 Kai Engert (:kaie) 2008-06-27 06:55:46 PDT
Created attachment 327106 [details] [diff] [review]
Patch v3 merged to 1.8 branch
Comment 16 Kai Engert (:kaie) 2008-06-27 07:05:26 PDT
Created attachment 327110 [details] [diff] [review]
Patch v4

This is v3 with Blake's request from comment 13 addressed.
Carrying forward his r+sr
Comment 17 Kai Engert (:kaie) 2008-06-27 07:06:40 PDT
Created attachment 327111 [details] [diff] [review]
Patch v4 merged to 1.8 branch
Comment 18 Kai Engert (:kaie) 2008-06-27 07:28:01 PDT
Changing Subject
Comment 19 Kai Engert (:kaie) 2008-06-27 07:54:58 PDT
patch v4 pushed to mozilla-central, marking fixed
Comment 20 Samuel Sidler (old account; do not CC) 2008-06-27 08:48:15 PDT
A couple of questions:
  1) This seems mostly theoretical (i.e., we don't have a crash testcase currently). Can we find a point at which we think this will crash and make a testcase?
  2) Given this code is in mozilla/security, my gut is that we need it both for Firefox and Thunderbird. Would there be an issue with taking it only for Thunderbird now and fixing it for Firefox later? Which is it more likely to be a problem with?
Comment 21 Kai Engert (:kaie) 2008-06-27 13:19:31 PDT
(In reply to comment #20)
> A couple of questions:
>   1) This seems mostly theoretical (i.e., we don't have a crash testcase
> currently).

I haven't crash myself, and I don't see a comment saying someone crashed.
So: you're probably right


> Can we find a point at which we think this will crash and make a
> testcase?

We'd have to investigate how to trigger this.


>   2) Given this code is in mozilla/security, my gut is that we need it both for
> Firefox and Thunderbird.

Absolutely. This about SSL client auth, which can happen with any application protocol layered inside SSL. https as well as any mail or ldap protocol over SSL.


> Would there be an issue with taking it only for
> Thunderbird now and fixing it for Firefox later? Which is it more likely to be
> a problem with?

I don't think one is more likely than the other.

It really depends on whether people are using client certs to authenticate to their SSL servers, can be corporate websites or mail.
Comment 22 Samuel Sidler (old account; do not CC) 2008-06-27 13:24:34 PDT
Thanks Kaie. We're not going to respin for this fix. We'll take it in the next release which will be on a short-cycle.
Comment 23 Samuel Sidler (old account; do not CC) 2008-06-27 15:29:52 PDT
I'm marking this as blocking1.8.1.16, but this fix will be a ride-along and we might ship without it. We'll still need to evaluate the fix before landing, however. We'll do that on Monday when dveditz is back.
Comment 24 Samuel Sidler (old account; do not CC) 2008-06-30 12:30:05 PDT
Comment on attachment 327111 [details] [diff] [review]
Patch v4 merged to 1.8 branch

Approved for 1.8.1.16. a=ss for release-drivers.

Please land on the MOZILLA_1_8_BRANCH as soon as possible.
Comment 25 Kai Engert (:kaie) 2008-06-30 15:54:34 PDT
checked in to MOZILLA_1_8_BRANCH
Comment 26 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-07-07 17:30:28 PDT
Comment on attachment 327110 [details] [diff] [review]
Patch v4

a=shaver for 1.9.0.x landing in service of TB3A2
Comment 27 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-07-07 17:32:52 PDT
If this is important enough to land on the 1.9.0.x branch, it's important enough to have a test.  Please, people who were so interested in this fix, provide such a test in the next 2 weeks?
Comment 28 Kai Engert (:kaie) 2008-07-08 07:49:39 PDT
How do you test for "no assertion" and "C++ implementation object implements weak reference"?
Comment 29 Carsten Book [:Tomcat] 2008-07-08 07:55:27 PDT
(In reply to comment #28)
> How do you test for "no assertion" and "C++ implementation object implements
> weak reference"?
> 

I will create a litmus test for this. 
Comment 30 Blake Kaplan (:mrbkap) (please use needinfo!) 2008-07-08 07:58:39 PDT
Kai, is it possible to test that you're successfully clearing the proper lists when we signal profile-before-change (i.e. nsClientAuthRememberService was successfully registered as an observer)?
Comment 31 Nelson Bolyard (seldom reads bugmail) 2008-09-16 19:13:35 PDT
reopened, since the fix was backed out.
Comment 32 Wayne Mery (:wsmwk, NI for questions) 2008-10-02 04:37:18 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > How do you test for "no assertion" and "C++ implementation object implements
> > weak reference"?
> > 
> 
> I will create a litmus test for this. 

Carsten, did your test get created?
Comment 33 Carsten Book [:Tomcat] 2008-10-05 13:33:09 PDT
(In reply to comment #32)
> (In reply to comment #29)
> > (In reply to comment #28)
> > I will create a litmus test for this. 
> 
> Carsten, did your test get created?

-> https://litmus.mozilla.org/show_test.cgi?id=7044 with my steps to reproduce from comment #0
Comment 34 Wayne Mery (:wsmwk, NI for questions) 2009-01-24 11:34:30 PST
> > Carsten, did your test get created?
> 
> -> https://litmus.mozilla.org/show_test.cgi?id=7044 with my steps to reproduce
> from comment #0

that testcase is disabled
Comment 35 Henrik Skupin (:whimboo) 2009-02-21 20:44:05 PST
Such tests make no sense on Litmus because none of the testers can run them without having their own debug build. We could create a new testgroup for that but I would like to hear that it is really not possible to create an automated test for.

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