114125 - nsStandardURL::Clone fails to clone the nsIFile if one exists

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Simon Fraser [no longer active]]

nsStandardURL::Clone() doesn't clone its nsIFile into the cloned spec. This 
causes code later to have to re-create the nsIFile, which (on Mac at leaast) 
involves a full path to FSSpec conversion, which is expensive.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Patch to clone the mFile

Comment 2

[Darin Fisher]

actually, i think calling nsIFile::clone is unnecessary... this should be ok:

Index: nsStandardURL.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/base/src/nsStandardURL.cpp,v
retrieving revision 1.5
diff -u -r1.5 nsStandardURL.cpp
--- nsStandardURL.cpp	5 Dec 2001 23:54:55 -0000	1.5
+++ nsStandardURL.cpp	8 Dec 2001 00:44:43 -0000
@@ -1138,6 +1138,7 @@
     clone->mQuery = mQuery;
     clone->mRef = mRef;
     clone->mParser = mParser;
+    clone->mFile = mFile;
 
     NS_ADDREF(*result = clone);
     return NS_OK;

because, mFile is cached only to improve the cost of GetFile.  the resulting
mFile is never modified... it is only released.

Comment 3

[Simon Fraser [no longer active]]

But nsStandardURL doesn't own the file that's passed to SetFile(); it just 
AddRefs it. So the caller could call SetFile(), then modify the nsIFile. If 
Clone() didn't clone the nsIFile, then someone could change the nsIFile for more 
than on nsIURI. Yuck.

Comment 4

[Darin Fisher]

in that case, nsStandardURL::SetFile is wrong.  it should clone the nsIFile if
it is going to hold onto it.  in the current world the caller of SetFile could
modify the nsIFile and nsStandardURL would still happily give out the modified
nsIFile via nsStandardURL::GetFile.  so, if the clone belongs anywhere it
belongs in SetFile :(

Comment 5

[neeti]

over to darin

Comment 6

[Darin Fisher]

-> 0.9.9

Comment 7

[Darin Fisher]

Attachment: v2: clone in SetFile

simon: how about this patch?  can you sr=?

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 66951 [details] [diff] [review]
v2: clone in SetFile

add a ns_warning if the clone fails.

Comment 9

[Simon Fraser [no longer active]]

Comment on attachment 66951 [details] [diff] [review]
v2: clone in SetFile

sr=sfraser

Comment 10

[Darin Fisher]

fixed-on-trunk

Comment 11

[Tom Everingham]

verified, fix checked in