Last Comment Bug 675221 - Remove XPCOM proxies
: Remove XPCOM proxies
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Benjamin Smedberg [:bsmedberg]
:
:
Mentors:
: 619294 674571 674597 689107 (view as bug list)
Depends on: 675407 675702 679036 679140 680818 708813 708935 712363 717491 719371 725016 726967 727367 746644
Blocks: 436379 582120 616072 663291 684711 692605 803856
  Show dependency treegraph
 
Reported: 2011-07-29 08:51 PDT by Benjamin Smedberg [:bsmedberg]
Modified: 2013-12-06 05:15 PST (History)
30 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part A - fix XPCOM code which used proxies, rev. 1 (19.11 KB, patch)
2011-07-29 08:55 PDT, Benjamin Smedberg [:bsmedberg]
bzbarsky: review+
Details | Diff | Splinter Review
Part B - remove the implementation, rev. 1 (19.11 KB, patch)
2011-07-29 08:56 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Splinter Review
Part B - remove the implementation, rev. 1 for real (121.98 KB, patch)
2011-07-29 09:12 PDT, Benjamin Smedberg [:bsmedberg]
doug.turner: superreview+
Details | Diff | Splinter Review
Part C - fix up necko, rev. 1 (18.14 KB, patch)
2011-07-29 12:53 PDT, Benjamin Smedberg [:bsmedberg]
jduell.mcbugs: review+
benjamin: checkin+
Details | Diff | Splinter Review
Part D - url classifier, rev. 1 (26.99 KB, patch)
2011-07-29 13:02 PDT, Benjamin Smedberg [:bsmedberg]
dcamp: review+
benjamin: checkin+
Details | Diff | Splinter Review
Part E - DOMStorage and miscellaenous includes, rev. 1 (4.57 KB, patch)
2011-07-29 13:02 PDT, Benjamin Smedberg [:bsmedberg]
honzab.moz: review+
benjamin: checkin+
Details | Diff | Splinter Review
PSM synchronous proxies, part F, rev. 1 (110.00 KB, patch)
2011-08-09 08:40 PDT, Benjamin Smedberg [:bsmedberg]
brian: review-
Details | Diff | Splinter Review
Part G: Define HAS-STDCALL for MSVC so that nsRunnableMethod specializes correctly, rev. 1 (559 bytes, patch)
2011-08-09 08:42 PDT, Benjamin Smedberg [:bsmedberg]
khuey: review+
benjamin: checkin+
Details | Diff | Splinter Review
Implement Runnable that can be waited on synchronously, for removing XPCOM proxies (5.75 KB, patch)
2011-09-23 10:16 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
benjamin: review+
brian: checkin+
Details | Diff | Splinter Review
Enforce that NSS dialogs are only shown via code that runs on the main thread (10.30 KB, patch)
2011-09-23 10:28 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Notify observers of PSM's background processes on the main thread (10.39 KB, patch)
2011-09-23 10:30 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Remove XPCOM Proxies: cleanup PRBool -> bool conversion (188.13 KB, patch)
2011-10-02 02:54 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
honzab.moz: review-
Details | Diff | Splinter Review
Notify observers of PSM's background processes on the main thread [v2] (10.45 KB, patch)
2011-10-02 05:43 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Enforce that NSS dialogs are only shown via code that runs on the main thread [v2] (10.39 KB, patch)
2011-10-02 05:45 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Do PKCS#11 token password prompting on the main thread (6.22 KB, patch)
2011-10-02 23:53 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Do SSL client authentication on the main thread only (16.53 KB, patch)
2011-10-03 00:28 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Enforce that NSS dialogs are only shown via code that runs on the main thread [v3] (16.47 KB, patch)
2011-10-06 17:23 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Do SSL client authentication on the main thread only [v2] (15.28 KB, patch)
2011-10-06 17:24 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Notify observers of PSM's background processes on the main thread [v2] (9.53 KB, patch)
2011-10-06 17:25 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert (7.70 KB, patch)
2011-10-25 17:26 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
kaie: review-
Details | Diff | Splinter Review
Remove XPCOM Proxies from PSM UI prompts (62.64 KB, patch)
2011-10-25 17:34 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
kaie: review-
Details | Diff | Splinter Review
Notify observers of PSM's background threads on the main thread (9.74 KB, patch)
2011-10-25 17:36 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
kaie: review+
Details | Diff | Splinter Review
Remove XPCOM Proxies: Do PSM client certificate processing on the main thread (16.97 KB, patch)
2011-10-25 17:38 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
kaie: review+
brian: checkin+
Details | Diff | Splinter Review
Remove XPCOM proxy header #includes from PSM (1.42 KB, patch)
2011-10-25 17:42 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
kaie: review+
brian: checkin+
Details | Diff | Splinter Review
NOT FOR CHECKIN: Changes to nsNSSCertificate, ignoring indention changes (5.62 KB, patch)
2011-10-26 02:54 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Notify observers of PSM's background threads on the main thread [v2] (10.00 KB, patch)
2011-10-30 17:01 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Notify observers of PSM's background threads on the main thread [v3] (11.07 KB, patch)
2011-10-30 17:32 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
kaie: review+
Details | Diff | Splinter Review
Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert [v2] (8.79 KB, patch)
2011-11-02 00:06 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
honzab.moz: review+
brian: checkin+
Details | Diff | Splinter Review
Remove XPCOM Proxies from PSM UI prompts [v2] (63.05 KB, patch)
2011-11-02 00:33 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
brian: review+
brian: checkin+
Details | Diff | Splinter Review
Notify observers of PSM's background threads on the main thread [v4] (11.06 KB, patch)
2011-11-02 21:17 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
brian: review+
brian: checkin+
Details | Diff | Splinter Review
Addendum to part A - reimplement recursion checking for the console service, rev. 1 (4.31 KB, patch)
2012-01-09 12:56 PST, Benjamin Smedberg [:bsmedberg]
bent.mozilla: review+
Details | Diff | Splinter Review
Fix a test which uses console listeners incorrectly, rev. 1 (1.04 KB, patch)
2012-01-10 06:07 PST, Benjamin Smedberg [:bsmedberg]
bzbarsky: review+
Details | Diff | Splinter Review

Description Benjamin Smedberg [:bsmedberg] 2011-07-29 08:51:27 PDT
XPCOM proxies are source of persistent pain, deadlocks, and overly complex code. I believe that they are completely unnecessary given that we've forbidden their use for multithreaded JS code, and binary code can just write compled runnables.
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-07-29 08:55:16 PDT
Created attachment 549390 [details] [diff] [review]
Part A - fix XPCOM code which used proxies, rev. 1
Comment 2 Benjamin Smedberg [:bsmedberg] 2011-07-29 08:56:06 PDT
Created attachment 549391 [details] [diff] [review]
Part B - remove the implementation, rev. 1
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-07-29 09:05:12 PDT
Looks like part A and B are the same patch.
Comment 4 Benjamin Smedberg [:bsmedberg] 2011-07-29 09:12:33 PDT
Created attachment 549394 [details] [diff] [review]
Part B - remove the implementation, rev. 1 for real
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-07-29 11:43:10 PDT
I won't get to carefully reading part A till Monday, but aren't there lots more callsites in the tree?  In particular, all over PSM and in some places in necko....
Comment 6 Benjamin Smedberg [:bsmedberg] 2011-07-29 11:47:20 PDT
Yes, I have separate patches for various other pieces. Necko wasn't too hard, I'm working on urlclassifier now, and PSM will be the bitch. Immediate review isn't necessary, I'm targetting this at FF9.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-29 11:48:34 PDT
bsmith had talked about making PSM more sane here.
Comment 8 Benjamin Smedberg [:bsmedberg] 2011-07-29 12:53:43 PDT
Created attachment 549444 [details] [diff] [review]
Part C - fix up necko, rev. 1
Comment 9 Benjamin Smedberg [:bsmedberg] 2011-07-29 13:02:11 PDT
Created attachment 549448 [details] [diff] [review]
Part D - url classifier, rev. 1

dcamp, I'm going to tag you with this, let me know if you won't be able to do this soon or just redirect it to the right person.
Comment 10 Benjamin Smedberg [:bsmedberg] 2011-07-29 13:02:49 PDT
Created attachment 549449 [details] [diff] [review]
Part E - DOMStorage and miscellaenous includes, rev. 1
Comment 11 Honza Bambas (:mayhemer) 2011-07-29 13:37:59 PDT
Comment on attachment 549449 [details] [diff] [review]
Part E - DOMStorage and miscellaenous includes, rev. 1

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

r=honzab
Comment 12 Benjamin Smedberg [:bsmedberg] 2011-08-09 08:40:31 PDT
Created attachment 551776 [details] [diff] [review]
PSM synchronous proxies, part F, rev. 1

This patch for PSM keeps synchronous proxies, but assumes that the main thread never blocks on the crypto/PSM thread, and so it is never necessary to spin a nested event loop or do event filtering.
Comment 13 Benjamin Smedberg [:bsmedberg] 2011-08-09 08:42:53 PDT
Created attachment 551778 [details] [diff] [review]
Part G: Define HAS-STDCALL for MSVC so that nsRunnableMethod specializes correctly, rev. 1
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-09 08:46:26 PDT
Comment on attachment 551778 [details] [diff] [review]
Part G: Define HAS-STDCALL for MSVC so that nsRunnableMethod specializes correctly, rev. 1

Heh, I was wondering if anyone was ever going to need this (https://bugzilla.mozilla.org/show_bug.cgi?id=622728#c4)
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-08-09 08:48:31 PDT
> but assumes that the main thread never blocks on the crypto/PSM thread

Why is that an OK assumption?  I mean, the main thread could go and do a sync XHR to an https URL, no?  Or is that not blocking for our purposes here?
Comment 16 Benjamin Smedberg [:bsmedberg] 2011-08-09 08:51:36 PDT
I don't think that's blocking for our purposes, because sync XHR still runs an event loop and can therefore keep processing the runnable which will be posted to it.
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2011-08-09 08:54:33 PDT
Comment on attachment 551776 [details] [diff] [review]
PSM synchronous proxies, part F, rev. 1

Drive-by nits:

>--- a/security/manager/boot/src/nsSecureBrowserUIImpl.cpp
>+++ b/security/manager/boot/src/nsSecureBrowserUIImpl.cpp
>+  my_result = new SecurityWarningDialogsProxy(my_result);
>+  

Trailing spaces.

>+  my_result.swap(*result);

This can be forget()

>--- a/security/manager/ssl/src/nsNSSComponent.cpp
>+++ b/security/manager/ssl/src/nsNSSComponent.cpp
>+        nsCOMPtr<nsIPrompt> proxyPrompt = new PromptProxy(prompter);
>+        proxyPrompt.swap(*reinterpret_cast<nsIPrompt**>(result));
>--- a/security/manager/ssl/src/nsSDR.cpp
>+++ b/security/manager/ssl/src/nsSDR.cpp
>+  nsCOMPtr<nsIPrompt> proxyPrompt =
>+    new PromptProxy(prompter);
>+  proxyPrompt.swap(*reinterpret_cast<nsIPrompt**>(result));

And these
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2011-08-09 09:22:08 PDT
Hmm.  So what would blocking for our purposes be?  Something that posts a runnable to the PSM thread and then waits for it to be processed without spinning the event loop (e.g. a busy wait or waiting on a monitor)?
Comment 19 Benjamin Smedberg [:bsmedberg] 2011-08-09 09:41:46 PDT
Yes.
Comment 20 Mozilla RelEng Bot 2011-08-10 11:19:48 PDT
Try run for 66668bd29884 is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=66668bd29884
Results (out of 153 total builds):
    exception: 3
    success: 79
    warnings: 66
    failure: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bsmedberg@mozilla.com-66668bd29884
Comment 21 Benjamin Smedberg [:bsmedberg] 2011-08-10 13:54:03 PDT
There appears to be a necko behavior change with these patches: in test_bug497578.js a channel is opened to a port where there is no server and then cancelled (I think?)... the previous behavior was .onStopRequest called with NS_ERROR_BINDING_ABORTED, but the new behavior is NS_ERROR_CONNECTION_REFUSED. I'm still investigating, this may be related to the two other xpcshell test failures I'm looking at also.
Comment 22 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-14 12:12:45 PDT
Ben, I am working on other bugs related to PSM threading issues and XPCOM proxies in PSM. Much of the work on PSM can be simplified by changing the calling code (the code that constructs the proxies) so that it itself runs on the main thread. In many such cases, multiple XPCOM proxies can be eliminated together with a single Runnable. Once we do that in a few key areas (e.g. certificate chain verification failure), we will be able to audit other parts of the PSM codebase to see if all the remaining callers (including in SeaMonkey and Thunderbird) are already on the main thread. I thnk that when we do this, we see that we can replace quite a few of the uses of XPCOM proxies with direct invocations (in particular, I think this is the case for some of the use of XPCOM proxies in the dialog/prompting code).
Comment 23 Benjamin Smedberg [:bsmedberg] 2011-08-15 08:35:08 PDT
Yes, I agree that the patch here is a kludge. But I'd still like to get it in for FF9 as a "no worse that the current code" patch in order to proceed with some of the event loop simplifications.
Comment 24 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-15 14:35:32 PDT
Ben, my plan is to spend the next day or two on this issue, cleaning up PSM to minimize the "kludginess" of your patch. Thank you very much for pointing out the problem with Sync dispatch. I will try  your idea of using normal dispatch + a monitor. If reviews go OK, then these PSM XPCOM proxy usages should be gone in a few days.
Comment 25 David :Bienvenu 2011-08-16 13:01:14 PDT
Benjamin, it would be really helpful if you could give Thunderbird a couple days warning before you land part B. I'm trying to plow through all our code that uses xpcom proxies but it's going to take a while before I can get the patches finished and reviewed. I've done the ldap and address book stuff, and have that reviewed, and am almost done with the code for import, but I haven't started IMAP yet.
Comment 28 Joshua Cranmer [:jcranmer] 2011-08-30 07:04:54 PDT
Comment on attachment 551776 [details] [diff] [review]
PSM synchronous proxies, part F, rev. 1

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

Drive-by comment:

::: security/manager/ssl/src/nsSyncRunnableHelpers.cpp
@@ +77,5 @@
> +template<>
> +struct RefType<nsAString&>
> +{
> +  typedef nsAString& type;
> +};

Do you really need all of these specializations? My copy of the C++ spec indicates that T& produces just T if the typename is itself a reference.
Comment 29 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-30 10:14:09 PDT
Comment on attachment 551776 [details] [diff] [review]
PSM synchronous proxies, part F, rev. 1

In my local tree, I have removed all the XPCOM proxies in PSM with a set of much simpler (and smaller) patches. These patches conflict overlap a lot with the patch for bug 674147 that I am working on. My plan is to get the patches for bug 674147 reviewed and landed (since it is a Q3 goal) and then finish the XPCOM proxy removal.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2011-08-30 13:26:42 PDT
Comment on attachment 549390 [details] [diff] [review]
Part A - fix XPCOM code which used proxies, rev. 1

>Bug 675221 part A: replace XPCOM proxies with runanble for code in XPCOM itself, r?bz

s/runanble/runnable/

>+++ b/xpcom/base/nsConsoleService.cpp
>+nsConsoleService::RegisterListener(nsIConsoleListener *listener)
>+        // Reregistering a listener isn't good

Why not?  It doesn't seem to be a problem to me...  It's probably safer to not make this behavior change.

>+nsConsoleService::UnregisterListener(nsIConsoleListener *listener)

Similar here.

>+    if (mListeners.GetWeak(canonical)) {
>+        // Unregistering a listener that was never registered?
>+        return NS_ERROR_FAILURE;

But if you do keep this check, its sense needs to be reversed, right?

>+++ b/xpcom/components/nsCategoryManager.cpp
>+class CategoryNotificationRunnable : public nsRunnable
>+  const char* mTopic;

This should probably be an nsCString; we have no guarantee that aTopic is a static string, and it could go away before our runnable actually runs.

> nsCategoryManager::NotifyObservers( const char *aTopic,
>+  if (NS_IsMainThread()) {
>+    r->Run();

This is changing behavior; the old behavior was always async, right?  Why is this a desirable change?

r=me with the above issues addressed.
Comment 31 Benjamin Smedberg [:bsmedberg] 2011-08-31 10:12:48 PDT
Adding the same console listener twice would only end up adding it once, and the first call to RemoveListener would remove it completely. I don't think this behavior makes any sense and I'm pretty certain that none of the existing callers double-register in a way that would make this behavior change harmful. I'd really like to keep the change.

For CategoryNotificationRunnable all the callers do indeed pass a static string, so I will update the docs of the relevant methods to make the requirement clear.

And the new behavior matched the old behavior in nsCategoryManager::NotifyObservers, because we originally passed NS_PROXY_ASYNC *without* NS_PROXY_ALWAYS, which means that same-thread proxies just ran directly.
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2011-08-31 10:34:39 PDT
> I'd really like to keep the change.

OK.

> so I will update the docs of the relevant methods to make the requirement clear.

Can any code not in our codebase call into this code?  I really dislike having assumptions like this baked in....

> because we originally passed NS_PROXY_ASYNC *without* NS_PROXY_ALWAYS, which means that
> same-thread proxies just ran directly.

nsProxyObjectManager::GetProxyForObject creates a proxy as long as NS_PROXY_ASYNC is set, no matter what thread things are on.

Once we have a proxy, nsProxyEventObject::CallMethod only does sync same-thread invoke if NS_PROXY_SYNC is set.

So I don't see why the lone PROXY_ASYNC case ran directly.  What am I missing?
Comment 33 Luke Wagner [:luke] 2011-09-06 12:06:43 PDT
*** Bug 674597 has been marked as a duplicate of this bug. ***
Comment 34 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-09-23 10:16:26 PDT
Created attachment 562083 [details] [diff] [review]
Implement Runnable that can be waited on synchronously, for removing XPCOM proxies

This is a utility class based on bsmedmerg's, which implements a Runnable that can be waited on synchronously without dispatching events in the current thread's event queue.
Comment 35 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-09-23 10:28:54 PDT
Created attachment 562095 [details] [diff] [review]
Enforce that NSS dialogs are only shown via code that runs on the main thread
Comment 36 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-09-23 10:30:30 PDT
Created attachment 562096 [details] [diff] [review]
Notify observers of PSM's background processes on the main thread
Comment 37 Honza Bambas (:mayhemer) 2011-09-27 14:35:34 PDT
Comment on attachment 562083 [details] [diff] [review]
Implement Runnable that can be waited on synchronously, for removing XPCOM proxies

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

::: security/manager/ssl/src/PSMRunnable.cpp
@@ +49,5 @@
> +  NS_ASSERTION(!NS_IsMainThread(),
> +               "DispatchToMainThreadAndWait called on the main thread.");
> +
> +  mozilla::MonitorAutoLock lock(monitor);
> +  nsresult rv = NS_DispatchToMainThread(this);

Here should be a check if you are already on the main thread and assert/fail or just call RunOnTargetThread directly...  This could deadlock when used on the main thread accidentally.

::: security/manager/ssl/src/PSMRunnable.h
@@ +53,5 @@
> +protected:
> +  SyncRunnableBase();
> +  virtual void RunOnTargetThread() = 0;
> +private:
> +  mozilla::Monitor monitor;

mMonitor ?
Comment 38 Honza Bambas (:mayhemer) 2011-09-27 14:52:40 PDT
(In reply to Honza Bambas (:mayhemer) from comment #37)
> > +  NS_ASSERTION(!NS_IsMainThread(),
> > +               "DispatchToMainThreadAndWait called on the main thread.");
> > +
> > +  mozilla::MonitorAutoLock lock(monitor);
> > +  nsresult rv = NS_DispatchToMainThread(this);
> 
> Here should be a check if you are already on the main thread and assert

New glasses needed... :)
Comment 39 Benjamin Smedberg [:bsmedberg] 2011-09-28 10:43:51 PDT
Comment on attachment 562083 [details] [diff] [review]
Implement Runnable that can be waited on synchronously, for removing XPCOM proxies

The namespace decl is a little nonstandard for our code but I like it. Maybe we should put that in the style guide.
Comment 40 :Ms2ger (⌚ UTC+1/+2) 2011-09-30 07:41:09 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #39)
> Comment on attachment 562083 [details] [diff] [review] [diff] [details] [review]
> Implement Runnable that can be waited on synchronously, for removing XPCOM
> proxies
> 
> The namespace decl is a little nonstandard for our code but I like it. Maybe
> we should put that in the style guide.

I have to admit I'm no fan of it :)
Comment 41 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-02 02:54:31 PDT
Created attachment 564035 [details] [diff] [review]
Remove XPCOM Proxies: cleanup PRBool -> bool conversion

The patch to replace PRBool with bool landed and all my patches bitrotted. This patch helps to un-bitrot my patches. Basically, in any file that my XPCOM proxy patches touch, I replace PR_TRUE with true and PR_FALSE with false, except for lines of the form "PRBool <variable> = <PR_TRUE or PR_FALSE>;" which are used to initialize PRBool values that are going to be passed via pointer to NSS C functions. I figure it is better to get this out of the way so we can avoid any potential future mass bitrotting of patches to these files.
Comment 42 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-02 05:43:27 PDT
Created attachment 564047 [details] [diff] [review]
Notify observers of PSM's background processes on the main thread [v2]
Comment 43 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-02 05:45:42 PDT
Created attachment 564048 [details] [diff] [review]
Enforce that NSS dialogs are only shown via code that runs on the main thread [v2]

This is the order in which to queue the patches posted so far.
psm-PRBool-to-bool.patch
psm-remove-default-ssl-error-UI.patch
psm-remove-certerror-test.patch
psm-runnable.patch
psm-handle-ssl-and-cert-errors-on-main-thread.patch
psm-dialogs-and-prompts-on-main-thread.patch
psm-notify-observers-on-main-thread.patch
Comment 44 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-02 23:53:22 PDT
Created attachment 564127 [details] [diff] [review]
Do PKCS#11 token password prompting on the main thread
Comment 45 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-03 00:28:53 PDT
Created attachment 564131 [details] [diff] [review]
Do SSL client authentication on the main thread only
Comment 46 Honza Bambas (:mayhemer) 2011-10-04 08:46:58 PDT
Comment on attachment 564035 [details] [diff] [review]
Remove XPCOM Proxies: cleanup PRBool -> bool conversion

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

This patch is conflicting (better say - is mostly the same) as the security/ parts of the patch for bug 690892.  What more, your patch is missing some changes.  On the other hand, you turn some NS_ASSERTION(false) to NS_NOTREACHED() which is good, but probably worth to separate to a new bug.

Please wait until that patch gets landed and then check the code for all PRBool/PR_TRUE/PR_FALSE encounters.  There is not that much places that call to NSS and must not be changed.

A general note: please use just 8 lines context for your patches to be consistent with patches from others.
Comment 47 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-05 22:48:57 PDT
Honza, 

I will factor out the security/manager changes from bug 690892 into its own patch so that we can get this stuff landed soon. I also have to factor out the cert error parts of 679140 into their own patch, because that will need to get committed to mozilla-aurora to fix bug 650858.

I need to move the changes to nsNSSSocketInfo::GetInterface from the patch in bug 679140 to the "PKCS#11 token password prompting on the main thread" patch. See: 

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/security/manager/ssl/src&command=DIFF_FRAMESET&file=nsNSSIOLayer.cpp&rev2=1.6&rev1=1.5
Comment 48 :Ms2ger (⌚ UTC+1/+2) 2011-10-06 03:17:31 PDT
(In reply to Brian Smith (:bsmith) from comment #47)
> Honza, 
> 
> I will factor out the security/manager changes from bug 690892 into its own
> patch so that we can get this stuff landed soon.

As discussed on m.d.planning, don't do that.
Comment 49 Honza Bambas (:mayhemer) 2011-10-06 11:10:58 PDT
(In reply to Brian Smith (:bsmith) from comment #47)
> Honza, 
> 
> I will factor out the security/manager changes from bug 690892 into its own
> patch so that we can get this stuff landed soon.

I wouldn't do that.  Rather coordinate with Chris on the existing patch.

> I also have to factor out
> the cert error parts of 679140 into their own patch, because that will need
> to get committed to mozilla-aurora to fix bug 650858.

Sure, please let me just check (r) the patch when you are done.
Comment 50 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-06 17:21:46 PDT
(In reply to Honza Bambas (:mayhemer) from comment #49)
> I wouldn't do that.  Rather coordinate with Chris on the existing patch.

The tryserver run for the patches with PR_TRUE/PR_FALSE -> true/false conversion is at:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=697290b18d0e

I have rebased the patches on mozilla-central without any PR_TRUE/PR_FALSE conversions. I will attach the new patches.

> > I also have to factor out
> > the cert error parts of 679140 into their own patch, because that will need
> > to get committed to mozilla-aurora to fix bug 650858.
> 
> Sure, please let me just check (r) the patch when you are done.

I decided not to factor these changes out, because there is some common code between the SSL error and cert error cases. It should be OK to take the patch attached to the bug as-is on mozilla-aurora.
Comment 51 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-06 17:23:20 PDT
Created attachment 565407 [details] [diff] [review]
Enforce that NSS dialogs are only shown via code that runs on the main thread [v3]
Comment 52 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-06 17:24:30 PDT
Created attachment 565408 [details] [diff] [review]
Do SSL client authentication on the main thread only [v2]
Comment 53 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-06 17:25:11 PDT
Created attachment 565409 [details] [diff] [review]
Notify observers of PSM's background processes on the main thread [v2]
Comment 54 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-07 00:11:59 PDT
Comment on attachment 562083 [details] [diff] [review]
Implement Runnable that can be waited on synchronously, for removing XPCOM proxies

https://hg.mozilla.org/mozilla-central/rev/871bfac06cf1
Comment 55 :Ms2ger (⌚ UTC+1/+2) 2011-10-07 01:32:25 PDT
Comment on attachment 565407 [details] [diff] [review]
Enforce that NSS dialogs are only shown via code that runs on the main thread [v3]

>--- a/security/manager/boot/src/nsSecureBrowserUIImpl.cpp
>+++ b/security/manager/boot/src/nsSecureBrowserUIImpl.cpp
> GetNSSDialogs(nsISecurityWarningDialogs **result)
>   nsCOMPtr<nsISecurityWarningDialogs> my_result(do_GetService(NS_SECURITYWARNINGDIALOGS_CONTRACTID, &rv));
>+  return CallQueryInterface(my_result, result);

Unless we've got a really weird QI implementation here, this should be 'my_result.forget(result);'

>--- a/security/manager/ssl/src/nsCrypto.cpp
>+++ b/security/manager/ssl/src/nsCrypto.cpp
>@@ -2605,43 +2605,34 @@ nsCrypto::SignText(const nsAString& aStr
>+  nsresult rv;
>+
>   nsCString host;
>   rv = uri->GetHost(host);

Move the declaration here.

>+void PK11PasswordPromptRunnable::RunOnTargetThread()
>+{
>   else
>   {
>     // The interface requestor object may not be safe, so
>     // proxy the call to get the nsIPrompt.

Still true?

>+    prompt = do_GetInterface(mIR);
>+    NS_ASSERTION(prompt != nsnull, "callbacks does not implement nsIPrompt");

s/prompt != nsnull/prompt/

>--- a/security/manager/ssl/src/nsNSSComponent.cpp
>+++ b/security/manager/ssl/src/nsNSSComponent.cpp

> NS_IMETHODIMP PipUIContext::GetInterface(const nsIID & uuid, void * *result)

This could be written more clearly as 

if (!uuid.Equals(NS_GET_IID(nsIPrompt))) {
  return NS_ERROR_NO_INTERFACE;
}

nsresult rv;
nsCOMPtr<nsIWindowWatcher> wwatch = do_GetService(NS_WINDOWWATCHER_CONTRACTID, &rv);

if (!wwatch) {
  return rv;
}

return wwatch->GetNewPrompter(0, (nsIPrompt**) result);

> getNSSDialogs(void **_result, REFNSIID aIID, const char *contract)
> {
>+  rv = svc->QueryInterface(aIID, _result);
>+
>   return rv;

'return svc->QueryInterface(aIID, _result);'

>--- a/security/manager/ssl/src/nsNSSIOLayer.cpp
>+++ b/security/manager/ssl/src/nsNSSIOLayer.cpp
> NS_IMETHODIMP nsNSSSocketInfo::GetInterface(const nsIID & uuid, void * *result)
> {
>   nsresult rv;

No need for it here either, you can return directly.
Comment 56 :Ms2ger (⌚ UTC+1/+2) 2011-10-07 01:49:21 PDT
Comment on attachment 565409 [details] [diff] [review]
Notify observers of PSM's background processes on the main thread [v2]

>--- a/security/manager/ssl/src/PSMRunnable.h
>+++ b/security/manager/ssl/src/PSMRunnable.h

>+class NotifyObserverRunnable : public nsRunnable
>+{
>+public:
>+  NotifyObserverRunnable(nsIObserver * observer,
>+                         const char * topicStringLiteral)
>+    : mObserver(), mTopic(topicStringLiteral) { mObserver = observer; }

: mObserver(observer)
, mTopic(topicStringLiteral)
{}
Comment 57 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-25 17:26:14 PDT
Created attachment 569560 [details] [diff] [review]
Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert
Comment 58 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-25 17:34:43 PDT
Created attachment 569564 [details] [diff] [review]
Remove XPCOM Proxies from PSM UI prompts
Comment 59 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-25 17:36:37 PDT
Created attachment 569565 [details] [diff] [review]
Notify observers of PSM's background threads on the main thread
Comment 60 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-25 17:38:25 PDT
Created attachment 569566 [details] [diff] [review]
Remove XPCOM Proxies: Do PSM client certificate processing on the main thread
Comment 61 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-25 17:42:23 PDT
Created attachment 569568 [details] [diff] [review]
Remove XPCOM proxy header #includes from PSM
Comment 62 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-25 17:46:08 PDT
Attachment 569560 [details] [diff] is the one that blocks bug 436379.
Comment 63 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-25 17:48:01 PDT
Here is the order in which patches should be applied/reviewed in order to get clean builds:

psm-move-isExtendedValidation-to-nsISSLStatus.patch (from bug 686248)
psm-remove-XPCOM-proxies-from-GetPreviousCert.patch
psm-remove-XPCOM-proxies-from-prompts.patch
psm-notify-observers-on-main-thread.patch
psm-client-auth-on-main-thread.patch
psm-remove-XPCOM-proxy-includes.patch
Comment 64 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-25 17:50:24 PDT
Also, when reviewing psm-remove-XPCOM-proxies-from-prompts.patch, it helps to know that nsNSSSocketInfo::GetInterface is called (only) as part of PKCS#11 module login. See the hg blame that shows when/why that method was first implemented.
Comment 65 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-25 18:01:50 PDT
Here is the TryServer run for the above patches (plus a couple of minor cleanup patches that can be reviewed and committed later):

https://tbpl.mozilla.org/?tree=Try&rev=f4a5c79bcbd3
Comment 66 Kai Engert (:kaie) 2011-10-26 02:24:00 PDT
Comment on attachment 569568 [details] [diff] [review]
Remove XPCOM proxy header #includes from PSM

r=kaie
Comment 67 Kai Engert (:kaie) 2011-10-26 02:26:01 PDT
Comment on attachment 569566 [details] [diff] [review]
Remove XPCOM Proxies: Do PSM client certificate processing on the main thread

-            caNameStrings[n] = "";
+            caNameStrings[n] = const_cast<char*>("");

Is this old and new code is safe, because caNameStrings doesn't get freed explicitly, rather only the arena gets freed?

Maybe add a comment. When in doubt, maybe use PORT_ArenaStrdup(arena, "") ?

Remainder looks ok.
r=kaie
Comment 68 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-26 02:54:24 PDT
Created attachment 569631 [details] [diff] [review]
NOT FOR CHECKIN: Changes to nsNSSCertificate, ignoring indention changes

This version of the changes to nsNSSCerticate.cpp without whitespace changes should help with the review, because my patch changes the indention of a large part of the file.
Comment 69 Kai Engert (:kaie) 2011-10-28 09:06:33 PDT
Comment on attachment 569565 [details] [diff] [review]
Notify observers of PSM's background threads on the main thread

Can you please test that key generation still works with your patch?

There are two ways how this can be triggered, either using (a) HTML <KEYGEN> tag or (b) by use of JS function crypto.generateCRMFRequest

test for (a)
http://kuix.de/misc/test3/rsa.php

test for (b)
http://kuix.de/misc/test3/crmf-rsa.php

If it works, r=kaie

I wish I could offer a test for protected-auth, but that requires the use of a special authentication hardward (I don't have that available).
Comment 70 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-30 17:01:08 PDT
Created attachment 570582 [details] [diff] [review]
Notify observers of PSM's background threads on the main thread [v2]

The original patch did not work; the observables do have to be notified synchronously. This version uses SyncRunnableBase.
Comment 71 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-30 17:32:12 PDT
Created attachment 570584 [details] [diff] [review]
Notify observers of PSM's background threads on the main thread [v3]

The problem with the patch previously was actually that AddRef was being called off the main thread for a JS component that doesn't have a thread-safe AddRef. It is actually OK to notify the observer and quit without waiting for it to return from its Notify() method.
Comment 72 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-30 17:46:59 PDT
Comment on attachment 569566 [details] [diff] [review]
Remove XPCOM Proxies: Do PSM client certificate processing on the main thread

https://hg.mozilla.org/mozilla-central/rev/04b4ea333800
Comment 73 Kai Engert (:kaie) 2011-11-01 09:55:49 PDT
Comment on attachment 569564 [details] [diff] [review]
Remove XPCOM Proxies from PSM UI prompts

In nsNSSCertificateDB::DisplayCertificateAlert
I don't see code that frees this temporary object:

    if (!ctx)
      ctx = new PipUIContext();

You later have:
      nsCOMPtr<nsIPrompt> prompt (do_GetInterface(ctx));

You probably expect that do_GetInterface will always work, and thereby your initial ctx object will get released/deleted, too. 

But this is:
- difficult to read (it took me 10 minutes to reassure myself)
- bad, because it leaks if do_GetInterface fails for whatever reason

This is why the original code used the nsCOMPtr my_ctx helper.

...

+nsNSSComponent::GetNewPrompter(nsIPrompt ** result)
+{
+  if (!NS_IsMainThread()) {
+    NS_ERROR("nsSDRContext::GetInterface called off the main thread");
+    return NS_ERROR_NOT_SAME_THREAD;
+  }

Please fix the error string to quote the correct context.

...

(still reviewing)
Comment 74 Kai Engert (:kaie) 2011-11-01 10:25:32 PDT
Inside PipUIContext::GetInterface, I don't like this line:
  return nsNSSComponent::GetNewPrompter(reinterpret_cast<nsIPrompt**>(result));

I'd avoid reinterpret_cast and rather use code like this:

  nsIPrompt *p;
  nsresult rv = nsNSSComponent::GetNewPrompter(&p);
  if (NS_SUCCEEDED(rv))
    *result = p;
  return rv;
Comment 75 Kai Engert (:kaie) 2011-11-01 10:56:01 PDT
Inside nsNSSComponent::GetNewPrompter

(a)
Instead of
> if (!rv) {
    NS_ERROR("wwatch->GetNewPrompter() failed\n");
  }

please use
  if (NS_FAILED(rv)) {


(b)
How about adding
  NS_ENSURE_ARG_POINTER(result);
  *result = nsnull
near the top of the function?


...


The old implementation of ShowAlert was more code size efficient, as each string ID was listed in the code only once.

Your new code contains string "ProfileSwitchCryptoUIActive" twice.
You should either have all occurrences refer to the same NS_NAMED_LITERAL_STRING, or change it back to some enum.

Or maybe you could introduce a helper function that does nothing else but call ShowAlertFromStringBundle("ProfileSwitchCryptoUIActive");

....
Comment 76 Kai Engert (:kaie) 2011-11-01 11:10:51 PDT
Comment on attachment 569564 [details] [diff] [review]
Remove XPCOM Proxies from PSM UI prompts

The remainder looks good.
If you address my change requests in comment 73 to 75, then r=kaie
Comment 77 Kai Engert (:kaie) 2011-11-01 11:48:04 PDT
Comment on attachment 570584 [details] [diff] [review]
Notify observers of PSM's background threads on the main thread [v3]

+    observer = new NotifyObserverRunnable(aObserver, "keygen-finished");

Please rename some variables, to make the code easier to read/understand.

I'd rename
  nsKeygenThread's member "observer" to "runnableNotifier"

I'd rename 
  nsProtectedAuthThread's member "mStatusObserver" to "mRunnableNotifier"

In both
  - nsKeygenThread::Run
  - nsProtectedAuthThread::Run
I'd rename local variable
  nsCOMPtr<nsIRunnable> notifyObserver
to something like
  pendingRunnable


r=kaie
Comment 78 Kai Engert (:kaie) 2011-11-01 14:22:52 PDT
Comment on attachment 569560 [details] [diff] [review]
Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert

Thank you very much for this work, I appreciate your effort to keep things working for Thunderbird etc.


+  // Make a copy in case mCallbacks is assigned to while this function runs.
+  nsCOMPtr<nsIInterfaceRequestor> callbacks = mCallbacks;

I think it is dangerous to share a nsCOMPtr* between threads (Hmm, but it seems you're actually not doing that.) I'd also avoid passing around nsCOMPtr objects.


Proposal:

- I'd change PreviousCertRunnable::constructor
  to take parameter type nsIInterfaceRequestor*

- same change for getSecureBrowserUI()

- in getSecureBrowserUI() remove the copy and the comment

(This might build, if not, you must add .get() to the parameter when you call the functions - this will give you access to the raw pointer, without changing reference counts.)


...

In nsNSSSocketInfo::GetPreviousCert
if runnable->dispatch() fails,
then you should return and not access runnable->mPrevCert.

...

Optional: you could declare getSecureBrowserUI
as a static member function of nsNSSSocketInfo
to clarify it's purpose.

...
Comment 79 Honza Bambas (:mayhemer) 2011-11-01 14:26:31 PDT
Comment on attachment 569560 [details] [diff] [review]
Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert

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

I'll rereview the new version quickly.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +357,5 @@
>  }
>  
> +static nsresult
> +getSecureBrowserUI(nsCOMPtr<nsIInterfaceRequestor> & mCallbacks,
> +                   nsISecureBrowserUI ** result)

Why using ref to nsCOMPtr to pass the arg?  Because of what I comment just bellow on?  I'm against passing refs like this, unless really necessary.

@@ +364,5 @@
> +  *result = nsnull;
> +
> +  NS_ASSERTION(NS_IsMainThread(), "getSecureBrowserUI called off the main thread");
> +
> +  // Make a copy in case mCallbacks is assigned to while this function runs.

You expect this can get modified by another thread?  Then it needs to sync and not just copy if you believe this.

However, the call trees for this method seems that there is no need for sync, even with the ssl thread still in.

This can get posted from checkHandshake, nsNSSBadCertHandler, HandshakeCallback.  All can be called only on the ssl thread while the network thread cannot perform any op on that socket that could cause change to mCallbacks thanks the pollable event masquerade in fd->lower.

After removal of the ssl thread, the network thread will be, simply (so very much simply!! :) ), blocked, thus the same situation.

@@ +372,1 @@
>      return NS_OK;

This is different: the old code returns NS_ERROR_FAILURE in this case.

@@ +388,4 @@
>      }
>    }
>  
>    return NS_OK;

Shouldn't you fail here?

@@ +825,5 @@
> +
> +  virtual void RunOnTargetThread()
> +  {
> +    nsCOMPtr<nsISecureBrowserUI> secureUI;
> +    (void) getSecureBrowserUI(mCallbacks, getter_AddRefs(secureUI));

As above: this may fail because of missing callbacks.  Not sure this error reporting has to be preserved, though.

@@ +849,5 @@
> +
> +  if (NS_IsMainThread()) {
> +    NS_ERROR("nsNSSSocketInfo::GetPreviousCert called on the main thread");
> +    return NS_ERROR_NOT_SAME_THREAD;
> +  }

Why exactly would this be wrong?  You may allow the code to call PreviousCertRunnable::RunOnTargetThread() code directly in this case.

That's why I wanted the runnable to handle this automatically, but if you would agree on that change, there is no need to do it for this bug.

@@ +854,5 @@
> +
> +  nsRefPtr<PreviousCertRunnable> runnable = new PreviousCertRunnable(mCallbacks);
> +  nsresult rv = runnable->DispatchToMainThreadAndWait();
> +  NS_ASSERTION(NS_SUCCEEDED(rv), "runnable->DispatchToMainThreadAndWait() failed");
> +  runnable->mPreviousCert.forget(_result);

Only when NS_SUCCEEDED(rv) ?
Comment 80 Kai Engert (:kaie) 2011-11-01 14:35:42 PDT
Comment on attachment 569560 [details] [diff] [review]
Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert

The old code keeps a permanent reference in nsNSSSocketInfo::mPreviousCert

I'm worried you're changing how mPreviousCert gets remembered.
I don't see where your new code sets it.

(All you have are temporary objects with the same variable name.)
Comment 81 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-11-01 16:04:44 PDT
Comment on attachment 569560 [details] [diff] [review]
Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert

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

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +849,5 @@
> +
> +  if (NS_IsMainThread()) {
> +    NS_ERROR("nsNSSSocketInfo::GetPreviousCert called on the main thread");
> +    return NS_ERROR_NOT_SAME_THREAD;
> +  }

I did this just for simplicity. GetPreviousCert is only called in one place and that is always on the SSL thread and never on the main thread.

@@ +854,5 @@
> +
> +  nsRefPtr<PreviousCertRunnable> runnable = new PreviousCertRunnable(mCallbacks);
> +  nsresult rv = runnable->DispatchToMainThreadAndWait();
> +  NS_ASSERTION(NS_SUCCEEDED(rv), "runnable->DispatchToMainThreadAndWait() failed");
> +  runnable->mPreviousCert.forget(_result);

It won't matter. The code works even when the dispatch fails.
Comment 82 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-11-01 16:08:00 PDT
(In reply to Kai Engert (:kaie) from comment #78)
> In nsNSSSocketInfo::GetPreviousCert
> if runnable->dispatch() fails,
> then you should return and not access runnable->mPrevCert.

Like I mentioned in my reply to Honza, it is OK to call runnable->mPreviousCert.forget(_result) here, because it will just copy a null pointer into _result.

I will post an updated patch.
Comment 83 Honza Bambas (:mayhemer) 2011-11-01 16:23:00 PDT
(In reply to Kai Engert (:kaie) from comment #80)
> Comment on attachment 569560 [details] [diff] [review] [diff] [details] [review]
> Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert
> 
> The old code keeps a permanent reference in nsNSSSocketInfo::mPreviousCert
> 
> I'm worried you're changing how mPreviousCert gets remembered.
> I don't see where your new code sets it.
> 
> (All you have are temporary objects with the same variable name.)

If we want to preserve this highly non-transparent code, then we should cache the cert whenever we call getSecureBrowserUI.

However, I think it is not needed, the chain is: infoObject -> docshell -> browsersecureui.  browsersecureui keeps the reference to nsSSLStatus extracted from the top-level channel on most recent call to onlocationchange/onstatuschange-stop.  Those calls happen after the handshake callback, so things don't get overwritten.  The top-level channel is kept by bfcache.  So the status holding mServerCert (which is assigned to mPreviousCert) will never be gone.

Actually, I'm not sure if the current code is correct.  Caching the prev cert might be invalidated when going from a third site in order, if I'm not wrong.
Comment 84 Honza Bambas (:mayhemer) 2011-11-01 16:24:22 PDT
(In reply to Honza Bambas (:mayhemer) from comment #83)
> Actually, I'm not sure if the current code is correct.  Caching the prev
> cert might be invalidated when going from a third site in order, if I'm not
> wrong.

Nonsense - ignore.
Comment 85 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-11-01 16:29:23 PDT
It is OK semantically if GetPreviousCert() returns the "wrong" certificate for any definition of "wrong", because the caller always checks to see if the cert if the right one. In fact, it is very normal for GetPreviousCert() to return a certificate that is useless for the purpose it is intended for, becuause there are many "cache misses" with this approach to certificate caching.

In bug 696976 and/or other bugs, I will improve the certificate verification caching and GetPreviousCert() will die. This is just a very temporary change to facilitate the work the JS team is doing.
Comment 86 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-11-02 00:06:07 PDT
Created attachment 571254 [details] [diff] [review]
Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert [v2]

I have addressed the review comments.

RE: mPreviousCert, it doesn't get set anywhere anymore and I removed it. Honza's explanation of why it isn't necessary to cache mPreviousCert in the nsNSSSocketInfo structure is exactly right; it is only ever used in one place.

RE: return values, it turns out these functions don't even need to return a value, so I made them void.
Comment 87 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-11-02 00:33:07 PDT
Created attachment 571256 [details] [diff] [review]
Remove XPCOM Proxies from PSM UI prompts [v2]

I addressed the review comments in comments 73-75. r=kaie
Comment 88 Honza Bambas (:mayhemer) 2011-11-02 08:16:55 PDT
Comment on attachment 571254 [details] [diff] [review]
Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert [v2]

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

r=honzab

Thanks!

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +804,3 @@
>  {
> +public:
> +  PreviousCertRunnable(nsCOMPtr<nsIInterfaceRequestor> & callbacks)

Still no need to pass ref to nsCOMPtr, just nsIInterfaceRequestor * callbacks is ok.
Comment 89 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-11-02 21:17:03 PDT
Created attachment 571552 [details] [diff] [review]
Notify observers of PSM's background threads on the main thread [v4]

Carrying forward r+. I renamed the member variables but not the local variables, because "runnable" is implied by the type.
Comment 92 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-11-03 16:57:32 PDT
Do uses of NS_ProxyRelease need to go too for Luke's work? I noticed there are a lot of them, mostly outside of PSM.
Comment 93 Benjamin Smedberg [:bsmedberg] 2011-11-03 19:56:14 PDT
No, NS_ProxyRelease is unrelated to XPCOM proxies.
Comment 94 Luke Wagner [:luke] 2011-11-07 08:02:09 PST
*** Bug 674571 has been marked as a duplicate of this bug. ***
Comment 95 James Napolitano 2011-12-02 10:15:25 PST
Can parts A and B be landed yet?
Comment 96 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-18 02:42:49 PST
*** Bug 689107 has been marked as a duplicate of this bug. ***
Comment 97 Luke Wagner [:luke] 2012-01-04 09:23:56 PST
Any hope of this landing?
Comment 98 Benjamin Smedberg [:bsmedberg] 2012-01-09 12:56:33 PST
Created attachment 587089 [details] [diff] [review]
Addendum to part A - reimplement recursion checking for the console service, rev. 1
Comment 99 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-09 13:48:03 PST
Comment on attachment 587089 [details] [diff] [review]
Addendum to part A - reimplement recursion checking for the console service, rev. 1

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

::: xpcom/base/nsConsoleService.cpp
@@ +114,1 @@
>          mMessage(message)

Doesn't look like you're saving 'service' into your member here, this should crash.

@@ +136,4 @@
>      for (PRInt32 i = 0; i < mListeners.Count(); ++i)
>          mListeners[i]->Observe(mMessage);
>  
> +    mService->SetDoneDelivering();

Hm, this won't work if one of the listeners spins the event loop and another LogMessageRunnable runs... Not sure if it matters.
Comment 100 Ben Bucksch (:BenB) 2012-01-10 03:57:03 PST
GetProxyForObject() is widely - and often - used in multithreaded C++ XPCOM modules in third-party apps and extensions. This should have gotten consideration of the work that this means for them and a clear and easy migration path.
Instead, all we have is one sentence "this is deprecated" on <https://developer.mozilla.org/en/nsISupports_proxies>. We need 1) an explanation/rationale why this was removed (more than "this was painful") 2) a description of what to use instead, for all situations, including sync proxying where it is necessary 3) example code and a tutorial how to convert old code to the new one 4) pointers to that migration doc from <https://developer.mozilla.org/en/nsISupports_proxies> and the relevant Gecko dev notes.

In general, if you don't want to destroy your developer community, you need to give their situation more consideration than was shown here and in other "break the world" bugs. Other developers cannot afford to fix their code (and even scratch their heads) every 6 weeks just to keep the code running, just because you break APIs. They want/need to spend their time implementing new stuff, not with mere maintenance.
Comment 101 Benjamin Smedberg [:bsmedberg] 2012-01-10 05:30:34 PST
GetProxyForObject may be used widely in your code, but it is definitely not widely used in general. The rationale was already posted here and in the newsgroups (see "Removal of XPCOM proxies" in mozilla.dev.platform on 1-Aug-2011). I will definitely be updating the docs on this (once it lands). There are in all the cases we've come across more efficient and correct ways of implementing the same thing (in C++ code) using runnables, which will make it possible to remove all of the code in the threading library which supports filterable event queues (nsIThreadInternal.pushEventQueue).
Comment 102 Benjamin Smedberg [:bsmedberg] 2012-01-10 06:07:43 PST
Created attachment 587283 [details] [diff] [review]
Fix a test which uses console listeners incorrectly, rev. 1

bz, can you expedite this one-line review?
Comment 103 Ben Bucksch (:BenB) 2012-01-10 07:38:11 PST
> GetProxyForObject may be used widely in your code, but it is definitely not
> widely used in general

What do you mean "my code"? I am a consultant, my job is to help companies when they need help with their projects, so I am seeing a fairly representative sample.

In pretty much *every* project with C++ code, I see GetProxyForObject(). Tight now, today, I am in 2 such projects, and they both use GetProxyForObject() in several places. 

Compare comment 25.

> The rationale was already posted here

I only see comment 0, which is not a rationale, but an opinion.

Please note that third-party developers are very busy trying to keep up with their project manager's demands and don't have time to follow newsgroups.

> There are in all the cases we've come across more efficient and correct ways of
> implementing the same thing (in C++ code) using runnables

Please post the solutions on MDC, for all the variations of usecases.
Comment 104 Boris Zbarsky [:bz] (still a bit busy) 2012-01-10 13:51:13 PST
Comment on attachment 587283 [details] [diff] [review]
Fix a test which uses console listeners incorrectly, rev. 1

r=me
Comment 105 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-01-10 19:34:54 PST
(In reply to Ben Bucksch (:BenB) from comment #103)
> Please post the solutions on MDC, for all the variations of usecases.

See http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/PSMRunnable.cpp for a couple of simple utilities that I used to update PSM. (Note that this is NOT a stable API.) I bet the other patches use similar methods.

IIRC, typical use of XPCOM proxies required the implementation of the proxied interface to support threadsafe AddRef in many/most cases. It seems to me that a lot of the motivation for removing XPCOM proxies, in addition to removing a complex feature, was to force the code that uses them to change in a way that doesn't require (as much) threadsafe AddRefs. For example, nsIDocShell will only be AddRefable on the main thread very soon, which enables it to take part in cycle collection, thanks largely to some of the patches in this bug. Changing your code now to do more things on main-thread runnables is likely to prevent future breakages that you might also not see coming.
Comment 107 Ben Bucksch (:BenB) 2012-01-11 10:26:22 PST
> Changing your code now to do more things on main-thread runnables is likely to
> prevent future breakages that you might also not see coming.

Thanks for the note.

FWIW, the main point of my comment was that we should never ever be breaking external code without their developers seeing it coming 6-12 months ahead and being acutely aware of it. Given that there was no warning at all here so far, that period didn't even start yet.

Alternative is to write shim code that maps the old on the new API. In this case, it would probably be difficult.
Comment 108 Ben Bucksch (:BenB) 2012-01-11 10:29:08 PST
And I would like to note that I particularly appreciate that you turn around and remove an API one day after I said that it's being widely used and that the change will cause massive breakage in pretty much *every* C++ XPCOM component I've ever seen.
Comment 109 Benjamin Smedberg [:bsmedberg] 2012-01-11 11:56:07 PST
https://developer.mozilla.org/en/Making_Cross-Thread_Calls_Using_Runnables
https://developer.mozilla.org/en/nsISupports_proxies

BenB, as noted, there was plenty of advance notice (1-aug-2011). I'm sorry that you don't have time to keep track of that notice, but that doesn't really change the facts of the matter. This has been in the works for a long time.
Comment 110 Ben Bucksch (:BenB) 2012-01-11 14:56:02 PST
A "notice" is the <https://developer.mozilla.org/en/Firefox_12_for_developers>. Some newsgroups discussion doesn't count. Put a pre-warning in <https://developer.mozilla.org/en/Firefox_10_for_developers> saying that it will be removed in Firefox 19 (= one year later). Creating massive breakage like this pre-warning is no-go.

We needed time to adapt as well, this bug took 5 months! Why don't you given that time to others? Externals need *much more* time, not less.

> I'm sorry that you don't have time to keep track of

See commment 103: this is not about *me*. It's about the developers in the companies that have products based on Mozilla. None of them know about this.
Comment 111 Eric Shepherd [:sheppy] 2012-01-11 17:18:58 PST
bsmedberg did updates to the content; I did a copy edit pass and restored the content for the old nsISupports proxies page, instead adding a section at the top saying not to use it and why, because we need that information for historical purposes.

Change is mentioned on Firefox 12 for developers.
Comment 112 Marco Bonardo [::mak] 2012-01-13 01:04:44 PST
follow-up https://hg.mozilla.org/mozilla-central/rev/d5830c12eb58
Comment 113 David Ward 2012-01-22 10:35:45 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #106)
> https://hg.mozilla.org/mozilla-central/rev/2d79876ee142 part H

Part H adds references to MOZ_ASSERT in xpcom/base/nsConsoleService.{cc,h}, but I think you forgot to #include one of <mozilla/Util.h> or <mozilla/Assertions.h> ?

I only noticed this because I am locally building Firefox 9.0.1 and adding these changes in order to work around bug 689107.
Comment 114 David Ward 2012-01-22 10:38:53 PST
(In reply to David Ward from comment #113)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #106)
> > https://hg.mozilla.org/mozilla-central/rev/2d79876ee142 part H
> 
> Part H adds references to MOZ_ASSERT in xpcom/base/nsConsoleService.{cc,h},
> but I think you forgot to #include one of <mozilla/Util.h> or
> <mozilla/Assertions.h> ?
> 
> I only noticed this because I am locally building Firefox 9.0.1 and adding
> these changes in order to work around bug 689107.

Sorry I meant to refer to part A':

https://hg.mozilla.org/mozilla-central/rev/86da3ca1e3ae
Comment 115 Benjamin Smedberg [:bsmedberg] 2012-01-22 10:55:16 PST
On mozilla-central mozilla/Assertion happens to be included through nsTArray.h. I'm not sure it's worth bothering about changing that, but you can just include it directly on your branch where nsTArray may not implicitly include that header.
Comment 116 :Cykesiopka 2013-12-06 05:15:00 PST
*** Bug 619294 has been marked as a duplicate of this bug. ***

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