nsClientAuthRememberService must be refcounted and implement nsISupportsWeakReference

REOPENED
Assigned to

Status

()

Core
General
REOPENED
9 years ago
6 years ago

People

(Reporter: Tomcat, Assigned: kaie)

Tracking

({assertion, fixed1.8.1.16, fixed1.9.0.2})

Trunk
assertion, fixed1.8.1.16, fixed1.9.0.2
Points:
---
Bug Flags:
wanted1.9.0.x +
blocking1.8.1.15 -
blocking1.8.1.16 +
in-testsuite ?
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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
kschwarz (on Windows Vista) and I (on Ubuntu Linux) both hit this assertion whenever we start fresh mozilla-central checkouts.

OS / Hardware --> All/All
OS: Windows XP → All
Hardware: PC → All
(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

9 years ago
I think that's a more recent regression.

Comment 4

9 years ago
Created attachment 325954 [details]
stack trace for the assertion (without the extension)

Updated

9 years ago
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.
Created attachment 326666 [details] [diff] [review]
Proposed fix
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #326666 - Flags: review?(kaie)
(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.
(Assignee)

Comment 8

9 years ago
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.
Attachment #326666 - Flags: review?(kaie) → review-
(Assignee)

Comment 9

9 years ago
Created attachment 327090 [details] [diff] [review]
Patch v2

Blake, does this make sense?
Assignee: mrbkap → kaie
Attachment #326666 - Attachment is obsolete: true
Attachment #327090 - Flags: review?(mrbkap)
(Assignee)

Updated

9 years ago
Flags: blocking1.8.1.15?
(Assignee)

Comment 10

9 years ago
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 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.
(Assignee)

Comment 12

9 years ago
Created attachment 327098 [details] [diff] [review]
Patch v3

is this sufficient?
Attachment #327090 - Attachment is obsolete: true
Attachment #327098 - Flags: review?(mrbkap)
Attachment #327090 - Flags: review?(mrbkap)
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.
Attachment #327098 - Flags: superreview+
Attachment #327098 - Flags: review?(mrbkap)
Attachment #327098 - Flags: review+
(Assignee)

Comment 14

9 years ago
(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**)
(Assignee)

Comment 15

9 years ago
Created attachment 327106 [details] [diff] [review]
Patch v3 merged to 1.8 branch
Attachment #327106 - Flags: approval1.8.1.16?
Attachment #327106 - Flags: approval1.8.1.15?
(Assignee)

Updated

9 years ago
Attachment #327106 - Attachment is obsolete: true
Attachment #327106 - Flags: approval1.8.1.16?
Attachment #327106 - Flags: approval1.8.1.15?
(Assignee)

Comment 16

9 years ago
Created attachment 327110 [details] [diff] [review]
Patch v4

This is v3 with Blake's request from comment 13 addressed.
Carrying forward his r+sr
Attachment #327098 - Attachment is obsolete: true
Attachment #327110 - Flags: superreview+
Attachment #327110 - Flags: review+
(Assignee)

Comment 17

9 years ago
Created attachment 327111 [details] [diff] [review]
Patch v4 merged to 1.8 branch
Attachment #327111 - Flags: approval1.8.1.16?
Attachment #327111 - Flags: approval1.8.1.15?
(Assignee)

Updated

9 years ago
Attachment #327110 - Flags: approval1.9.0.1?
(Assignee)

Comment 18

9 years ago
Changing Subject
Summary: ###!!! ASSERTION: Oops! You're asking for a weak reference to an object that doesn't support that → nsClientAuthRememberService must be refcounted and implement nsISupportsWeakReference
(Assignee)

Comment 19

9 years ago
patch v4 pushed to mozilla-central, marking fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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?
Blocks: 431819
(Assignee)

Comment 21

9 years ago
(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.
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.
Flags: blocking1.8.1.16+
Flags: blocking1.8.1.15?
Flags: blocking1.8.1.15-
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.
Flags: blocking1.8.1.16+
Attachment #327111 - Flags: approval1.8.1.15?
Attachment #327111 - Flags: approval1.8.1.16?
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.
Attachment #327111 - Flags: approval1.8.1.17?
Attachment #327111 - Flags: approval1.8.1.16?
Attachment #327111 - Flags: approval1.8.1.16+
(Assignee)

Comment 25

9 years ago
checked in to MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1.16
Attachment #327110 - Flags: approval1.9.0.2?
Attachment #327110 - Flags: approval1.9.0.1?
Attachment #327110 - Flags: approval1.9.0.1-

Updated

9 years ago
Flags: wanted1.9.0.x?
Comment on attachment 327110 [details] [diff] [review]
Patch v4

a=shaver for 1.9.0.x landing in service of TB3A2
Attachment #327110 - Flags: approval1.9.0.2? → approval1.9.0.2+
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?
Flags: in-testsuite?
(Assignee)

Comment 28

9 years ago
How do you test for "no assertion" and "C++ implementation object implements weak reference"?
(Reporter)

Comment 29

9 years ago
(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. 
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)?
(Assignee)

Updated

9 years ago
Keywords: fixed1.9.0.2
Flags: blocking1.8.1.17+
reopened, since the fix was backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?

Updated

9 years ago
Flags: in-litmus?
(Reporter)

Comment 33

9 years ago
(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
Flags: in-litmus? → in-litmus+
> > 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
Flags: wanted1.9.0.x? → wanted1.9.0.x+
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.
You need to log in before you can comment on or make changes to this bug.