187014 - files referencing things that don't exist don't fire onload

Status: VERIFIED FIXED

(Core :: Networking: File, defect, P1)

Description

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

I've been seeing this problem with gcc builds on Linux (both releases and my own
gcc 3.2 builds), but I suspect it doesn't happen on Windows.  It's
compiler-dependent, since it depends on what's in a spot of memory where there's
an uninitialized variable.

For about a month now I've been having major problems running the layout
regression tests, since many files weren't firing their onload.  I worked around
this by watching as the tests run, and manually hitting "Back" in viewer to prod
them to go on every time they hung.

After a month of hoping the problem would go away, I binary searched builds to
find the source of the problem, and the problem started with revision 1.47 of
nsFileProtocolHandler.cpp, when dougt turned on HTML directory listings.

The problem, as it turns out, is the use of uninitialized variablems.  It seems
that the implementation of nsIFile on Unix (perhaps this is inconsistent with
other platforms -- I'll check in a minute) returns failure when the file doesn't
exist, and doesn't fill in the out parameter.  The nsFileChannel code doesn't
check the return value and just uses the uninitialized |isDirectory| variable.

I'll attach a trivial patch that fixes / works around the problem, but other
fixes may be desirable.  I'd like to get something (this patch is harmless) in
reasonable soon, since I run the regression tests on different trees and I'd
like to avoid having to patch all of my trees...

Comment 1

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

Attachment: trivial patch

Also, I forgot to mention the steps to reproduce the bug:  load
file:///builds/trunk/mozilla/layout/html/tests/block/bugs/10777.html (or the
equivalent for your tree's location), and watch the throbber keep spinning
forever.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 110280 [details] [diff] [review]
trivial patch

I would be more happy with a call to Exists() or checking the for the error
code of IsDirectory().

Comment 3

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

Attachment: patch to make localfile implementations consistent

This makes the Unix and OSX nsLocalFile implementations consistent with the
others.

Comment 4

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

So how about making the nsLocalFile implementations consistent, and then you can
fix the FTP code as you want?

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 110870 [details] [diff] [review]
patch to make localfile implementations consistent

we shouldn't be trusting out parameters if there is a failure result code
returned, but I am sure we do it all over the place, so r=dougt.

Comment 6

[Alec Flett]

Comment on attachment 110870 [details] [diff] [review]
patch to make localfile implementations consistent

sr=alecf

Comment 7

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

OK, I checked in attachment 110870 [details] [diff] [review] to the trunk, 2003-01-13 15:17 PDT.  Giving
to default Networking:File owner to handle cleaning up the code there.

Comment 8

[Doug Turner (:dougt)]

i am marking this as fixed because dbaron did fix the bug mentioned here.  

fixing all of the callers to not trust the out parameters if there is a failure
result code is a swell task, but i really don't have any time for that.  File a
new bug if you want, but I don't think it is very important.

Comment 9

[benc]

VERIFIED/bonsai:
david, if you put the cvs checkin output, you can verify your own code-change
bugs... (saves QA time for something we don't directly test...)

nsLocalFileOSX changed per patch.

nsLocalFileUnix.cpp changed 1.104, rather than 1.103.
1.105	dbaron%dbaron.org	Jan 13 15:17	 	Make implementations consistent across
platforms: ensure IsDirectory and IsFile return false even when the file doesn't
exist (and they return an nsresult error status). b=187014 r=dougt sr=alecf