187031 - [nsLocalFileWin] nsLocalFile::Remove returns os remove values (-1) as nsresults

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

WARNING: cacheMap->DeleteStorage() failed., file
i:/build/mozilla/netwerk/cache/src/nsDiskCacheStreams.cpp, line 552
###!!! ASSERTION: Flush() failed: 'NS_SUCCEEDED(rv)', file
i:/build/mozilla/netwerk/cache/src/nsDiskCacheStreams.cpp, line 508

nsDebug::Assertion(const char * 0x044d6360, const char * 0x044d634c, const char
* 0x044d6310, int 508) line 280 + 13 bytes
nsDiskCacheStreamIO::CloseOutputStream(nsDiskCacheOutputStream * 0x14874fd0)
line 508 + 38 bytes
nsDiskCacheOutputStream::Close(nsDiskCacheOutputStream * const 0x14874fd0) line 227
nsDiskCacheOutputStream::~nsDiskCacheOutputStream() line 218
nsDiskCacheOutputStream::`scalar deleting destructor'(unsigned int 1) + 15 bytes
nsDiskCacheOutputStream::Release(nsDiskCacheOutputStream * const 0x14874fd0)
line 204 + 147 bytes
nsCOMPtr<nsIOutputStream>::~nsCOMPtr<nsIOutputStream>() line 491
nsCacheEntryDescriptor::nsOutputStreamWrapper::~nsOutputStreamWrapper() line 138
+ 11 bytes
nsCacheEntryDescriptor::nsOutputStreamWrapper::`scalar deleting
destructor'(unsigned int 1) + 15 bytes
nsCacheEntryDescriptor::nsOutputStreamWrapper::Release(nsCacheEntryDescriptor::nsOutputStreamWrapper
* const 0x1494fa50) line 635 + 182 bytes
nsCOMPtr<nsIOutputStream>::assign_assuming_AddRef(nsIOutputStream * 0x00000000)
line 473
nsCOMPtr<nsIOutputStream>::assign_with_AddRef(nsISupports * 0x00000000) line 915
nsCOMPtr<nsIOutputStream>::operator=(nsIOutputStream * 0x00000000) line 585
nsStreamListenerTee::OnStopRequest(nsStreamListenerTee * const 0x1494faa8,
nsIRequest * 0x10c50da0, nsISupports * 0x00000000, unsigned int 0) line 65

rv is 0xFFFFFFFF (-1, except that nsresult is unsigned...)

So we tracked the problem down to nsLocalFileWin's nsLocalFile::Remove impl
returning the raw value from windows' ::remove/::rmdir which was -1 (error).

Comment 1

[timeless]

Attachment: copy the logic from nsLocalFileUnix

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 110284 [details] [diff] [review]
copy the logic from nsLocalFileUnix

Where is NSRESULT_FOR_ERRNO defined for windows?

Comment 3

[timeless]

Attachment: compiling version

Comment 4

[timeless]

Comment on attachment 110488 [details] [diff] [review]
compiling version

wrt macos, the Errors listed appear in
MPW-GM\Interfaces&Libraries\Interfaces\CIncludes\errno.h

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 110488 [details] [diff] [review]
compiling version

does this compile on the mac classic?  I think that you have to wrap that
#include <errno.h> and NSRESULT_FOR_RETURN in some define.

Not sure what the nsNativeCharsetUtils.cpp
 changes are about.

Can we move the OS2 version of this error mapping function into the common
header?

Comment 6

[timeless]

Attachment: cleans out recursive include and builds on os/2

those other changes were simple optimizations/cleanup that have nothing to do
with this bug, i'm not sure how they ended up in that diff - they will not be
in subsequent diffs for this bug.

ok, so there was this really strange dance:
nsLocalFilePlat.cpp included nsLocalFilePlat.h which included nsLocalFile.h
which included nsLocalFilePlat.h which upset things eventually on os/2.

and most nsLocalFilePlat.h's and the nsLocalFileUnix.cpp included
nsILocalFile.h

this was really messy, so I changed it so that only nsLocalFile.h includes
nsILocalFile.h and the Plat.cpp files only include nsLocalFile.h which then
includes nsLocalFilePlat.h

I'm still relatively confident that the mac changes work since they rely on
includes from mpw's standard include directory. It's also present in
CodeWarrior:
CodeWarrior 7.2\Metrowerks CodeWarrior\MSL\MSL_C\MSL_Common\Include\errno.h

This should just work, i'll have a mac buddy test before i commit.

Comment 7

[timeless]

Attachment: supplemental patch for mac and unix

ok, kaie built on mac with the patch, it needed one minor change (xpinstall).
this supplemental patch also makes to changes for unix font catalogs.

Comment 8

[Doug Turner (:dougt)]

can you post a complete patch for review?

Comment 9

[timeless]

Attachment: combined

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 111382 [details] [diff] [review]
combined

some platforms have no notion of ERRNO.  Maybe nsLocalFile.h isn't the right
place for this macro?  Thoughts?

Where and how have you tested this patch.

Comment 11

[timeless]

kaie built a mac tree based on the patch (cfm i believe)
mkaply built on os/2
i built on linux (xlib and gtk) and windows.

to me we have two issues, correctness of handling os api calls (check) and
compiling of various include changes (checked on the platforms listed).

cls: do i need to be more careful about errno?

Comment 12

[hacker formerly known as [e-mail]]

The classic mac builds would be the only ones I'd worry about regarding the lack
of errno.  If kaie said it worked there, then that should be fine.  

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 111382 [details] [diff] [review]
combined

shouldn't the nsresultForErrno be in some kind of #ifdef?  I don't think the
errno's are xp.

I also adjusted your super review request.  darin is required to review this
checkin (at least the xpcom.io part)

Comment 14

[Darin Fisher]

Comment on attachment 111382 [details] [diff] [review]
combined

>Index: mozilla/xpcom/io/nsLocalFileOS2.h

>+#ifdef XP_OS2_VACPP
>+#define ENOTDIR EBADPOS
>+#endif

nit: this deserves a comment i think.


>Index: mozilla/xpcom/io/nsLocalFileUnix.h
>Index: mozilla/xpcom/io/nsLocalFileWin.cpp

>-                remove(backup.get());
>+                (void) remove(backup.get());

ick!  please don't :)


otherwise, this looks fine to me.  XP error mapping is probably for the best. 
sr=darin

Comment 15

[timeless]

that (void) markers are for consistency with the file, i'll remove the others
from the file to match your review.

as for the define, um.. mkaply, help?

Comment 16

[Mike Kaply [:mkaply]]

We don't have all of the E error values in the VACPP compiler and it made a lot
more sense to #ifdef it at the top.

If it makes you feel better, all the VACPP stuff will go away in a few weeks :)

Comment 17

[Darin Fisher]

timeless: only 1 other instance of (void) prefix in nsLocalFileWin.cpp ;)

Comment 18

[timeless]

Attachment: no (void)'s :)

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 125840 [details] [diff] [review]
no (void)'s :)

r=dougt

Comment 20

[timeless]

checked in