462497 - nsComponentManagerImpl::HashContractID() reenters mMon

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Sergey Yanovich]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008092816 Iceweasel/3.0.3 (Debian-3.0.3-3)
Build Identifier: trunk

nsComponentManagerImpl::HashContractID() is only called when mMon is already entered, so its re-entrance is probably excessive. Removing this should slightly reduce Ts.

Reproducible: Always




Patch follows.

Comment 1

[Sergey Yanovich]

Attachment: patch v1.0

Patch removes |nsAutoMonitor| from  |nsComponentManagerImpl::HashContractID()|.

Comment 2

[Benjamin Smedberg]

Comment on attachment 345687 [details] [diff] [review]
patch v1.0

The patch looks correct, but please document in the header that this method must only be called while the monitor is held (and ideally assert this is true, but I don't think NSPR has that capability currently)

Comment 3

[Sergey Yanovich]

Attachment: patch v2.0 [Backout: Comment 8]

Patch:
* adds a comment in xpcom/components/nsComponentManager.h;
* adds an assertion using PR_GetMonitorEntryCount().

Comment 4

[Benjamin Smedberg]

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

thanks!

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

We'll get this after beta 2

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

a191=beltzner who likes Ts wins

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

http://hg.mozilla.org/mozilla-central/rev/8b5a38ba459a

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 346107 [details] [diff] [review]
patch v2.0
[Backout: Comment 8]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/efe3c6f76bca
http://hg.mozilla.org/mozilla-central/rev/e45938aee309
See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229736880.1229736940.3072.gz
Linux mozilla-central leak test build on 2008/12/19 17:34:40

/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp: In member function ‘nsresult nsComponentManagerImpl::HashContractID(const char*, PRUint32, nsFactoryEntry*)’:
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:1301: error: ‘PR_GetMonitorEntryCount’ was not declared in this scope

/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp: In member function ‘nsIModuleLoader* nsComponentManagerImpl::LoaderForType(LoaderType)’:
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:2702: warning: comparison between signed and unsigned integer expressions
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp: In member function ‘void nsComponentManagerImpl::LoadDeferredModules(nsTArray<DeferredModule>&)’:
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:3124: warning: comparison between signed and unsigned integer expressions
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp: In member function ‘virtual nsresult nsComponentManagerImpl::AutoUnregisterComponent(PRInt32, nsIFile*)’:
/builds/moz2_slave/mozilla-central-linux-debug/build/xpcom/components/nsComponentManager.cpp:3166: warning: comparison between signed and unsigned integer expressions
}

NB: The 3 warnings are probably not yours, but if you could have a look at them too...

Comment 9

[Sergey Yanovich]

Attachment: patch v2.1

* fixed debug build by including "private/pprthred.h"
* fixed signed/unsigned mismatch warnings

Can r+/a+ be carried over in this situation?

Comment 10

[Benjamin Smedberg]

Comment on attachment 353965 [details] [diff] [review]
patch v2.1

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

>+/* Linux/Win32 export private NSPR files to include/private */
>+#ifdef XP_MAC

XP_MAC is dead (was MacOS9)

Comment 11

[Sergey Yanovich]

(In reply to comment #10)
> XP_MAC is dead (was MacOS9)

Any comments how it should look like instead? I copied the include block from here:
http://mxr.mozilla.org/mozilla/source/xpcom/base/nsGarbageCollector.c#54

Comment 12

[Sergey Yanovich]

Attachment: patch v2.2

* plainly include "private/pprthred.h", following this:
 http://mxr.mozilla.org/mozilla/source/netwerk/base/src/nsFileStreams.cpp#49

* use special macro PR_InMonitor() instead of PR_GetMonitorEntryCount()

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354404 [details] [diff] [review]
patch v2.2

Please only ask for 1.9.1-approval once you've gotten review and the patch has landed on mozilla-central.

Comment 14

[Benjamin Smedberg]

Comment on attachment 354404 [details] [diff] [review]
patch v2.2

>diff --git a/nsprpub/configure b/nsprpub/configure

Extraneous change?

>diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp

>-    nsAutoMonitor mon(mMon);
>+    NS_ASSERTION(PR_InMonitor(mMon), "called from outside mMon");
> 

Go ahead and make this NS_ABORT_IF_FALSE, here and below.

>@@ -3163,7 +3164,7 @@ nsComponentManagerImpl::AutoUnregisterComponent(PRInt32 /* unused */,
>     nsCOMPtr<nsIModule> module;
>     rv = mNativeModuleLoader.LoadModule(lf, getter_AddRefs(module));
>     if (NS_FAILED(rv)) {
>-        for (LoaderType i = 0; i < mLoaderData.Length(); ++i) {
>+        for (LoaderType i = 0; (PRUint32) i < mLoaderData.Length(); ++i) {

I don't like ridealong changes... please put this in a separate bug/patch. Also, I prefer C++ initializations instead of C-style casts, so

PRUint32(i) < mLoaderData.Length()

The core of this patch looks good. Give me a clean one and I can r+ it right away!

Comment 15

[Sergey Yanovich]

Attachment: patch v2.3

* uses NS_ABORT_IF_FALSE instead of NS_ASSERTION;
* leaves out warning fixes.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed fc85349c89b4

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7d740ae63d5c