478549 - CopyTo fails on Mac OS X when destination file name arg is null

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Mark Banner (:standard8)]

The landing of bug 476230 caused Thunderbird tinderboxes to go orange. There are two tests failing on a nsIFile::CopyTo call.

I haven't had time to investigate fully, there are two tests that fail on Mac with Trunk builds, but not on 1.9.1 (and likewise I can back out bug 476230 and it won't fail either).

http://hg.mozilla.org/comm-central/file/e61d893526ba/mailnews/local/src/nsLocalMailFolder.cpp#l1967

Debug Test output from one of the tests (ignore the component loader failures, they aren't anything to do with this):

TEST-UNEXPECTED-FAIL | ../../../mozilla/_tests/xpcshell-simple/test_mailnewsbase/unit/test_nsIMsgFolderListenerLocal.js | test failed, see log
../../../mozilla/_tests/xpcshell-simple/test_mailnewsbase/unit/test_nsIMsgFolderListenerLocal.js.log:
>>>>>>>
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/moztest/comm/trunk/src/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp, line 1495
JS Component Loader: ERROR (null):0
                     uncaught exception: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXPCComponents_Utils.import]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///Users/moztest/comm/trunk/tb/mozilla/dist/bin/components/nsActivity.js :: <TOP_LEVEL> :: line 40"  data: no]
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/moztest/comm/trunk/src/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp, line 1495
JS Component Loader: ERROR (null):0
                     uncaught exception: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXPCComponents_Utils.import]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///Users/moztest/comm/trunk/tb/mozilla/dist/bin/components/nsActivityManager.js :: <TOP_LEVEL> :: line 40"  data: no]
*** test pending
Directory request for: MailD that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: MFCaF that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: DefRt that we (mailDirService.js) are not handling, leaving it to another handler.
*** test pending
*** test finished
*** running event loop
WARNING: NS_ENSURE_TRUE(aWindow) failed: file /Users/moztest/comm/trunk/src/mailnews/base/util/nsMsgDBFolder.cpp, line 1614
WARNING: NS_ENSURE_TRUE(aWindow) failed: file /Users/moztest/comm/trunk/src/mailnews/base/util/nsMsgDBFolder.cpp, line 1614
WARNING: NS_ENSURE_TRUE(aWindow) failed: file /Users/moztest/comm/trunk/src/mailnews/base/util/nsMsgDBFolder.cpp, line 1614
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/moztest/comm/trunk/src/mailnews/local/src/nsLocalMailFolder.cpp, line 1968
*** exiting
*** TEST-UNEXPECTED-FAIL | /Users/moztest/comm/trunk/src/mozilla/../mailnews/base/test/resources/msgFolderListenerSetup.js | 2147500037 == 0
JS frame :: /Users/moztest/comm/trunk/src/mozilla/testing/xpcshell/head.js :: do_throw :: line 109
JS frame :: /Users/moztest/comm/trunk/src/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 128
JS frame :: /Users/moztest/comm/trunk/src/mozilla/../mailnews/base/test/resources/msgFolderListenerSetup.js :: anonymous :: line 154
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
JS frame :: ../../../mozilla/_tests/xpcshell-simple/test_mailnewsbase/unit/test_nsIMsgFolderListenerLocal.js :: copyFolders :: line 73
JS frame :: ../../../mozilla/_tests/xpcshell-simple/test_mailnewsbase/unit/test_nsIMsgFolderListenerLocal.js :: testMoveFolder1 :: line 167
JS frame :: ../../../mozilla/_tests/xpcshell-simple/test_mailnewsbase/unit/test_nsIMsgFolderListenerLocal.js :: doTest :: line 237
JS frame :: /Users/moztest/comm/trunk/src/mozilla/testing/xpcshell/head.js :: anonymous :: line 70
JS frame :: /Users/moztest/comm/trunk/src/mozilla/testing/xpcshell/head.js :: anonymous :: line 70
*** FAIL ***
WARNING: nsExceptionService ignoring thread destruction after shutdown: file /Users/moztest/comm/trunk/src/mozilla/xpcom/base/nsExceptionService.cpp, line 194
recorder: started(5), aborted(5), completed(0), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(0), breaks(0), returns(0), unstableInnerCalls(0)
monitor: triggered(0), exits(0), type mismatch(0), global mismatch(0)
nsStringStats
 => mAllocCount:           2682
 => mReallocCount:          168
 => mFreeCount:            2681  --  LEAKED 1 !!!
 => mShareCount:            358
 => mAdoptCount:            145
 => mAdoptFreeCount:        145

<<<<<<<

Comment 1

[Serge Gautherie (:sgautherie)]

Maybe these two tests could be temporarily disabled on trunk, to let the following tests run (and the box go green) ?

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> Maybe these two tests could be temporarily disabled on trunk, to let the
> following tests run (and the box go green) ?

Although we're not actively developing on comm-central + mozilla-central, we are running those boxes to track regressions. This is a clear regression and is an obvious failure in Mac for mailnews - the first test failure is showing the fact that when we copy a mailbox folder, it will fail.

This is not good for folks that are build + testing with mozilla-central and I would rather show this failure than hide it as if it didn't matter.

Comment 3

[Josh Aas]

I'm going to try to fix this right now or I'll back out the patch soon.

Comment 4

[Josh Aas]

Attachment: fix v1.0

This probably fixes the error though I did not reproduce the exact failure here. We should take this patch whether or not it fixes this bug, if it isn't the fix for this bug I'll move it to a new bug.

Comment 5

[Josh Aas]

Attachment: alternative fix, permissions, v1.0

This is an alternative fix in case the first one doesn't work. The nsIFile API does not specify whether or not permissions errors in a source directory should cause a copy to fail, this allows copy operations to continue after a source permission error. We should fix up the documentation either way, but for now lets see if this is necessary.

Comment 6

[Josh Aas]

I used xpcshell to verify that the first fix does in fact fix a bug where we don't handle null as a valid destination name argument.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 362425 [details] [diff] [review]
fix v1.0

Yep, fails without this patch, passes with. Thanks for fixing this.

+  if (newName.Length() > 0) {

My only comment would be that using:

if (!newName.IsEmpty()) would be slightly clearer.

Not sure if my review counts here, but r=me anyway ;-)

Comment 8

[Josh Aas]

pushed to mozilla-central, thanks for the info Mark

http://hg.mozilla.org/mozilla-central/rev/068a8108e623

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> the first test failure is showing the
> fact that when we copy a mailbox folder, it will fail.

Agreed: I didn't know it was that much broken.

***

V.Fixed, as the tinderbox turned green :-)

Comment 13

[Marcia Knous [:marcia]]

This looks good as I am no longer seeing the failed update problem using the Trunk that I was seeing in Bug 478450. Thanks for the fix.