Closed Bug 142629 Opened 22 years ago Closed 22 years ago

[win32 only] recursive copy / move errors are not properly bubbled up.

Categories

(Core :: XPCOM, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.0.1

People

(Reporter: sspitzer, Assigned: neeti)

Details

Attachments

(1 file, 4 obsolete files)

[win32 only] recursive copy / move errors are not properly bubbled up.

While working on another bug, I noticed this.

I was trying to move a directory, and one of the files in the directory could
not be moved, due to a sharing error.  The windows error was
ERROR_SHARING_VIOLATION,
here was the stack:

ConvertWinError(unsigned long 32) line 208
nsLocalFile::CopySingleFile(nsIFile * 0x015e06a0, nsIFile * 0x015e09c8, const
char * 0x00000000, int 0, int 1) line 960 + 12 bytes
nsLocalFile::CopyMove(nsIFile * 0x015e09c8, const char * 0x00000000, int 0, int
1) line 1051 + 39 bytes
nsLocalFile::MoveTo(nsLocalFile * const 0x015e06a0, nsIFile * 0x015e09c8, const
char * 0x00000000) line 1196
nsLocalFile::CopyMove(nsIFile * 0x00000000, const char * 0x0012fba4, int 0, int
1) line 1124 + 43 bytes
nsLocalFile::MoveTo(nsLocalFile * const 0x0169c850, nsIFile * 0x00000000, const
char * 0x0012fba4) line 1196
nsProfile::RemigrateProfile(nsProfile * const 0x015db7b8, const unsigned short *
0x015b3eb0) line 2313 + 44 bytes
nsAppShellService::CheckAndRemigrateDefunctProfile() line 389 + 42 bytes
nsAppShellService::DoProfileStartup(nsAppShellService * const 0x015c8070,
nsICmdLineService * 0x015e8e08, int 1) line 256 + 8 bytes
InitializeProfileService(nsICmdLineService * 0x015e8e08) line 1073 + 31 bytes
main1(int 3, char * * 0x003047a0, nsISupports * 0x00000000) line 1371 + 14 bytes
main(int 3, char * * 0x003047a0) line 1766 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e87903()

We need to bubble up any errors.

Here's where to start:

Index: xpcom/io/nsLocalFileWin.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v
retrieving revision 1.82.4.3
diff -u -w -r1.82.4.3 nsLocalFileWin.cpp
--- xpcom/io/nsLocalFileWin.cpp 26 Apr 2002 00:39:53 -0000      1.82.4.3
+++ xpcom/io/nsLocalFileWin.cpp 6 May 2002 21:22:28 -0000
@@ -1121,7 +1121,7 @@
                 if (followSymlinks)
                     rv = NS_ERROR_FAILURE;
                 else
-                    rv = file->MoveTo(target, nsnull);
+                    rv = file->MoveTo(target, nsnull);  // this rv needs to bub
ble out.
             }
             else
             {
Target Milestone: --- → mozilla1.0.1
Seth, is this blocking you?
me
Assignee: dougt → neeti
Seth, how do I reproduce this bug?
Attached patch proposed patch (obsolete) — Splinter Review
doug, seth: Attached a patch. Are there any tests I could use to test this?

thanks,
neeti
no, this is not blocking me anymore as the code that was hitting this was
changed to do something different.

the way to test it would be to write some test code, or use xpcshell and write
some test js.

the way to reproduce the problem:

from your test code, try to move a directory that contains a file that is in use.
Attached patch patch with testcase (obsolete) — Splinter Review
Attached a new patch with a test case.
Comment on attachment 85653 [details] [diff] [review]
patch with testcase

		     rv = NS_ERROR_FAILURE;
		 else
		     rv = file->MoveToNative(target, nsCString());
+		 if (NS_FAILED(rv)) 
+		     return rv;

If something is going to set rv and return, might as well just return the
error:

return NS_ERROR_FAILURE;

rv =  file->MoveToNative(target, nsCString());
+		 if (NS_FAILED(rv)) 
+		     return rv;


nsComponentManager::CreateInstance is not the correct form.  You should use the
NS_GetComponentManager() api.

assuming that this fixes the stated problem, r=dougt
Attachment #85653 - Flags: review+
Comment on attachment 85804 [details] [diff] [review]
revised patch with changes suggested above

r=dougt
Attachment #85804 - Flags: review+
Comment on attachment 85804 [details] [diff] [review]
revised patch with changes suggested above

sr=sspitzer

do we want to assert in any of these scenarios, so that we jump into the
debugger?

instead of if (NS_FAILED(rv)) return rv;

do

NS_ENSURE_SUCCESS(rv,rv);

I'll let the module owner (dougt) decide.
Attachment #85804 - Flags: superreview+
Comment on attachment 85804 [details] [diff] [review]
revised patch with changes suggested above

neeti, lets add an assertion like sspitzer suggestions.
same as neeti's with some minor clean up.
Attachment #84658 - Attachment is obsolete: true
Attachment #85653 - Attachment is obsolete: true
Attachment #85804 - Attachment is obsolete: true
Attachment #86237 - Attachment is obsolete: true
Comment on attachment 86240 [details] [diff] [review]
fixes non sequitur

neeti's code, nits fixed.

r=dougt
Attachment #86240 - Flags: review+
Comment on attachment 86240 [details] [diff] [review]
fixes non sequitur

sr=sspitzer

since we have it, I'd say check in your test code as well.
Attachment #86240 - Flags: superreview+
checked in fix
Status: NEW → RESOLVED
Closed: 22 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: