675221 - Remove XPCOM proxies

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

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]

Attachment: Part A - fix XPCOM code which used proxies, rev. 1

Comment 2

[Benjamin Smedberg]

Attachment: Part B - remove the implementation, rev. 1

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks like part A and B are the same patch.

Comment 4

[Benjamin Smedberg]

Attachment: Part B - remove the implementation, rev. 1 for real

Comment 5

[Boris Zbarsky [:bzbarsky]]

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]

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 (Exited; not receiving bugmail, old account, do not use)]

bsmith had talked about making PSM more sane here.

Comment 8

[Benjamin Smedberg]

Attachment: Part C - fix up necko, rev. 1

Comment 9

[Benjamin Smedberg]

Attachment: 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]

Attachment: Part E - DOMStorage and miscellaenous includes, rev. 1

Comment 11

[Honza Bambas (:mayhemer)]

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]

Attachment: 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]

Attachment: Part G: Define HAS-STDCALL for MSVC so that nsRunnableMethod specializes correctly, rev. 1

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 [:bzbarsky]]

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

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 (he/him; ⌚ UTC+1/+2)]

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 [:bzbarsky]]

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]

Yes.

Comment 20

[Mozilla RelEng Bot]

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/[e-mail]-66668bd29884

Comment 21

[Benjamin Smedberg]

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

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]

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

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]

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 26

[Benjamin Smedberg]

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.

Comment 27

[Ed Morley [:emorley]]

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 28

[Joshua Cranmer [:jcranmer]]

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

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 [:bzbarsky]]

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]

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 [:bzbarsky]]

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

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: 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?)]

Attachment: Enforce that NSS dialogs are only shown via code that runs on the main thread

Comment 36

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Notify observers of PSM's background processes on the main thread

Comment 37

[Honza Bambas (:mayhemer)]

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

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

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 (he/him; ⌚ UTC+1/+2)]

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

Attachment: 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?)]

Attachment: Notify observers of PSM's background processes on the main thread [v2]

Comment 43

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: 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?)]

Attachment: Do PKCS#11 token password prompting on the main thread

Comment 45

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Do SSL client authentication on the main thread only

Comment 46

[Honza Bambas (:mayhemer)]

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

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 (he/him; ⌚ UTC+1/+2)]

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

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

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

Attachment: Enforce that NSS dialogs are only shown via code that runs on the main thread [v3]

Comment 52

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Do SSL client authentication on the main thread only [v2]

Comment 53

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Notify observers of PSM's background processes on the main thread [v2]

Comment 54

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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 (he/him; ⌚ UTC+1/+2)]

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 (he/him; ⌚ UTC+1/+2)]

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

Attachment: Remove XPCOM Proxies from nsNSSSocketInfo::GetPreviousCert

Comment 58

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Remove XPCOM Proxies from PSM UI prompts

Comment 59

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Notify observers of PSM's background threads on the main thread

Comment 60

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Remove XPCOM Proxies: Do PSM client certificate processing on the main thread

Comment 61

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Remove XPCOM proxy header #includes from PSM

Comment 62

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 63

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

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

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

Comment on attachment 569568 [details] [diff] [review]
Remove XPCOM proxy header #includes from PSM

r=kaie

Comment 67

[Kai Engert (: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

Comment 68

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: 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:)]

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

Attachment: 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?)]

Attachment: 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?)]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Attachment: 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?)]

Attachment: Remove XPCOM Proxies from PSM UI prompts [v2]

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

Comment 88

[Honza Bambas (:mayhemer)]

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

Attachment: 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 90

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 91

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 92

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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]

No, NS_ProxyRelease is unrelated to XPCOM proxies.

Comment 95

[James Napolitano]

Can parts A and B be landed yet?

Comment 97

[Luke Wagner [:luke]]

Any hope of this landing?

Comment 98

[Benjamin Smedberg]

Attachment: Addendum to part A - reimplement recursion checking for the console service, rev. 1

Comment 99

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

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]

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]

Attachment: Fix a test which uses console listeners incorrectly, rev. 1

bz, can you expedite this one-line review?

Comment 103

[Ben Bucksch (:BenB)]

> 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 [:bzbarsky]]

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

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

[Benjamin Smedberg]

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

Comment 107

[Ben Bucksch (:BenB)]

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

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]

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

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

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] (Away Apr 25 - May 5)]

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

Comment 113

[David Ward]

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

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

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.