510991 - NS_GetURLSpecFromFile does a stat()

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[(dormant account)]

This one is ridiculous in that it does a stat() to determine if the url should have a '/' on the end. I'm pretty sure all of the callers already know if the url should point at a file or a directory.

Comment 1

[(dormant account)]

(In reply to comment #0)
> This one is ridiculous in that it does a stat() to determine if the url should
> have a '/' on the end. I'm pretty sure all of the callers already know if the
> url should point at a file or a directory.

it's a TODO, filed it in hopes of someone fixing it before I get to.

Comment 2

[Dietrich Ayala (:dietrich)]

per startup triage discussion, this might need an additive apis for GetURLSpecFromDir/GetURLSpecFromCallerIsSureItsAFile.

Comment 3

[Benedict Hsieh [:benedict]]

Attachment: creates getURLSpecFromDir and getURLSpecFromNonDirFile

I only modified two callers, but this patch plus bug 511761 removes all stats of components during startup.

Comment 4

[(dormant account)]

Comment on attachment 402227 [details] [diff] [review]
creates getURLSpecFromDir and getURLSpecFromNonDirFile

>diff --git a/js/src/xpconnect/loader/mozJSComponentLoader.cpp b/js/src/xpconnect/loader/mozJSComponentLoader.cpp
>--- a/js/src/xpconnect/loader/mozJSComponentLoader.cpp
>+++ b/js/src/xpconnect/loader/mozJSComponentLoader.cpp
>@@ -1181,17 +1181,17 @@ mozJSComponentLoader::GlobalForLocation(
>     nsCAutoString nativePath;
>     // Quick hack to unbust XPCONNECT_STANDALONE.
>     // This leaves the jsdebugger with a non-URL pathname in the 
>     // XPCONNECT_STANDALONE case - but at least it builds and runs otherwise.
>     // See: http://bugzilla.mozilla.org/show_bug.cgi?id=121438
> #ifdef XPCONNECT_STANDALONE
>     localFile->GetNativePath(nativePath);
> #else
>-    NS_GetURLSpecFromFile(aComponent, nativePath);
>+    NS_GetURLSpecFromNonDirFile(aComponent, nativePath);
> #endif
> 
>     // Before compiling the script, first check to see if we have it in
>     // the fastload file.  Note: as a rule, fastload errors are not fatal
>     // to loading the script, since we can always slow-load.
>     nsCOMPtr<nsIFastLoadService> flSvc = do_GetFastLoadService(&rv);
> 
>     // Save the old state and restore it upon return
>diff --git a/netwerk/base/public/nsNetUtil.h b/netwerk/base/public/nsNetUtil.h
>--- a/netwerk/base/public/nsNetUtil.h
>+++ b/netwerk/base/public/nsNetUtil.h
>@@ -742,16 +742,42 @@ NS_GetURLSpecFromFile(nsIFile      *file
>     nsresult rv;
>     nsCOMPtr<nsIFileProtocolHandler> fileHandler;
>     rv = NS_GetFileProtocolHandler(getter_AddRefs(fileHandler), ioService);
>     if (NS_SUCCEEDED(rv))
>         rv = fileHandler->GetURLSpecFromFile(file, url);
>     return rv;
> }
> 
>+inline nsresult
>+NS_GetURLSpecFromNonDirFile(nsIFile      *file,
>+                            nsACString   &url,
>+                            nsIIOService *ioService = nsnull)
>+{
>+    nsresult rv;
>+    nsCOMPtr<nsIFileProtocolHandler> fileHandler;
>+    rv = NS_GetFileProtocolHandler(getter_AddRefs(fileHandler), ioService);
>+    if (NS_SUCCEEDED(rv))
>+        rv = fileHandler->GetURLSpecFromNonDirFile(file, url);
>+    return rv;
>+}
>+
>+inline nsresult
>+NS_GetURLSpecFromDir(nsIFile      *file,
>+                     nsACString   &url,
>+                     nsIIOService *ioService = nsnull)
>+{
>+    nsresult rv;
>+    nsCOMPtr<nsIFileProtocolHandler> fileHandler;
>+    rv = NS_GetFileProtocolHandler(getter_AddRefs(fileHandler), ioService);
>+    if (NS_SUCCEEDED(rv))
>+        rv = fileHandler->GetURLSpecFromDir(file, url);
>+    return rv;
>+}
>+
> /**
>  * Obtains the referrer for a given channel.  This first tries to obtain the
>  * referrer from the property docshell.internalReferrer, and if that doesn't
>  * work and the channel is an nsIHTTPChannel, we check it's referrer property.
>  *
>  * @returns NS_ERROR_NOT_AVAILABLE if no referrer is available.
>  */
> inline nsresult
>diff --git a/netwerk/base/src/nsURLHelper.cpp b/netwerk/base/src/nsURLHelper.cpp
>--- a/netwerk/base/src/nsURLHelper.cpp
>+++ b/netwerk/base/src/nsURLHelper.cpp
>@@ -36,16 +36,17 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsURLHelper.h"
> #include "nsReadableUtils.h"
> #include "nsIServiceManager.h"
> #include "nsIIOService.h"
>+#include "nsILocalFile.h"
> #include "nsIURLParser.h"
> #include "nsIURI.h"
> #include "nsMemory.h"
> #include "nsEscape.h"
> #include "nsCOMPtr.h"
> #include "nsCRT.h"
> #include "nsNetCID.h"
> #include "netCore.h"
>@@ -124,16 +125,70 @@ net_GetNoAuthURLParser()
> nsIURLParser *
> net_GetStdURLParser()
> {
>     if (!gInitialized)
>         InitGlobals();
>     return gStdURLParser;
> }
> 
>+//---------------------------------------------------------------------------
>+// GetFileFromURLSpec implementations
>+//---------------------------------------------------------------------------
>+nsresult
>+net_GetURLSpecFromDir(nsIFile *aFile, nsACString &result)
>+{
>+    nsCAutoString escPath;
>+    nsresult rv = net_GetURLSpecFromFileHelper(aFile, escPath);
>+
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    if (escPath.Last() != '/') {
>+        escPath += '/';
>+    }
>+    
>+    result = escPath;
>+    return NS_OK;
>+}
>+
>+nsresult
>+net_GetURLSpecFromNonDirFile(nsIFile *aFile, nsACString &result)
>+{
>+    return net_GetURLSpecFromFileHelper(aFile, result);
>+}

Naming could use some work in this patch. I'm not great at that either. How about s/NonDirFile/ActualFile/? Looks to me like GetURLSpecFromFileHelper should be called net_GetURLSpecFromNonDirFile, then we can skip having this extra forwarding function.


Also, Benjamin/Brendan/others, could we maybe modify existing net_GetURLSpecFromFile() by adding a default parameter "bool aIpromiseThisIsAFile=false" so we could skip the stat() without adding too many alternative functions?

>+
>+nsresult
>+net_GetURLSpecFromFile(nsIFile *aFile, nsACString &result)
>+{
>+    // This method does an extra stat().
>+    NS_WARNING("Callers of GetURLSpecFromFile should use \
>+ GetURLSpecFromDir or GetURLSpecFromNonDirFile instead.");

that's not real c++. do NS_WARNING("blah " <newline> "blah2")


Rest of the patch looks good to me. This is a nice API fix, we should clean this up and land it asap.

Comment 5

[Benedict Hsieh [:benedict]]

Attachment: renames NonDirFile --> ActualFile, fixes NS_WARNING

A few concerns about this patch:
- I'm not sure the NS_WARNING should remain: there are legitimate callers of the undifferentiated function. However, it'd be nice to ask people to move away from that function when possible.

- It would be nice not to have three versions of the same function. Taras suggests a bool, but we really need a flag for: ActualFile / Dir / Either. An enum kind of works, but there is code that calls NS_GetURLSpec, net_GetURLSpec, and fileHandler->GetURLSpec, and callers of all of these functions would need to know about the enum, which is also painful.

Comment 6

[Benedict Hsieh [:benedict]]

Attachment: ack hg only got half of my last patch

see above comments.

Comment 7

[Benjamin Smedberg]

Comment on attachment 402253 [details] [diff] [review]
ack hg only got half of my last patch

I am not a peer of any of this code except perhaps accidentally the xpconnect stuff.

Comment 8

[Benedict Hsieh [:benedict]]

More numbers. Small sample size, caveat emptor, ymmv, etc etc. "Patched" numbers are with both this patch AND patch in bug 511761 applied.

cold, no patch:
10829 10929 10395 13340 9573 10253
average: 10886
--------
cold, patch:
9707 11376 10563 11323 9590 10834
average: 10565

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 402253 [details] [diff] [review]
ack hg only got half of my last patch

+++ b/netwerk/base/public/nsNetUtil.h
+NS_GetURLSpecFromActualFile(nsIFile      *file,
+                      nsACString   &url,
+                      nsIIOService *ioService = nsnull)

this method needs a comment describing it. also, the indentation for the parameter lines is wrong.

the same goes for NS_GetURLSpecFromDir

+++ b/netwerk/base/src/nsURLHelper.cpp
+    nsresult rv = net_GetURLSpecFromActualFile(aFile, escPath);
+
+    if (NS_FAILED(rv))

please delete that newline
(similar in the other function)

+++ b/netwerk/protocol/file/public/nsIFileProtocolHandler.idl

the IDL needs a new IID

+++ b/netwerk/protocol/file/src/nsFileProtocolHandler.cpp
+nsFileProtocolHandler::GetURLSpecFromActualFile(nsIFile *file, nsACString &result)

line too long, please limit lines to 80 characters

Comment 10

[Benedict Hsieh [:benedict]]

Attachment: addresses above comments

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 404326 [details] [diff] [review]
addresses above comments

sorry, missed this one before, but otherwise looks good:
+++ b/netwerk/base/src/nsURLHelperOS2.cpp
+net_GetURLSpecFromActualFile(nsIFile *aFile, nsACstring &result)

nsACstring -> nsACString

Comment 12

[Benedict Hsieh [:benedict]]

Attachment: fixes typo

Comment 13

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/0b299d45734b

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 404336 [details] [diff] [review]
fixes typo

This could improve startup time for Fennec

Comment 15

[Boris Zbarsky [:bzbarsky]]

So...  After this fix, my console is full of warning spew:

WARNING: If possible, callers of GetURLSpecFromFile should use GetURLSpecFromDir or GetURLSpecFromActualFile instead.: file /Users/bzbarsky/mozilla/css-frameconst/mozilla/netwerk/base/src/nsURLHelper.cpp, line 157

Hundreds of those.  In particular, every single nsIOService::NewFileURI call leads to such a warning, via nsFileProtocolHandler::NewFileURI and nsStandardURL::SetFile.  This happens all over the place in the chrome registry, etc.

Can we either remove the warning or change the caller or something?  This makes actually using a debug build painful.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I filed bug 522025 on that.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 404336 [details] [diff] [review]
fixes typo

a=jst, but we should take the followup debug spew fix bug 522025 as well at the same time.

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d2c5fc49dbb1

Comment 19

[David Teller [:Yoric] - still alive but not very active]

We actually hit this a lot as part of bug 890712, in particular every time we use Cu.import.