Background is bug 682329 and 739563. In short, SSL error reporting got removed, and nobody is helping to bring it back. Firefox doesn't want any SSL error reporting prompts whatsoever. Thunderbird and other applications do want them. I've been asked to find a solution for Thunderbird 17. But any solution for Thunderbird will require, that we have sufficient APIs in core that empower applications to perform the full error reporting. We don't have such API yet, and we would need it by Monday. As you know, new API discussion should be done carefully. Especially if we are just before a new ESR release branch. Therefore I would like to go the following path, which doesn't require new API, and can solve all problems. I will attach a patch that restores error reporting to PSM. Firefox can very easily be disable any such error reporting, using the simple patch attached in bug 744945. I understand the opinion that many people have raised, that Firefox should never show a error prompt popup, and that's exactly what the patch in bug 744945 will guarantee. But you must understand that Thunderbird does want the prompt, and as the module owner of PSM I have a responsiblity to find a general purpose solution. Given that nobody has actively written code to find a remedy for this situation, and given that forking PSM is not a good idea either, I would like to use my power as a module owner of PSM to land the approach I have just explained. You are free to implement a better solution in the Firefox 18 time frame, but I need to ensure that Thunderbird 17 will be fixed, and the only solution that *I* can do with the few resources I have available for this task, is by using the approach I have described. I understand that Brendan thinks these error dialogs are bad for Firefox. That's fine with me, and I have a simple patch that can be used to disable them. All I'm asking is that we land this compromise for Thunderbird 17, and postpone the better solution until Firefox 18. Firefox 17 will not have prompts. I'm open for a different solution, but only under the following conditions: People must actively work on a different solution for Thunderbird by monday, prior to the aurora merge. This would involve API enhancements and commitment to get such work done at the comm-central level. But I don't have time for that.

It has been suggested to implement error console logging. But there has been dispute regarding this approach, too. The patch I attach will allow Firefox to make its own decision, whether it wants console logging or silence. The patch for core that I'm going to attach will be a mix of attachment 653038 [details] [diff] [review] and attachment 654567 [details] [diff] [review] with some changes (I need to do some merging, so it will take another 2-3 hours before I attach it here).

Just a note: you're writing "by monday" as if nothing ever changed during aurora and beta development phases. This is far from true. We have uplifted much bigger changes on aurora (and even on beta).

(In reply to Mike Hommey [:glandium] from comment #2) > Just a note: you're writing "by monday" as if nothing ever changed during > aurora and beta development phases. This is far from true. We have uplifted > much bigger changes on aurora (and even on beta). I grant permission to replace my solution with a better solution during the aurora phase. But since nobody has stepped up or made an effort or a commitment to get such a better solution implemented, I won't wait for it. Given that I will have trouble landing things after the merge, I want to land this approach by Monday.

Kai's approach has already been rejected before in other bugs, and the appeal of that went all the way up to Brendan and was resolved Monday is not the deadline for this work. Everything that has been said about this has been said. Nobody on platform team should be wasting time dealing with this discussion when we have so much basecamp work to do. Bugzilla is not the right forum for this type of discussion. This is not a bug report and it isn't an enhancement request that isn't already covered by existing (WONTFIX'd or INVALID) bugs.

I'm the module owner, and the solution here is appropriate.

I propose to wait until I have a patch (soon). If Brendan then wants to reject this work, he should say so in this bug.

The patch I have attached is intended as an immediate remedy for the Mozilla 17 cycle, in order to have a solution for the Thunderbird 17 cycle. The patch has zero effect on Firefox. Prompts are disabled. Logging is disabled. (patch is on top of the patch from bug 783947). I would like to land this bug now, for the Mozilla 17 cycle.

Comment on attachment 655127 [details] [diff] [review] Patch v1 - ZERO EFFECT ON FIREFOX - implement prompts that applications can use by opting in Brendan, do you object, or can you tolerate this for the Mozilla 17 cycle? See previous comments. This patch has zero effect on Firefox 17. No prompts whatsoever, completely disabled by default. Logging to error console enabled (as done in bug 783974).

I know there have been suggestions that PSM should never show a dialog, and rather use a callback API, where PSM notifies an application that the user should be informed about an error. Any such solution would be very similar to this patch (which has all prompts disabled by default). Other approaches would differ from this patch only in the following way: Whenever we call ShowCertError() or displayAlert(), we'd simply have to call the API of a new API (which is not implemented by default). We can enable such applications who really want to have their own implementation of such error reporting, with a future follow-up patch, that provides a new callback interface. But I cannot see any harm by having this default implementation in PSM. We need it anyway, because only PSM can produce the correct error messages (localized). We don't want to go a path where each application has to produce their own implementation of error messages (the text).

merged to mozilla-central (still on top of the latest patch from bug 783974).

successful try server build for patch v2 https://tbpl.mozilla.org/?tree=Try&rev=bdba5cb070ce http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/kaie@kuix.de-bdba5cb070ce/

Ok. This patch implements the indirection that everyone has asked for. PSM will never show a patch on its own. Firefox will never show a SSL prompt popup. This patch requires that the application must register a callback that implements prompts. Opting in is no longer sufficient.

adding dependency to bug 783974, because the patch here is on top of the work from that bug.

(In reply to Brian Smith (:bsmith) from comment #4) > Nobody on platform team should be wasting time dealing with this discussion > when we have so much basecamp work to do. Well there is even a patch now - and it all sound very reasonable to me. Dismissing the right approach for all apps because firefox doesn't need it would be a slap in the face for everything non-firefox.

merged on top of final patch from bug 783974.

Comment on attachment 655310 [details] [diff] [review] patch v6 - introduce the indirection that everyone has asked for - never show a prompt from firefox nor from core psm OK, thanks a lot kaie, for the effort. This is definitely the right direction. Unfortunately, it's not what we had discussed. I had suggested specifically an nsISSLErrorListener2, which has a notifyError(in int errorCode, in wstring message); and that is registered per nsIChannel. It's fine to have an app-wide callback, but allowing only one app-wide UI for SSL errors doesn't cut it. Here are some use cases where we absolutely need callbacks per channel: - Thunderbird Account Setup Wizard We don't show any UI at the time of the connection [1], but save the error and - if needed - show it later in our own UI [2]. Here, we could do with the existing API, but they were cumbersome, and having an error string would help I think. Also, we still have issues with the error callback [3]. - App-updater - we want to abort the connection and kill it and show an error to the user, within the updater. - Firefox errors in a tab. Firefox can simple register the callback, and provide its own UI, per tab. For that, the callback needs a reference to the tab, which is typically saved in the callback object as member variable before it's passed to PSM. You see that one UI for the whole app isn't sufficient. We need different error treatment based on the code that opens the connection. If we show our own UI, we must save the UI context (pointer to the window and other things) in the callback, so that we can open the appropriate UI in the right place in case when we get called. The above are just some examples, there are probably many more variations. Apart from that, the nsISSLErrorAction IDL even mixes setting of flags and callbacks in one interface. We already have too many IDLs, it's already a problem and confusing for apps that we have nsIBadCertListerner and nsISSLErrorListener (see e.g. [4]). These should be united. Now you add yet another, which makes things even more confusing. If I would see this, having to use it, I'd tear my hairs out. I can't give this my review+. Although I had other plans for today, I'm willing to work with you to reshaape this patch to do/allow the above. [1] http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/guessConfig.js#893 [2] http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/emailWizard.js#1606 [3] http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/emailWizard.js#54 [4] http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/MyBadCertHandler.js

Here's what I had in mind for the interface.

Why do you guys think that what you're trying to do can't be done with the existing interfaces in PSM? A patch like this isn't useful--and is in fact harmful--for Firefox at all, as already explained in the other bug about this that got WONTFIXed by *TWO* PSM peers. A patch like this isn't needed for Thunderbird, as my PoC shows. All that is needed is a bugfix to PSM so that the nsISSLStatus instance gets passed to the nsISSLErrorListener. At best, such a patch simply spends Gecko/Firefox resources to make it slightly easier to get a horrible UI in non-Firefox, non-Thunderbird, XULRunner applications. Keep in mind that the nsIBadCertHandler gets called whenever there's an overridable cert error, and the nsISSLErrorListener gets called for all other errors on an SSL connection. Thus, a callback that implements both interfaces can get all errors. Secondly, elsewhere in other bugs it has been shown that it is possible to register such callbacks in a global way (at least Firefox extensions are able to do so) using *existing* functionality, without any special support from PSM. So, AFAICT, these patches are just adding redundant functionality in a way that makes PSM more difficult to maintain. Further, Kai's patch has the low-level PSM SSL networking code interacting with the docshell. I spent a significant amount of effort eliminating those docshell dependencies and I am only one patch away from completely eliminating them. It is important to avoid such dependencies because these dependencies on UI code make it difficult or impossible to test PSM using xpcshell and other UI-less testing frameworks. Also, none of these patches include tests. Kai went back and canceled a handfull of my review requests because they did not include automated tests, and I agree that he was right to do so, because that is standard practice in Gecko development. Patches such as these should have xpcshell or GTest tests with them.

bsmith, you're being destructive now, and highly disrespectful. I have explained above, in detail and with very concrete use cases that I encountered myself, even with code references, why we need this, and in the way I described. And why your proposal and kai's proposal do *not* work. There are dozens of XULRunner apps and thousands of Firefox extensions. The solution I show is fairly simple and generic, and is a very common design pattern. This patch cannot be harmful to Firefox, because 1) it's explicitly designed to now change behavior for Firefox at all 2) it gives Firefox more possibilities in the future.

> it's explicitly designed to now change typo: not change

> This patch cannot be harmful to Firefox If it adds docshell dependencies to the PSM guts, it's definitely harmful to Firefox.

> If it adds docshell dependencies to the PSM guts, it's definitely harmful to Firefox. OK, I can understand that. Note that my proposal has no such dependencies. And I believe it achieves the same goal without them. > Why do you guys think that what you're trying to do can't be done with the > existing interfaces in PSM? The error message must contain information helpful to diagnose the problem, and that includes variable replacements for hostname, cert names, ciphers (available and used), or other variables depending on the error. Static strings are not sufficient. > A patch like this isn't needed for Thunderbird, as my PoC shows. All that is needed > is a bugfix to PSM so that the nsISSLStatus instance gets passed to the nsISSLErrorListener. Could you please reference the relevant bug here, so that we have a solution for TB 17?

Here is what I wrote last week to Kai, bsmith, and Mark Banner: my primary interest is to restore the error message to the end user. The current situation that the user doesn't get any error, or an error without hint at the actual problem, is an untenable situation. (Such a bug must not exist, not even in one release, much less in an ESR. It costs users a horrible amount of time and makes the app completely broken for them with no way to remedy.) From a design perspective (having designed many applications and APIs before), I can see only one proper solution: * PSM allows the app to register an error callback on the nsIChannel * If an error occurs, PSM creates the error message string for the end user, already translated. (It's not feasible for any application to know - much less translate - all the errors that PSM can have. Also, please note that there are about 50 XULRunner apps, not just Thunderbird.) * The error string is passed via the callback to the application * The application creates the error UI. In case of Thunderbird, that's a dialog box. (In case of Firefox, it's some inline error UI in the relevant tab.) This is a reasonable API, one that we already have in other places and that's generally common, and most of all that's reasonable to implement for all parties. It's also straight-forward: * We already have nsISSLErrorListener, but it doesn't allow to pass an error string. But we just need to create nsISSLErrorListener2 which allows that. * That means that we can use the existing callbacks and there's no major redesign work. * I'd hope that PSM already has error string, and if not, PSM is the right place to add them, not Thunderbird and all other apps. * Adding an error dialog in Thunderbird should be straight-forward, too. For that to happen, it would be nice: * To get it done by Monday, to make TB 17. * bsmith to state that he's OK with the above proposal * Mark Banner to state that he's OK with it from Thunderbird perspective * kaie then said that he'd be willing to make the patch, from what I understood * Somebody to review the patch Guys? Can we get back together, please?

(In reply to Ben Bucksch (:BenB) from comment #21) > bsmith, you're being destructive now, and highly disrespectful. FWIW - I disagree. Brian's response is a fair argument against what he perceives to be a harmful technical choice. There is nothing destructive or disrespectful in it--he just strongly disagrees with you, which is fine.

Firefox will NEVER run through the DETECT_DOCSHELL codepath. The docshell piece was primarily intended for seamonkey, because it's an application that does both mail and browser. By removing it, seamonkey will either get no prompts at all, or duplicate prompts for errors already in the browser error page.

I'm OK to find an #ifdef that will disable the DocShell codepath for any build flavors that MozillaFirefox developers care about. Would that be acceptable? Can you propose on #ifdef that will enable the code for Thunderbird+SeaMonkey+XulRunner?

Kai, if we used the nsISSLErrorListener2, Seamonkey could enable the default prompts, as outlined in the code comments, and then, for browser connections, register its own nsISSLErrorListener2 that does the docshell stuff. That would keep the docshell stuff out of PSM, which I agree with bz is a good thing. Correct?

(i.e., seamonkey would already have the docshell pointer in the browser UI and pass it to the errorlistener implementation instance as member var.)

(In reply to Ben Bucksch (:BenB) from comment #17) > Unfortunately, it's not what we had discussed. I had suggested specifically > an nsISSLErrorListener2, which has a notifyError(in int errorCode, in > wstring message); > and that is registered per nsIChannel. When you make a call to Necko that does network I/O, the various Necko APIs will give you an nsresult. If the error was SSL-related, that nsresult will be in NS_ERROR_MODULE_SECURITY, and you can determine the NSPR/NSS error code by calculating -1 * NS_ERROR_GET_CODE(err). So, you can incorporate the error handling of SSL errors into your existing networking error handling code. This doesn't even require the nsISSLErrorListener/nsIBadCertHandler stuff *at all* and this is the approach I recommend all apps to use. > It's fine to have an app-wide callback, but allowing only one app-wide UI > for SSL errors doesn't cut it. I can see why it would be useful to register multiple callbacks for a given event. But, you can get the same effect in your code by using the chain-of-responsibility pattern or other design patterns to convert the single-callback-per-channel mechanism into multiple-callbacks-per-channel. Note that all of Necko's callbacks consistently allow only one callback to be registered per event, including the existing nsISSLErrorListener and nsIBadCertListener and the redirect handlers, and auth handlers, and everything else. There's nothing special about this particular kind of callback. > Here are some use cases where we absolutely need callbacks per channel: > - Thunderbird Account Setup Wizard > We don't show any UI at the time of the connection [1], but save the > error and - if needed - show it later in our own UI [2]. Can be done by using nsIBadCertHandler and/or nsISSLErrorListener. > Here, we could do with the existing API, but they were cumbersome, and > having an error string would help I think. Also, we still have issues > with the error callback [3]. First, the formatting of error messages is highly application-specific and I don't recommend that anybody depend on the formatting used by PSM, because it is subject to change without notice (as we've seen). Anyway, you should be able to get the error string from nsISSLStatus.errorMessage as part of your nsISSLErrorListener, but there's a bug in PSM that causes us to pass NULL as the nsISSLStatus parameter. This is the (only) bug that needs to be fixed to enable you to use the existing callback interface. But, again, I don't recommend you to use the callback interfaces anyway; I recommend you to handle SSL-related errors in the same place you handle other networking errors in your application. > - App-updater - we want to abort the connection and kill it and show an > error to the user, within the updater. PSM will always abort the connection when it encounters an SSL error, unless that error is a cert error that has been overridden by the SSL error override mechanism. Again, the error reporting here is the same as described everywhere else. > - Firefox errors in a tab. Firefox can simple register the callback, > and provide its own UI, per tab. For that, the callback needs a > reference to the tab, which is typically saved in the callback object > as member variable before it's passed to PSM. Firefox already provides the UI that we want to provide, using the technique that I described above--it sees it has a networking error, determines that the networking error was an SSL error, and then shows the SSL error page. In the future, we'll also log these errors to the web console, but that still won't require us to add any new callback interfaces to PSM, AFAICT. As far as UI for subresources go, we need to work with the Firefox UX team to determine what, if any, UI should be shown in those cases. > You see that one UI for the whole app isn't sufficient. We need different > error treatment based on the code that opens the connection. This is exactly what PSM already provides in multiple ways. > We already have too many IDLs Yes.

(In reply to Kai Engert (:kaie) from comment #28) > I'm OK to find an #ifdef that will disable the DocShell codepath for any > build flavors that MozillaFirefox developers care about. Would that be > acceptable? Can you propose on #ifdef that will enable the code for > Thunderbird+SeaMonkey+XulRunner? Kai, we've been around and around on this. We need to be optimizing things for Firefox. Adding dead code to Firefox and making it harder to maintain is the opposite of doing that. I've already described two different kinds of solutions for XULRunner applications that don't require us to do anything to PSM except fix a simple bug. Let's just fix that bug.

bsmith wrote: > you can determine the NSPR/NSS error code by calculating -1 * NS_ERROR_GET_CODE(err). Have you ever gotten an "File not found" error and not known which file the app means? There's no way to solve such an error. Static error messages don't cut it - as I have stated above. That's particularly true for hostname mismatch errors, cipher mismatches, and similar stuff that we have in PSM. > I can see why it would be useful to register multiple callbacks for a given event. > But, you can get the same effect in your code by using the chain-of-responsibility pattern > or other design patterns to convert the single-callback-per-channel mechanism into > multiple-callbacks-per-channel. Agreed, including your proposal to work around it. That's not what I meant. I meant that there are different places in one app that make SSL connections, and they need different handling or UI. For example, an ext needs a different handling than the browser. Or the TB Account Setup dialog needs a different handling than the normal IMAP code. This is why I said "per channel" (= TCP connection), not app wide (as Kai has it). > the formatting of error messages is highly application-specific and I don't recommend > that anybody depend on the formatting used by PSM, because it is subject to change > without notice (as we've seen). I didn't mean to parse the error message. That's nonsense, I agree. I mean: If the app doesn't know much about SSL or cert errors, just wants to *show* them to the user in its own UI, it should be able to get a user-readable error message string from PSM that it can just show verbatim to the end user. (In a prompt dialog, in a <div>, or whatever.) No parsing at all. The app just need the error message, including hostname, cert name or whatever is relevant to fix the problem. Only PSM knows how to construct a meaningful and helpful error message, including all details (variables) pertinent to the error. If the app needs to handle some errors specifically, the error code and other information that we currently already have and will still have can be used. But that's optional, and most apps don't need that. > PSM will always abort the connection when it encounters an SSL error It used to show the override dialog by default, and we didn't want to allow the user to override, in this particular case, for security reasons. Also, we had a case in Thunderbird Account Setup dialog where we didn't want the abort, we needed the connection to continue, and it was a huge problem, big mess, and we had several badly failing attempts. But that part is definitely for another bug. > Firefox already provides the UI that we want to provide From what I understood, the changes to make that happen caused the Thunderbird regression in the first place that we try to fix here. What I am saying is that with the proposed IDL, we wouldn't have had this problem in the first place. You could have just used this API. It would be trivial and nice for Firefox to implement whatever UI you want, or none at all. That said, any patch we have here shouldn't change Firefox browser behavior. It should make things easier for Firefox extensions, though. The current API is still horrible for callers - I've been there many times in the past, with many different projects. I'd like to get this straight for everybody now. > I've already described two different kinds of solutions ... > fix a simple bug. Let's just fix that bug. Again: Could you please reference them here, in terms of bug numbers?

I agree with Brian's comment 31 and comment 32. I previously wrote to Kai against #ifdefs that will rot on the untested side, and harbor bugs that we shouldn't have to spend another second on. I'm amazed by all the arguments about needing a new interface (by Monday) here. We have enough interfaces. We have actually one more than enough, but together, enough to cover the error space. We must not violate layering any longer, especially regarding docshell. Will the PSM bug that drops the nsISSLStatus parameter to nsISSLErrorListener? /be

> Will the PSM bug that drops the nsISSLStatus parameter to nsISSLErrorListener? "be fixed", of course? If that fix is made and the existing interfaces are enough, then this bug should be INVALIDated or WONTFIXed. /be

Brendan, how do we solve the following problem with the existing interfaces? Comment 33: > Have you ever gotten an "File not found" error and not known which file the app means? > There's no way to solve such an error. Static error messages don't cut it. > That's particularly true for hostname mismatch errors, cipher mismatches, and similar > stuff that we have in PSM. You cannot do this with the existing interfaces. The app can't do that without handling each such error explicitly. On top, having to calculate error code and using another API to get the error string is a cumbersome API. There are many callers to this, and they all have to do it, so it makes sense to make this simpler for apps. I think attachment 655623 [details] is a fairly minimal API that is used instead of 3 existing interfaces, and is more powerful (above quote).

(In reply to Brendan Eich [:brendan] from comment #35) > > Will the PSM bug that drops the nsISSLStatus parameter to nsISSLErrorListener? > > "be fixed", of course? > > If that fix is made and the existing interfaces are enough, then this bug > should be INVALIDated or WONTFIXed. That bug is bug 754369. It will be fixed. I will comment in that bug on the status of it.

(In reply to Ben Bucksch (:BenB) from comment #36) > Comment 33: > > Have you ever gotten an "File not found" error and not known which > > file the app means? > > There's no way to solve such an error. Static error messages don't > > cut it. That's particularly true for hostname mismatch errors, cipher > > mismatches, and similar stuff that we have in PSM. PSM will format an error message for you as part of nsISSLStatus.errorMessage, which contains the contextual information. Again, Firefox is able to maintain the context information it needs while using the existing error reporting API, so I don't see why it would be problematic for a XULRunner application to do the same. I admit it may require modifications to the XULRunner application, but that is required anyway because we don't provide a stable API for XULRunner apps or even Firefox extensions. > You cannot do this with the existing interfaces. The app can't do that > without handling each such error explicitly. > On top, having to calculate error code and using another API to get the > error string is a cumbersome API. There are many callers to this, and they > all have to do it, so it makes sense to make this simpler for apps. How many apps currently use this API is missing the point. This is not a stable API. When Firefox redoes its SSL error handling, we're likely to remove even the existing error formatting logic from PSM, because it will be dead code in Firefox, since Firefox is likely to format the error messages itself in code under browser. The formatting of error messages is part of the UI and it doesn't belong in networking code. It is only still living in PSM now because we haven't improved the SSL error handling in Firefox yet. Put another way, the more UI-related stuff you rely on PSM doing, the more likely your app is likely to break in the future. Consequently, it is a good idea to just bite the bullet now and keep all your UI logic in your UI.

> That bug is bug 754369. It will be fixed. I will comment in that bug on the > status of it. In time for Thunderbird 17? That bug 754369 isn't a fix for the issue we're trying to solve, it's simply a blocker to your proposed new alternative implementation for error reporting. Your approach requires new Thunderbird specific code, and I don't have time to work on that. You shouldn't reject my non-Firefox-harming compromise patch unless you have someone who'll actually be able to implement the "better" approach.

(In reply to Brian Smith (:bsmith) from comment #38) > > How many apps currently use this API is missing the point. This is not a > stable API. When Firefox redoes its SSL error handling, we're likely to > remove even the existing error formatting logic from PSM, because it will be > dead code in Firefox, since Firefox is likely to format the error messages > itself in code under browser. The formatting of error messages is part of > the UI and it doesn't belong in networking code. It is only still living in > PSM now because we haven't improved the SSL error handling in Firefox yet. > > Put another way, the more UI-related stuff you rely on PSM doing, the more > likely your app is likely to break in the future. Consequently, it is a good > idea to just bite the bullet now and keep all your UI logic in your UI. Words like that may be appropriate for a new long development cycle. But I think they are inappropriate in the last minute of a develpoment cycle towards an ESR release.

> This is not a stable API. "Not stable" doesn't mean "we'll deliberately break stuff". That policy was always combined with "we'll do our best to keep stuff running". What you say conflicts with that. Please note that Firefox extensions are part of Firefox as well, not just the browser. They are one big reason why people use Firefox. Callers cannot have the knowledge how to create error messages. In *any* good architecture, be it client/server or app/library, the component which has the error is responsible for creating the error messages, including translating and replacing variables ("formatting"). Even HTTP comes with error messages next to the error code. Apps that use libraries that don't follow that rule are constantly broken, because the set of error messages changes, and no caller can format every single error message, and keep them up to date over the years. (Well, 1 caller could, but not 10.) The error message generation must be centralized. And to be easy to use, the error message should come next to the error code in the callback.

(In reply to Ben Bucksch (:BenB) from comment #41) > "Not stable" doesn't mean "we'll deliberately break stuff". That policy was > always combined with "we'll do our best to keep stuff running". What you say > conflicts with that. Ben, I understand that there is some disagreement on what the support policies are and how we apply those policies to our work together. Rather than debate the policy here in this bug, let's figure out a way to get the policy clarified so that we can avoid wasted effort and hurt feelings in the future.

> PSM will format an error message for you as part of nsISSLStatus.errorMessage, > which contains the contextual information. That's nice. However, while nsIBadCertListener has an nsISSLStatus parameter, nsISSLErrorListener doesn't. Where would I get my error message with contextual information in this case? > policy clarified I don't see what that would gain. Let's stay concrete: Either we do stuff that helps callers here in this case or we don't. With 13 years of Mozilla experience, including using these very interfaces in various kinds of projects, I think I can judge fairly well myself what's a fair tradeoff and what's not. I'm not a party to this discussion, either, I just came here to help, but I don't see any willingness.

Well, bsmith keeps saying that he has a simple solution, so instead of talking in hot air, I think it's best to wait for that and see how the Thunderbird change looks like. Because all other callers will need this as well. If it 1) is simple enough (about 10 lines of code) 2) is maintainable / stable (doesn't need any forseeable changes in the future) 3) can produce translated error message strings with context information, which the app can show in its own UI I guess we'll just document that in the IDL and MDC and we can close this bug. If that's not possible, then we'll change PSM to make it possible. We'll wait for bug 754369 and bug 739563. (Please note that attachment 619514 [details] [diff] [review] fails 1) and 3) above.) Fair?

No Brian, we don't need a comittee. Either we're willing to work together on a compromise, or we are not. Dictating how other people shall work around the hurdles you have created, isn't a compromise. Words don't fix bugs. You broke it. You propose to do things differently, but you don't work on the implementation of that different proposal. The different proposal is even blocked by other work, and you haven't fixed that blocker yet. It's not even straightforward to get that blocker fixed, because it depends on a third set of work you "plan" to do. That doesn't sound like a viable path for a last minute fix for the Thunderbird 17 cycle. If Mozilla is still a place for constructive multi-project collaboration, then please stop fighting with arguments, and help to work towards a constructive solution with patches. That's what I did, but all I get is rejection. That's not fun. This attitude is hurting my motivation to contribute.

And a solution, that instead of having a global handler, requires that each of smtp, pop3, imap, ldap, http, and each of the chat protocols to register their own handler, isn't a solution that I have time to implement.

To summarize briefly: In the past we had a global, catch all, fallback error reporting mechanism. I've attempted in numerous iterations to restore such a mechanism. I believe the latest patch is reasonable, because it has zero harm for Firefox. The alternative that has been proposed (not by me) is to solely use the channel-specific listeners nsISSLErrorListener and nsIBadCertListener2. This isn't an equivalent solution, because it requires a much greater effort in the various application. Applications would have to ensure to identify each and every place in the code where channels are being constructed, and register appropriate listeners. Given the amount of work this alternative would require, and given the few resources we have, I consider the breakage and the removal of the central mechanism a major regression. If you want to help to find a remedy to this solution, then please help to find a remedy that makes a global, catch all error reporting mechanism possible. While Ben appreciates the flexibility of the channel-specific solution, in my understanding, he agrees with me that a global catch-all reporting is necessary, too. Unless we work together to find a solution for a global catch-all error reporting mechanism, that applications can easily enable in just one place, applications like Thunderbird et.al. will appear to be broken with no end user visible indication.

... and because the primary development phase of mozilla 17 is already over, any solution for this dilemma must be acceptable to the aurora drivers, and therefore we should strive to find a compromise that doesn't introduce new side effects. I believe Mozilla has given a promise to produce a usable Thunderbird 17 ESR release. Let's work together to find a solution that fulfils this promise.

Kai, the proposal I made in attachment 655623 [details] allows both channel-specific handlers as well as a global one, with the same API. The text in the IDL describes how.

(In reply to Ben Bucksch (:BenB) from comment #49) > Kai, the proposal I made in attachment 655623 [details] allows both > channel-specific handlers as well as a global one, with the same API. The > text in the IDL describes how. Your IDL will have to be combined with a new contract ID, that the application could implement. And we should avoid the new sideeffect that your boolean return value allows. This should be done later, because it will further complicate this discussion. But the major question is, will we get support in getting this approach implemented and accepted? I don't want to work on this further, unless we get some statements under which circumstances a new patch with a global fallback will be accepted.

> Your IDL will have to be combined with a new contract ID, that the application could implement. I had thought that PSM provides that default implementation, for all callers to use. It merely takes the string and pops up an nsIPromptService.prompt(). Trivial. > And we should avoid the new sideeffect that your boolean return value allows. > This should be done later, because it will further complicate this discussion. Completely agreed. You can just comment that the return value is ignored for now, with bug# that will implement it. > will we get support ... ? Whose support are you looking for, exactly? (You're the module owner, after all.)

Actually, Kai, I think it's best to wait for what bsmith has in the making. And we use that for Gecko/TB 17. Only if that doesn't come through in time, or it's not a viable API for callers, I'd continue with the approach discussed here. So, let's just wait for bsmith, he should have a patch shortly.

Kai and BenB, please stop over-commenting. Bugzilla is not a newsgroup or server-side forum. An echo chamber is just going to drive away people who do not agree. BenB: your assertion that errors needing to be string-converted and formatted and localized at the site they where they are raised is false on its face. Many systems, including Unix and libraries built on it including Sun NFS and RPC/XDR, POSIX libraries, and oodles of JS libraries, pass back structured (unformatted) error information that an outer-layer client front-end then formats in localized fashion. Kai: if you are willing to hack on code in one place, why not in another? At bottom this argument is about whether PSM should be putting up error dialogs. I've already ruled against that, several times. You wrote: "If Mozilla is still a place for constructive multi-project collaboration, then please stop fighting with arguments, and help to work towards a constructive solution with patches." I am here to say that collaboration must involve "fighting with arguments", not just "make everyone happy" patching. I like win-win; I'm Captain Kirk in STII:TWoK -- I don't believe in the no-win scenario. But this does *not* mean adding interfaces and implementation glue to support all extension or error reporting scenarios, if the existing interfaces (with a fix to propagate error details) can suffice. To avoid bugs and misfeatures, we must minimize extensibility and interface overhead. This is part of the job. Minimization must involve rational argument. /be

(In reply to Brendan Eich [:brendan] from comment #53) > > Kai: if you are willing to hack on code in one place, why not in another? It's not a question about place, but about amount of work. I can do an implementation at one place that handles everything. I don't have time to track down and maintain many places in various Mozilla based applications. > At > bottom this argument is about whether PSM should be putting up error > dialogs. I've already ruled against that, several times. an I understand, and I accept your wish, and my patch v8 in this bug never puts up error dialog. All it does is notifying an app with all the necessary information, and the app can decide if it puts up something or not. Firefox will never put up a dialog with patch v8.

1. Discussion Brendan, I agree with your win-win idea, finding the best solution. But that solution must satisfy all needs, not one party telling the other to get lost ("you're less important" etc.). 2. Error strings with details You're right that many Unix libs report errors as numbers. The result is that they often give very generic errors that are not helpful, and some logging, but that doesn't work at all with common end users. They are horrible to use even for developers, I've experienced that too often myself, it was highly painful. So that's a real problem and it's simple to see why: If you have detailed error reporting, then many new functions will add new error cases. All callers then will need to handle these new errors in the new lib version. Because the callers can't constantly add new code or translated strings, and the lib can't rely on the app adding them, there's a problem. If a library is used by 100 applications, these 100 developers can't all add the error strings, in the same pace as the library - that just doesn't work, it doesn't scale, it's impossible in practice. OTOH, it's simple for the lib developer to add the error string to a list of his own, and the lib returns that string to the app via a lib-wide mechanism. 3. PSM putting up dialogs In my proposal, the application *explicitly* tells PSM to use a certain implementation (by XPCOM pointer) to handle the errors. PSM can provide a default implementation for simplicity, or the app provides it. 4. Interfaces > To avoid bugs and misfeatures, we must minimize extensibility and interface > overhead. This is part of the job. Minimization must involve rational argument. In my proposal, there are less interfaces than today. We have 3-4 today, then we'd have 2-3. And they are simpler and clearer, easier to use. I've used the current interfaces in the past, and I was very confused. For example: (In reply to Brian Smith (:bsmith) from comment #31) > you should be able to get the error string from > nsISSLStatus.errorMessage as part of your nsISSLErrorListener a) nsISSLErrorListener doesn't give any nsISSLStatus, only nsIBadCertListener2 (so for some errors, I can't get it) b) nsISSLStatus doesn't have any attribute errorMessage http://mxr.mozilla.org/comm-central/source/mozilla/security/manager/ssl/public/nsISSLStatus.idl Just returning a string next to the error code is 1) no problem at all for IDL, luckily it can do strings without problem 2) simple to implement in PSM (in fact, we already had that. The issues were UI access and DOM access, not the strings.) 3) makes life a *lot lot* easier for callers (apps and exts). --- I hope these were rational arguments.

(In reply to Ben Bucksch (:BenB) from comment #55) > 1. Discussion > > Brendan, I agree with your win-win idea, finding the best solution. But that > solution must satisfy all needs, not one party telling the other to get lost > ("you're less important" etc.). "get lost" != "Firefox is highest priority". I think you know this, so stop exaggerating or actually misstating. Last warning from me. Priotizing is necessary and also part of the process in any healthy open source project. We don't get to serve all priorities equally. Some are lower than others. > 2. Error strings with details No need to generalize about Unix, we already established that your claim that formatting must happen at throw site was false :-|. If not enough structured data was included in the exception info, that's a bug to fix (see below). > 3. PSM putting up dialogs > > In my proposal, the application *explicitly* tells PSM to use a certain > implementation (by XPCOM pointer) to handle the errors. PSM can provide a > default implementation for simplicity, or the app provides it. PSM in Firefox should provide no implementation. > 4. Interfaces > > > To avoid bugs and misfeatures, we must minimize extensibility and interface > > overhead. This is part of the job. Minimization must involve rational argument. > > In my proposal, there are less interfaces than today. We have 3-4 today, > then we'd have 2-3. And they are simpler and clearer, easier to use. I'll let Brian comment on this counting exercise, but observe that we need a fix soon for the proximate problem, and we can always stage work to perfect the interfaces later. What we must not do in any timeframe is add redundant interfaces, or implementations in the wrong place. > I've used the current interfaces in the past, and I was very confused. For > example: > (In reply to Brian Smith (:bsmith) from comment #31) > > you should be able to get the error string from > > nsISSLStatus.errorMessage as part of your nsISSLErrorListener > > a) nsISSLErrorListener doesn't give any nsISSLStatus, only > nsIBadCertListener2 > (so for some errors, I can't get it) > b) nsISSLStatus doesn't have any attribute errorMessage > > http://mxr.mozilla.org/comm-central/source/mozilla/security/manager/ssl/ > public/nsISSLStatus.idl Did you read comment 37? /be

(In reply to Ben Bucksch (:BenB) from comment #55) > In my proposal, there are less interfaces than today. We have 3-4 today, > then we'd have 2-3. And they are simpler and clearer, easier to use. I am open to the idea of simplifying the existing API that Gecko exposes to applications (through PSM) at some point in the future. I agree that the current interfaces are confusing. But, that kind of refactoring is not needed to solve these problems. Also, I know existing addons use the existing API and I'm not sure what the compatibility impact would be to replace the existing API. > (In reply to Brian Smith (:bsmith) from comment #31) > a) nsISSLErrorListener doesn't give any nsISSLStatus, only > nsIBadCertListener2 This is the bug referenced in comment 37; see bug 754369, and see my next comment. > (so for some errors, I can't get it) > b) nsISSLStatus doesn't have any attribute errorMessage Sorry for the confusion. When your nsISSLErrorListener implementation is called, the first parameter passed to it is socketInfo, which is a nsIInterfaceRequestor. You can QI that to a nsITransportSecurityInfo and then obtain formatted-for-Firefox nsITransportSecurityInfo.errorMessage from it. If you're happy to use the formatted-for-Firefox error message, then great, you're done, and we don't even have to fix bug 754369 for 17 ESR. (Note that the formatted-for-Firefox error message may contain HTML tags. If the message is fine except for the HTML tags, then your application can strip the HTML tags from the error message before processing it. See [1]) Otherwise, if Firefox's formatting is not appropriate for your application, your application can format its own error message using the information provided by QIing the socketInfo to nsISSLStatusProvider and then accessing the structured SSL error info from the the nsISSLStatus, after we fix bug 754369. The thing to keep in mind is that Firefox is likely to format its own error messages in the future, so you should plan on having nsITransportSecurityInfo.errorMessage go away completely. In fact, all of this API is likely to change substantially in the not-too-distant future. But, this removal won't happen in the ESR timeframe, so if you need an immediate hack, relying on nsITransportSecurityInfo.errorMessage should be acceptable based on what I've seen requested. [1] <Mook_as> (new DOMParser()).parseFromString("<a href=asdf>foo</a>", "text/html").body.textContent is probably ridiculously over-engineered. <gavin> https://developer.mozilla.org/en-US/docs/DOM/DOMParser#Parsing_an_SVG_or_HTML_document

(In reply to Brian Smith (:bsmith) from comment #57) > This is the bug referenced in comment 37; see bug 754369, and see my next > comment. s/my next comment/the rest of this comment/ > You can QI that > QIing the socketInfo to nsISSLStatusProvider Rather, you must use getInterface, not QueryInterface, as you would with any nsIInterfaceRequestor.

Thanks, bsmith, for this constructive comment. This is the way to talk and make progress. (In reply to Brian Smith (:bsmith) from comment #57) > I am open to the idea of simplifying the existing API that Gecko exposes to > applications (through PSM) at some point in the future. I agree that the > current interfaces are confusing. OK, great. I'd keep this bug open for that, until we reach a consensus - alternatively, we could discuss on the newsgroup -, then file a clean new implementation bug, with a summary of the rationale. > socketInfo ... You can QI that to a ... nsITransportSecurityInfo.errorMessage > ... your application can strip the HTML tags from the ... formatted-for-Firefox error message OK. That's maximum obscure and arcane, but if that works, that sounds like a good workaround for TB 17. Is this available for both nsISSLErrorListener and nsIBadCertListener? I really don't think that's a proper solution, though, even mid-term. Esp. since you say it's going to go away anyway (in fact, I'd be happy for such an API to go away :) ).

(In reply to Ben Bucksch (:BenB) from comment #59) > Thanks, bsmith, for this constructive comment. This is the way to talk and > make progress. I know I should let this statement go, but: On March 27, 2012 (5 months ago), I not only described this technique, but I wrote a PoC patch for Thunderbird that uses this technique to report errors in a dialog box, in bug 739563. The attachment is labeled "Example modification showing how to add default cert error behavior back to Thunderbird." > > socketInfo ... You can QI that to a ... nsITransportSecurityInfo.errorMessage > > ... your application can strip the HTML tags from the ... formatted-for-Firefox error message > > OK. That's maximum obscure and arcane I'd say "welcome to XPCOM" but you're more experienced with it than I am :). > Is this available for both nsISSLErrorListener and nsIBadCertListener? Yes. Both callbacks get the same kind of socketInfo parameter. Also, the second parameter to nsIBadCertListener2 is sslStatus, which is just redundantly socketInfo.SSLStatus.

> bug 739563. The attachment Yes, I've looked at that. It wasn't clear to me that the |msg| variable alone is sufficient, given that you're using a nsSSLCertErrorDialog from PSM. Now it is clear, thanks. > I'd say "welcome to XPCOM" heh. Stuff like that is fairly common in Microsoft COM. I'm glad we didn't repeat that mistake in Mozilla. Luckily, that is not the case for the vast majority of XPCOM interfaces. (PSM being an exception.) Completely undocumented interfaces are bad in any technology. > Yes. Both callbacks get the same kind of socketInfo parameter. OK, good.

(In reply to Ben Bucksch (:BenB) from comment #61) > Yes, I've looked at that. It wasn't clear to me that the |msg| variable > alone is sufficient, given that you're using a nsSSLCertErrorDialog from > PSM. Now it is clear, thanks. Right. If you want to use nsSSLCertErrorDialog, then you need the nsISSLStatus, which is what bug 754369 is about, and why bug 754369 is blocking bug 739563. Note that nsSSLCertErrorDialog should be removed from PSM when we have time, because it is dead code in Firefox and because PSM shouldn't provide any default UI for this stuff. (I already wrote the patch, when I removed the usages of nsSSLCertErrorDialog from Gecko, but it got r-d. But, given the resolution of this bug, I am going to re-attach it to a new bug.) I will CC you, Kai, and Thunderbird maintainers on the bug to remove it so you are aware of it. I think that somebody is likely to copy nsSSLCertErrorDialog into comm-central so Thunderbird can continue to use it, as mbanner still wants to use that UI. > majority of XPCOM interfaces. (PSM being an exception.) Completely > undocumented interfaces are bad in any technology. In a previous comment you wrote: > I really don't think that's a proper solution, though, even mid-term. > Esp. since you say it's going to go away anyway (in fact, I'd be happy > for such an API to go away :) ). This is exactly why I don't write patches to document these interfaces--to minimize the chance that people will increase the compatibility burden that will make removing them or improving them. This is also why I recommended many times in various bugs (including this one) that people use the technique that Firefox uses to handle SSL errors: get the (nsresult, nsITransportSecurityInfo/nsISSLStatuProvider) pair from Necko, determine if its a cert error or SSL error, if so present UI using your own formatting. That is the most stable API that you are going to get out of Gecko for SSL error handling or network error handling in general. I understand that formatting SSL errors is tedious and error-prone. On the other hand, Firefox isn't exactly doing a great job either, so if you think that showing really specific SSL errors is important, I'd not use Firefox messages. And, if you do not think specific SSL errors are important, then "The certificate is bad (error code 0x12345678)" or "Could not establish a secure connection (error code 0x12345678)" is actually not too bad for most applications. Also, there's not much of a "business case" or "product requirement" for creating said API, AFAICT, so I wouldn't be surprised if we eventually removed nsISSLErrorListener and/or nsIBadCertListener2 without replacing them with any kind of callback, since applications can get the information they need through the (nsresult, nsITransportSecurityInfo/nsISSLStatusProvider) pair provided by the Necko channel APIs.

I've merged the patch to the version 17 branch, allowing consumers of the Mozilla 17 platform to apply the patch locally, if desired. I understand that Firefox developers doesn't want this patch and that it therefore won't be applied to the code. If Thunderbird developers would like this fix, they can ship their own patched version of Gecko.

I'm closing this as incomplete, because we couldn't reach an agreement, and I'm unable to work on this further. Regards.