Georg Koppen Reporter
	Description • 12 years ago

User Agent: Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121010030605

Steps to reproduce:

A user installed FoxyProxy Plus on a recent Firefox (16) and tried to whitelist self-signed certificates.


Actual results:

The user could not click on "Confirm Security Exception" (it was greyed out) and he got a 

"No information available
 Unable to obtain identification status for the given site." 

on the usual "Add Security Exception" dialog.

I tracked the issue down a bit: The last working nightly is the one from 2012-03-31 and the first broken one is from 2012-04-01. Furthermore, comparing the mozilla-central pushlog (http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c43cfe73516&tochange=4a8a5e8ef78b) and the one from mozilla-inbound it seems the culprit is http://hg.mozilla.org/mozilla-central/rev/bbe5086163c9 which results in the following bug range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7bfb26afd19b&tochange=5b524a2b9ec1

Attached is a simple test case (extension) showing the issue. In order to reproduce it do the following:

1) Install the extension in a new profile in a recent Firefox (say 15.0.1)
2) Try to add a security exception for a site with a self-signed certificate (e.g. https://svn.jondos.de)


Expected results:

The user should be able to whitelist the sites with self-signed certificates.

	Renaud Allard
	Comment 1 • 12 years ago

I confirm this issue. I must admit it is annoying to not be able to even whitelist if information is unknown. For example, if you have HP ILO access, you cannot even access the ILO as you cannot whitelist it.

	Matthias Versen [:Matti]
	Comment 2 • 12 years ago

The first bad revision is:
changeset:   90729:1bdb337e3136
user:        Peter Van der Beken <peterv@propagandism.org>
date:        Fri Mar 30 21:42:20 2012 -0700
summary:     Fix for bug 740069 (Generate JS bindings in C++ with a python scrip
t for DOM objects on the main thread and in workers. Infrastructure and new bind
ings for XMLHttpRequest). Patch by bent/bz/bholley/jst/khuey/peterv, r=bent/bz/b
holley/jlebar/khuey/peterv/sicking/smaug.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 12 years ago

Well, what's foxyproxy doing to mess with that XHR?  ;)

Any exceptions in error console?

	Georg Koppen Reporter
	Comment 4 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #3)
> Well, what's foxyproxy doing to mess with that XHR?  ;)
Well, we are following your advice :):
"(06:56:39 PM) bz: ericjung: how about just returning the original notificationCallbacks?
 (06:56:51 PM) bz: ericjung: if asked for an nsIBadCertListener/2?"
The code is part of hooking nsIAuthPrompt, nsIAuthPrompt2 and nsIAuthPromprProvider in order to replace some of their functionality with a custom one.
 
> Any exceptions in error console?
Only this one:

Timestamp: 13.10.2012 20:17:25
Error: Attempted to connect to a site with a bad certificate in the add exception dialog. This results in a (mostly harmless) exception being thrown. Logged for information purposes only: [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://pippki/content/exceptionDialog.js :: checkCert :: line 135"  data: no]
Source File: chrome://pippki/content/exceptionDialog.js
Line: 143

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 12 years ago

Hmm.  So my first guess was bug 780529, but we fixed that in 16.

Then again, it might be a manifestation of the same issue.  What does the relevant foxyproxy code that sets up the notification callbacks and deal with nsIBadCertListener stuff look like?

	Eric Jung [:ericjung]
	Comment 6 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #5)
> Then again, it might be a manifestation of the same issue.  What does the
> relevant foxyproxy code that sets up the notification callbacks and deal
> with nsIBadCertListener stuff look like?

This piece of FoxyProxy is not open-source. Is it possible to either secure this ticket or send you the relevant code in a non-public manner?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 12 years ago

You can certainly send it directly to my e-mail address.  I _could_ mark the entire bug security-sensitive, but that's not quite what we want in general....

	Georg Koppen Reporter
	Comment 8 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #7)
> You can certainly send it directly to my e-mail address.  I _could_ mark the
> entire bug security-sensitive, but that's not quite what we want in
> general....
No need for such a drastic measure. Especially as this is no security issue and other users/add-on developers may be affected by it as well. I just sent you an e-mail with the code.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 12 years ago

Thanks.  Sorry for the lag; the upstream spam filters didn't like it...

So looking at your code, a question.  In your getInterface implementation, is it possible that the call through to getInterface on original notification callbacks is throwing?  It looks to me like if it did that exception would just get silently swallowed, maybe.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 12 years ago

Anyway, trying it now with the attached test extension.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 12 years ago

> is it possible that the call through to getInterface on original notification callbacks
> is throwing?

Yes, this is exactly what happens.  For some reason we can't seem to unwrap the argument to nsIJSIID.  Looking into why.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 12 years ago

So we get into xpc_qsUnwrapArgImpl, wrapper is non-null and we have:

(gdb) p wrapper->mIdentity
$12 = (nsJSID *) 0x1106b5e40

But getInterface on XMLHttpRequest is declared to take "IID" and Bindings.conf says that maps to nsIJSIID.  And nsJSID does not implement nsIJSIID; it only implements nsIJSID.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 12 years ago

Oh, and the XPCOM getInterface is declared as nsIIDRef which is then declared as:

[ref, nsid]   native nsIIDRef(nsIID);

Looking at XPCConvert, looks like XPCConvert::NativeData2JS in the nsXPTType::T_IID case uses xpc_NewIDObject, which calls nsJSID::NewID.  And going the other way, XPCConvert::JSData2Native in the nsXPTType::T_IID case does xpc_JSObjectToID, which basically also goes through nsIJSID.

All of which is to say we should be mapping IID to nsIJSID.  And we seriously need tests for this stuff.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 15 • 12 years ago

Comment on attachment 671686 [details] [diff] [review]
This fixes the attached extension.  Tests coming up in a bit

Review of attachment 671686 [details] [diff] [review]:
-----------------------------------------------------------------

We were only one letter off!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/c317ac04497a

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 12 years ago

Comment on attachment 671709 [details] [diff] [review]
Fix getInterface from JS on XMLHttpRequest objects to actually work.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 740069 
User impact if declined: Some extensions cause various functionality (the
   certificate exception dialog in this case, but other things could break too)
   to break for XMLHttpRequest network loads.
Testing completed (on m-c, etc.): Passes attached unit test, fixes attached
   extension.
Risk to taking this patch (and alternatives if risky): Risk should be low.  The
   test checks that the getInterface that used to work before in 16 still
   continue to work.
String or UUID changes made by this patch: None.

	Ed Morley [:emorley]
	Comment 19 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/c317ac04497a

	Peter Van der Beken [:peterv]
	Comment 20 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #13)
> Looking at XPCConvert, looks like XPCConvert::NativeData2JS in the
> nsXPTType::T_IID case uses xpc_NewIDObject, which calls nsJSID::NewID.  And
> going the other way, XPCConvert::JSData2Native in the nsXPTType::T_IID case
> does xpc_JSObjectToID, which basically also goes through nsIJSID.

This is probably because we only have one nsXPTType enum for both CID and IID (and they both get mapped to the native nsID type). We do have nsJSIID and nsJSCID classes too :-/.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 12 years ago

> We do have nsJSIID and nsJSCID classes too :-/.

Yes, but both QI to nsIJSID, so this is fine here.

It does mean you can try to getInterface a CID, but that's life.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 22 • 12 years ago

Comment on attachment 671709 [details] [diff] [review]
Fix getInterface from JS on XMLHttpRequest objects to actually work.

passes tests, low risk, approving for uplift.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 23 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/5aaba9acabc5
https://hg.mozilla.org/releases/mozilla-beta/rev/ba644e1f49e5