235882 - Make nsComponentManagerImpl::AddLoaderType return only one type of value

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

from bug 235381 comment 5. dougt agrees that
nsComponentManagerImpl::AddLoaderType needs to be changed, the proposal is to
make it return an nsresult and out the loadertype.

an alternative could be to just have it return a result and make people use
GetLoaderType after calling AddLoaderType.

Comment 1

[timeless]

Attachment: proposal

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 142477 [details] [diff] [review]
proposal

don't make this change:

-	 int loadertype = GetLoaderType(values[2]);
+	 PRUint32 loadertype = GetLoaderType(values[2]);


why bother with this:

+	     if (rv == NS_ERROR_OUT_OF_MEMORY)
+		 return NS_ERROR_OUT_OF_MEMORY;

Just let it bail out later -- maybe this will happen in the arena_allocate
(maybe). 

drop the ws change:
-	 //*/
+	 // */

with that, r=

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch caused a significant leak increase on tinderbox.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

From lhasa tinderbox log:

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsMemoryImpl                                     68          -
GlobalWindowImpl                                684          -
nsSecretDecoderRing                              12          -
nsXPCComponents                                 240          -
nsLocalFile                                     116          -
XPCWrappedNative                                396          -
nsVoidArray                                      24          -
nsPrefBranch                                     60          -
nsHashtable                                      44          -
NavigatorImpl                                    48          -
nsDOMClassInfo                                  112          -
XPCNativeScriptableInfo                          72          -
nsPrefService                                    40          -
XPCNativeScriptableShared                       608          -
nsEntropyCollector                             1044          -
nsDebugImpl                                       8          -
AtomImpl                                         12          -
nsSharedPrefHandler                              84          -
nsPrincipal                                     116          -
nsSimpleURI                                       4          -
XPCWrappedNativeProto                           252          -
nsSystemPrincipal                                28          -

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

From brad tinderbox log:

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
GlobalWindowImpl                                684          -
nsSystemPrincipal                                28          -
nsDOMClassInfo                                  128          -
nsPrincipal                                     232          -
NavigatorImpl                                    48          -
nsPrefService                                    40          -
BarPropImpl                                      12          -
nsPrefBranch                                     60          -
nsBaseURLParser                                   8          -
nsHashtable                                      88          -
nsSharedPrefHandler                              84          -
nsVoidArray                                      32          -
nsEntropyCollector                             1044          -
nsStandardURL                                   192          -
XPCNativeScriptableInfo                          72          -
nsLocalFile                                     116          -
nsSimpleURI                                       4          -
XPCWrappedNativeProto                           252          -
XPCWrappedNative                                396     800.00%
XPCNativeScriptableShared                       684     800.00%
nsXPCComponents                                 240     400.00%

(Note that the trace-malloc based leak stats show about a 100K increase.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

This patch is also likely responsible for bug 236903

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix leak regression

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 143444 [details] [diff] [review]
fix leak regression 

verbal r=caillon sr=jst

Comment 9

[Scott MacGregor]

this patch broke the JS components. JS components can no longer be registered
with the component manager.

Backing out this checkin fixes the regression.

See Bug #236952 for some of the side effects. this needs fixed before 1.7b

Comment 10

[Doug Turner (:dougt)]

I think we should back out both patches and try again later.

Comment 11

[Scott MacGregor]

thanks doug. I just backed this out. 

Comment 12

[Boris Zbarsky [:bzbarsky]]

So dbaron's patch did not fix the regressions?  In addition to fixing leaks it
also made the code actually work, if you look at it.  It certainly fixes bug
236903 (once you recreate compreg.dat after applying that patch).

Comment 13

[Doug Turner (:dougt)]

I am not sure, but I do not want to have to have people be blocked for any
length of time while we are unsure about the regression.  This bug isn't that
important.

Comment 14

[Scott MacGregor]

dbaron's patch did indeed fix the regression we were seeing. So I put the
changes back in.

Comment 15

[timeless]

final versions:
mozilla/xpcom/components/nsComponentManager.cpp 	1.248
mozilla/xpcom/components/nsComponentManager.h 	1.96

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 236993 has been marked as a duplicate of this bug. ***

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 237036 has been marked as a duplicate of this bug. ***