106949 - Bad use of nsIFile in component manager [FIX]

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect, P1)

Description

[Conrad Carlen (not reading bugmail)]

This bit:
http://lxr.mozilla.org/mozilla/source/xpcom/components/nsComponentManager.cpp#2247
is evil. mComponentsOffset is the length of the path of the components dir.
Because this code is getting the full path and then advancing the pointer past
the parent dir portion, it requires that the path does not end in a separator.
This shouldn't be required of nsIFile. If it is more efficient, or the norm on
the OS, for directory paths to end in separators, the implementation should be
able to do that. Code which depends on this detail, such as this, needs to be
changed.

Comment 1

[Conrad Carlen (not reading bugmail)]

Not only that, this code is slow. It seems that what it's after is the leaf name
of the file. For that, there's GetLeaf().

Comment 2

[Pete Collins (MDG)]

Fix forthcoming

--pete

Comment 3

[Pete Collins (MDG)]

Attachment: fix, need review

Comment 4

[Pete Collins (MDG)]

Doug or Conrad, assign this bug to me. Someone give me an r= and i'll work on an sr=


--pete

Comment 5

[Conrad Carlen (not reading bugmail)]

Comment on attachment 60898 [details] [diff] [review]
fix, need review

>@@ -710,8 +710,6 @@
>     if (!componentDescriptor)
>         return NS_ERROR_NULL_POINTER;
> 
>-    mComponentsOffset = strlen(componentDescriptor);
>-

Once this is taken out, componentDescriptor is not used and should come out,
along with the calls that use it.

>     if (componentDescriptor)
>         nsMemory::Free(componentDescriptor);
> 
>@@ -2110,9 +2108,11 @@
>         if (NS_FAILED(rv))
>             return rv;
>

There's a call to GetPath() in here that can go now.

>-        char* relativeLocation = persistentDescriptor + mComponentsOffset + 1;
>+        nsXPIDLCString relativeLocation;
>+        if (NS_FAILED(rv = aSpec->GetLeafName(getter_Copies(relativeLocation))))
>+           return rv;
>         
>-        rv = MakeRegistryName(relativeLocation, XPCOM_RELCOMPONENT_PREFIX, 
>+        rv = MakeRegistryName(relativeLocation.get(), XPCOM_RELCOMPONENT_PREFIX, 
>                               aRegistryName);

Comment 6

[Pete Collins (MDG)]

Attachment: revised patch

Comment 7

[John Bandhauer]

Comment on attachment 60951 [details] [diff] [review]
revised patch

sr=jband
Looks reasonable to me. I'm sure Conrad will tell you if you slipped up on some
detail I don't see.

Comment 8

[Pete Collins (MDG)]

Attachment: changed persistentDescriptor over to nsXPIDLCString

Comment 9

[Pete Collins (MDG)]

patch 60969 is the same except i changed char *persistentDescriptor to
nsXPIDLCString and
removed call to ::Free since nsXPIDLCString class has it's own destructor.

So either patch is good for review. patch 60969 is my preference. 

Reguards performance, this patch doesn't really make any difference at least on
unix.

Using time, here are some fuzzy random results. These numbers were generated by
xpcshell 
running an external js script the prints 'Hello World'.  With and without the
generation of 
component.reg.

(Spacing may be horked below)

patch               original
TRY #1              TRY #1
-----------------   -----------------
no components.reg   no components.reg
real    0m6.233s    real    0m6.039s
user    0m5.565s    user    0m5.569s
sys     0m0.427s    sys     0m0.403s

real    0m0.808s    real    0m0.809s
user    0m0.631s    user    0m0.581s
sys     0m0.178s    sys     0m0.228s
-----------------   -----------------

TRY #2              TRY #2
-----------------   -----------------
no components.reg   no components.reg
real    0m6.161s    real    0m6.051s
user    0m5.574s    user    0m5.534s
sys     0m0.429s    sys     0m0.439s

real    0m0.812s    real    0m0.813s
user    0m0.610s    user    0m0.644s
sys     0m0.203s    sys     0m0.169s
-----------------   -----------------

Comment 10

[Conrad Carlen (not reading bugmail)]

Comment on attachment 60969 [details] [diff] [review]
changed persistentDescriptor over to nsXPIDLCString

I like this patch r=ccarlen.
Here's some nits you can take or leave:
(1) Since persistentDescriptor is only used within that else block, I'd declare
the variable within that scope.
(2) I'd rename persistentDescriptor -> absoluteLocation. Calling it
"persistentDescriptor" is odd since it didn't come from
GetPersistentDescriptor(). 

As far as the performance, I didn't think this would bring noticable
improvement - though it may on Mac because GetPath() is very expensive there.
It was mainly this bit:
-	 char* relativeLocation = persistentDescriptor + mComponentsOffset + 1;

which assumes (requires)  that paths to a directory don't end in directory
separator char.

Comment 11

[Pete Collins (MDG)]

Attachment: renamed var and moved into scope. removed unused GetPath accessor in if block

Comment 12

[Conrad Carlen (not reading bugmail)]

Comment on attachment 61023 [details] [diff] [review]
renamed var and moved into scope. removed unused GetPath accessor in if block

r=ccarlen

Comment 13

[Pete Collins (MDG)]

jband, i'm gonna take your sr= for the latest patch, ok.

I'll check this in as soon as the tree reopens today.


Thanks

--pete

Comment 14

[John Bandhauer]

Comment on attachment 61023 [details] [diff] [review]
renamed var and moved into scope. removed unused GetPath accessor in if block

sr=jband

Comment 15

[Pete Collins (MDG)]

Checked in marking FIXED.

Thanks guys.

--pete

Comment 16

[John Bandhauer]

I suggest that we back this out. 

This causes http://bugscape.netscape.com/show_bug.cgi?id=11374

That bug is about the NS spell checker not working.

The general problem is that nsNativeComponentLoader::RegisterComponentsInDir
goes down into subdirectories. The old code here handles that and returned a
substring that might not be the same as the string returned when only asking for
the leaf node.
Since this fix changes the behavior of the code (who knew?) and does not
actually fix anything that *needs* to be fixed. I think the only thing to do is
back it out immediately until a better fix is suggested.

Comment 17

[Kevin McCluskey (gone)]

cvs commands to backout patch 61023

cvs update -j1.71 -j1.70 mozilla/xpcom/components/nsComponentManager.h
cvs update -j1.180 -j1.179 mozilla/xpcom/components/nsComponentManager.cpp

Comment 18

[Brendan Eich [:brendan]]

a=[e-mail] on behalf of drivers for back-out.

/be

Comment 19

[Kevin McCluskey (gone)]

I backed out patch 61023

Comment 20

[Pete Collins (MDG)]

pushing back

Comment 21

[Pete Collins (MDG)]

Attachment: fix for backed out patch

Comment 22

[Pete Collins (MDG)]

Hey guys, have a look at patch 62297.

This fixes the problem of components residing in subdirs in the components dir.
Tested and works great. 

There seems to be no need at all to register based on relative location.
So that block is now gone. 

Please review.

Thanks

--pete 

Comment 23

[Conrad Carlen (not reading bugmail)]

Relative paths are a good thing. Absolute paths become invalid if somebody moves
their installation from one place to another. On some platforms ;-) people do
this all the time. I think we should leave it as-is until we have the relative
path stuff I've aways wanted on nsIFile. Sorry I filed this - I just completely
overlooked the nested directory case :-/ 

Comment 24

[Pete Collins (MDG)]

Gonna future this for now.

--pete