413627 - nsCertOverrideService can't register observers (NS_ERROR_UNEXPECTED)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[(no longer active)]

It seems that nsCertOverrideService can't register observers.  The error return value (which is not checked for) is 0x8000ffff (NS_ERROR_UNEXPECTED).  Check the code: <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/security/manager/ssl/src/nsCertOverrideService.cpp&rev=1.3&mark=158-160#127>

There are two problems that I can spot in this code:

1. This seems to get called from a background thread, and not the main thread.  Bug 326491 changed nsObserverService to only accept observers from the main thread (see bug 326491 comment 9 for more info).  This comment mentions using an XPCOM proxy to work with nsObserverService from background threads, but I'm not sure how to do that.  Check the code: <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/ds/nsObserverService.cpp&rev=3.41&mark=113-121,130#113>

2. Specifying true as the third param of nsIObserverService::AddObserver() will only succeed when the component is capable of providing an nsIWeakReference, which does not seem to be the case with the cert override service.  So, the last param should be changed to PR_FALSE.

I observed this problem while working on bug 248970 and trying to add a new observer to this component, which failed for no apparent reason.  I'm willing to provide a patch by using a proxy as per bug 326491 comment 9, but I have no idea how to do that.  So, if someone can throw a hint, I'm willing to come up with a patch.

Comment 1

[(no longer active)]

Hmm, is this relevant here? <http://developer.mozilla.org/en/docs/nsISupports_proxies>

Comment 2

[(no longer active)]

I posted a patch (attachment 298757 [details] [diff] [review]) in bug 248970 which solves the issues in comment 0.  See bug 248970 comment 109 for more details.  Once that patch lands, we can mark this as RESOLVED FIXED.

Comment 3

[Mike Schroepfer]

Not going to block 1.9 for this - moving to wanted list

Comment 4

[timeless]

Attachment: derivative

Comment 5

[(no longer active)]

Comment on attachment 298906 [details] [diff] [review]
derivative

Thanks for the patch, timeless.  I think this should be reviewed by Kai.

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 298906 [details] [diff] [review]
derivative

Timeless, thank you very much for your patch.

I had not seen macro NS_WITH_ALWAYS_PROXIED_SERVICE before, I'm glad it exists :-)

r=kengert

Comment 7

[Reed Loden [:reed]]

Renominating for blocking since kaie believes this will fix b3 blocker bug 414808.

Comment 8

[Kai Engert [:KaiE:]]

Ehsan, thanks for your help, too. I understand this was originally your work and timeless copied it over here, so thanks to both of you.

Ehsan, I have a question about this patch, but I would like to defer any updates to a later time, because right now this patch seems to help for blocker bug 414808.

The question is: Is there really a need to hold on to mObserverService? nsCertOverrideService will only use it during init.

Comment 9

[Mike Schroepfer]

Comment on attachment 298906 [details] [diff] [review]
derivative

Given that this appears to fix bug 414808 approving for b3.  Can we land tonight?

Comment 10

[Kai Engert [:KaiE:]]

I'd be ok to check it in, but tree is orange. Adding checkin-needed keyword. Please feel free to land.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch v2

I can not reproduce Tomcat's crash, but I see a hang.

As I said before, I'd prefer to avoid keeping a reference to the observer service, it might be responsible for the hang?

Here is a new patch that doesn't hold a reference. I don't see a hang with it.

Tomcat, is this patch v2 better?

Comment 12

[(no longer active)]

(In reply to comment #8)
> Ehsan, thanks for your help, too. I understand this was originally your work
> and timeless copied it over here, so thanks to both of you.

Happy to help.  :-)

> Ehsan, I have a question about this patch, but I would like to defer any
> updates to a later time, because right now this patch seems to help for blocker
> bug 414808.
> 
> The question is: Is there really a need to hold on to mObserverService?
> nsCertOverrideService will only use it during init.

Nope, I don't see why that would be necessary.  I didn't hold on to mObserverService in my original patch in bug 248970, but I obtained it again at cleanup to call RemoveObserver() which was not really necessary since we're using weak references here.  I guess timeless might have been inclined to do this because of the variable naming convention which suggests it's a member variable.

Comment 13

[(no longer active)]

Oops, I accidentally touched some fields when posing the above comment.  :(

Comment 14

[Carsten Book [:Tomcat]]

(In reply to comment #11)
> Tomcat, is this patch v2 better?
> 

yes, works for me in Mozilla/5.0
(Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008013119
Firefox/3.0b3pre with this patch applied. No crash, no hang

Comment 15

[Kai Engert [:KaiE:]]

Comment on attachment 300826 [details] [diff] [review]
Patch v2

since this is a fix that people are waiting for, and since my changes are trivial, I'm carrying over my own review and approval and are going to check it in.

Comment 16

[Kai Engert [:KaiE:]]

Checked in, marking fixed.