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.

Looks like part A and B are the same patch.

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....

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.

bsmith had talked about making PSM more sane here.

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 on attachment 549449 [details] [diff] [review] Part E - DOMStorage and miscellaenous includes, rev. 1 Review of attachment 549449 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab

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 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)

> 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?

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 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

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)?

Yes.

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

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.

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).

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.

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.

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.

http://hg.mozilla.org/integration/mozilla-inbound/rev/c2ca78ba49e2 part C http://hg.mozilla.org/integration/mozilla-inbound/rev/97407f5d696f part D http://hg.mozilla.org/integration/mozilla-inbound/rev/e8cb17fdc2f6 part E http://hg.mozilla.org/integration/mozilla-inbound/rev/df2aebf58b68 part G The remaining bits are on hold for the alternate PSM solution that bsmith is working on.

Parts C, D, E, G respectively: http://hg.mozilla.org/mozilla-central/rev/c2ca78ba49e2 http://hg.mozilla.org/mozilla-central/rev/97407f5d696f http://hg.mozilla.org/mozilla-central/rev/e8cb17fdc2f6 http://hg.mozilla.org/mozilla-central/rev/df2aebf58b68 Leaving open for PSM patch.

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 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 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.

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.

> 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?

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 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 ?

(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 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.

(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 :)

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.

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 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.

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

(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.

(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.

(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 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 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 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) {}

Attachment 569560 [details] [diff] is the one that blocks bug 436379.

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

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.

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 on attachment 569568 [details] [diff] [review] Remove XPCOM proxy header #includes from PSM r=kaie

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

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 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).

The original patch did not work; the observables do have to be notified synchronously. This version uses SyncRunnableBase.

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 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 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)

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;

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 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 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 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 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 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 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.

(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.

(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.

(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.

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.

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.

I addressed the review comments in comments 73-75. r=kaie

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.

Carrying forward r+. I renamed the member variables but not the local variables, because "runnable" is implied by the type.

https://hg.mozilla.org/integration/mozilla-inbound/rev/182821718b51 https://hg.mozilla.org/integration/mozilla-inbound/rev/75c2fd3d64c1 https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4f0ef1ef33 https://hg.mozilla.org/integration/mozilla-inbound/rev/e81f544236ea

https://hg.mozilla.org/mozilla-central/rev/e81f544236ea https://hg.mozilla.org/mozilla-central/rev/7d4f0ef1ef33 https://hg.mozilla.org/mozilla-central/rev/75c2fd3d64c1 https://hg.mozilla.org/mozilla-central/rev/182821718b51 can this be resolved now?

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.

No, NS_ProxyRelease is unrelated to XPCOM proxies.

Can parts A and B be landed yet?

Any hope of this landing?

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.

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.

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).

bz, can you expedite this one-line review?

> 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 on attachment 587283 [details] [diff] [review] Fix a test which uses console listeners incorrectly, rev. 1 r=me

(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.

https://hg.mozilla.org/mozilla-central/rev/4d03df4a60dc part A https://hg.mozilla.org/mozilla-central/rev/ab435575ac33 part B https://hg.mozilla.org/mozilla-central/rev/86da3ca1e3ae part A' - console service recursion checking https://hg.mozilla.org/mozilla-central/rev/2d79876ee142 part H

> 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.

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.

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.

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.

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.

follow-up https://hg.mozilla.org/mozilla-central/rev/d5830c12eb58

(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.

(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

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.