522025 - Incessant slew of GetURLSpecFromFile warnings

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

With the fix for bug 510991, my console is now filled with hundreds of lines of spew like this:

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

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 1

[(dormant account)]

Looks like this is just touchup work leftover from bug 510991. Once again most of the callers already know whether the url is a file/dir, so there is no point in syscalling.

Comment 2

[Benedict Hsieh [:benedict]]

Yeah, I asked about this earlier (both the issue that makes this annoying to fix, and also whether the warning is a good idea):
(https://bugzilla.mozilla.org/show_bug.cgi?id=510991#c5)

The basic problem before and now is that a bunch of code does the same thing for file/directory/not-sure and then calls getURLSpecFromFile. So even though some caller up the chain might know whether it's a file or dir, that information has to be passed down through (in this case) NewFileURI -> fileHandler::NewFileURI -> SetFile -> GetURLSpecFromFile. Also, some of these interfaces are frozen.

It's a little bit weird to me that GetURLSpecFromFile even guarantees the trailing '/' for directories, given that both nsURI and nsIFile are practically just strings representing where a file or directory might be.

I think that we've gotten the relevant stat()s out of the startup process. It's definitely worth fixing the callers eventually, but if this is really annoying right now maybe the best thing to do is just remove the warning for now.

Comment 3

[Benedict Hsieh [:benedict]]

Attachment: removes warning

also adds some comments directing callers to use the non-statting versions, which callers will inevitably ignore.

Comment 4

[Benedict Hsieh [:benedict]]

If we want to revisit this in the future, I think that one approach would be to add some method to nsILocalFile (or a new interface which inherits from it) to allow callers to guarantee certain properties about the file, like whether it's actually a directory or not. 

That would avoid the problem of needing to split up or pass flags through all these call chains, and then just before whatever system call we could check to see if someone has already guaranteed whatever property of the file. We'd be able to fold the GetURLSpecFromActualFile/Dir/File guys back together, too.

Comment 5

[Benedict Hsieh [:benedict]]

Comment on attachment 406134 [details] [diff] [review]
removes warning

just to be clear, this patch should not make any changes to the release build whatsoever. it adds two comments and removes one warning.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/dbe3efa8ff5a

Comment 7

[Dão Gottwald [:dao]]

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