Closed Bug 187031 Opened 22 years ago Closed 21 years ago

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

Categories

(Core :: XPCOM, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: timeless)

Details

Attachments

(1 file, 5 obsolete files)

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).
Attachment #110284 - Flags: superreview?(brendan)
Attachment #110284 - Flags: review?(dougt)
Comment on attachment 110284 [details] [diff] [review]
copy the logic from nsLocalFileUnix

Where is NSRESULT_FOR_ERRNO defined for windows?
Attached patch compiling version (obsolete) — Splinter Review
Attachment #110284 - Attachment is obsolete: true
Attachment #110284 - Flags: superreview?(brendan)
Attachment #110284 - Flags: review?(dougt)
Comment on attachment 110488 [details] [diff] [review]
compiling version

wrt macos, the Errors listed appear in
MPW-GM\Interfaces&Libraries\Interfaces\CIncludes\errno.h
Attachment #110488 - Flags: review?(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?
Attachment #110488 - Flags: review?(dougt) → review-
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.
Attachment #110488 - Attachment is obsolete: true
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.
Attachment #110847 - Flags: superreview?(dveditz)
Attachment #110847 - Flags: review?(dougt)
can you post a complete patch for review?
Attached patch combined (obsolete) — Splinter Review
Attachment #110800 - Attachment is obsolete: true
Attachment #110847 - Attachment is obsolete: true
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.
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?
Status: NEW → ASSIGNED
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.  
Attachment #110847 - Flags: superreview?(dveditz)
Attachment #110847 - Flags: review?(dougt)
Attachment #111382 - Flags: superreview?(dveditz)
Attachment #111382 - Flags: review?(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)
Attachment #111382 - Flags: superreview?(dveditz) → superreview?(darin)
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
Attachment #111382 - Flags: superreview?(darin) → superreview+
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?
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 :)
timeless: only 1 other instance of (void) prefix in nsLocalFileWin.cpp ;)
Attached patch no (void)'s :)Splinter Review
Attachment #111382 - Attachment is obsolete: true
Attachment #125840 - Flags: review?(dougt)
Attachment #111382 - Flags: review?(dougt)
Comment on attachment 125840 [details] [diff] [review]
no (void)'s :)

r=dougt
Attachment #125840 - Flags: review?(dougt) → review+
checked in
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: