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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(not reading, please use [e-mail] instead)]

[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
             {

Comment 1

[Doug Turner (:dougt)]

Seth, is this blocking you?

Comment 2

[neeti]

me

Comment 3

[neeti]

Seth, how do I reproduce this bug?

Comment 4

[neeti]

Attachment: proposed patch

doug, seth: Attached a patch. Are there any tests I could use to test this?

thanks,
neeti

Comment 5

[(not reading, please use [e-mail] instead)]

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.

Comment 6

[neeti]

Attachment: patch with testcase

Attached a new patch with a test case.

Comment 7

[Doug Turner (:dougt)]

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

Comment 8

[neeti]

Attachment: revised patch with changes suggested above

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 85804 [details] [diff] [review]
revised patch with changes suggested above

r=dougt

Comment 10

[(not reading, please use [e-mail] instead)]

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.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 85804 [details] [diff] [review]
revised patch with changes suggested above

neeti, lets add an assertion like sspitzer suggestions.

Comment 12

[neeti]

Attachment: revised patch using NS_ENSURE_SUCCESS

Comment 13

[Doug Turner (:dougt)]

Attachment: fixes non sequitur

same as neeti's with some minor clean up.

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 86240 [details] [diff] [review]
fixes non sequitur

neeti's code, nits fixed.

r=dougt

Comment 15

[(not reading, please use [e-mail] instead)]

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.

Comment 16

[neeti]

checked in fix