718190 - Intermittent orange on Windows | TEST-UNEXPECTED-FAIL | test_over2GBMailboxes.js (NS_ERROR_FILE_NO_DEVICE_SPACE)

Status: RESOLVED WORKSFORME

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

See start of discussion in bug 717182.

{
TEST-UNEXPECTED-FAIL | e:\buildbot\comm-central-win32-opt-unittest-xpcshell\build\xpcshell\tests\mailnews\local\test\unit\test_over2GBMailboxes.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x80520010 (NS_ERROR_FILE_NO_DEVICE_SPACE) [nsIFileOutputStream.write]"  nsresult: "0x80520010 (NS_ERROR_FILE_NO_DEVICE_SPACE)"  location: "JS frame :: e:/buildbot/comm-central-win32-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/local/test/unit/test_over2GBMailboxes.js :: run_test :: line 42"  data: no]
}

***

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1326442091.1326447411.4402.gz
WINNT 5.2 comm-central-trunk debug test xpcshell on 2012/01/13 00:08:11
(This one was before bug 717182 fix.)
+
http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1326464119.1326468363.26907.gz
WINNT 5.2 try-comm-central opt test xpcshell on 2012/01/13 06:15:19
(Very try run for bug 717182 !)
+
http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTrunk/1326547663.1326552060.8180.gz
WINNT 5.2 comm-central test xpcshell on 2012/01/14 05:27:43
(Current trunk.)

Comment 1

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1328887095.1328890485.22220.gz
WINNT 5.2 try-comm-central opt test xpcshell on 2012/02/10 07:18:15

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) 3 tests which use over 2 GiB files: Document them better, Improve code order, Fix nits wrt their endTest(), Improve related utilities

This first part is "only" to make these tests more similar.
(I am finalizing the actual fixes wrt to size handling...)

NB: Moved lines/blocks as not modified.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 598120 [details] [diff] [review]
(Av1) 3 tests which use over 2 GiB files: Document them better, Improve code order, Fix nits wrt their endTest(), Improve related utilities

Review of attachment 598120 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/test/resources/mailTestUtils.js
@@ +429,5 @@
>  function mark_file_region_sparse(aFile, aRegionStart, aRegionBytes) {
> +  let fileSystem = get_file_system(aFile);
> +  do_print("[mark_file_region_sparse()] File system = " + fileSystem +
> +           ", file region = " + toMiBString(aRegionStart) +
> +           " to " + toMiBString(aRegionBytes));

These 2 lines will be:
+           ", file region = at " + toMiBString(aRegionStart) +
+           " for " + toMiBString(aRegionBytes));

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) 3 tests which use over 2 GiB files: Fix .diskSpaceAvailable handling

The issue was that .diskSpaceAvailable works on (nsILocalFile) .filePath, not on (nsIFile) .filePath.clone().

http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#298
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsILocalFile.idl#126

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 598262 [details] [diff] [review]
(Bv1) 3 tests which use over 2 GiB files: Fix .diskSpaceAvailable handling

Review of attachment 598262 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/local/test/unit/test_over4GBMailboxes.js
@@ +65,5 @@
>    // "Master" do_test_pending(), paired with a do_test_finished() at the end of
>    // all the operations.
>    do_test_pending();
>  
> +  let neededFreeSpace = k4GiB + 0x110000000;

This line won't have the "k4GiB + " part.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) 3 tests which use over 2 GiB files: Document them better, Improve code order, Fix nits wrt their endTest(), Improve related utilities

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) 3 tests which use over 2 GiB files: Fix .diskSpaceAvailable handling

(ToDo: (sparse) file handling patch(es)...)

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 598783 [details] [diff] [review]
(Av1a) 3 tests which use over 2 GiB files: Document them better, Improve code order, Fix nits wrt their endTest(), Improve related utilities

Review of attachment 598783 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/test/unit/test_largeOfflineStore.js
@@ +60,5 @@
>    }
>  
> +  // pref tuning: one connection only, turn off notifications
> +  gIMAPIncomingServer.maximumConnectionsNumber = 1;
> +  let prefBranch = Cc["@mozilla.org/preferences-service;1"]

Please move these prefs to near the start of the run_test function and use Services.prefs instead of getting the prefBranch.

@@ +111,3 @@
>    gOfflineStoreSize = gIMAPInbox.filePath.fileSize;
> +
> +  // Use downloadAllForOffline() to append created messages to local IMAP inbox.

Please revert this comment change. There's no need to repeat the function name, and you've effectively lost the info about being for offline use.

::: mailnews/local/test/unit/test_over2GBMailboxes.js
@@ +29,5 @@
>    }
>  
> +  // "Trash" folder
> +  gLocalTrashFolder = gLocalIncomingServer.rootMsgFolder.getChildNamed("Trash");
> +  gCopyService = Cc["@mozilla.org/messenger/messagecopyservice;1"].getService(Ci.nsIMsgCopyService);

Please convert this to mailServices.copy

@@ +48,4 @@
>    copyFileMessageInLocalFolder(bugmail10, 0, "", null, copyMessages);
>  }
>  
> +// Get message whose msg key is over 2 G.

Either use GiB or don't have the space between the 2 and the G (same a few lines down).

@@ +123,5 @@
>  
> +function endTest() {
> +  // free up disk space - if you want to look at the file after running
> +  // this test, comment out this line.
> +  gLocalInboxFolder.filePath.remove(false);

If we're doing this here, I think we should move it to the same place for test_largeOfflineStore.js as well to be consistent.

::: mailnews/local/test/unit/test_over4GBMailboxes.js
@@ +39,5 @@
>  
> +// This alert() is triggered when file size becomes close (enough) to or
> +// exceeds 4 GiB.
> +// See "nsMsgBrkMBoxStore::HasSpaceAvailable() > (hard coded) 0xFFC00000",
> +// which is 4 GiB - 4 MiB.

Just say see nsMsgBrkMBoxStore::HasSpaceAvailable(). If it ever changes, we don't want to have to change here as well.

@@ +72,5 @@
>        get_file_system(gLocalInboxFolder.filePath) != "NTFS")
>    {
>      dump("On Windows, this test only works on NTFS volumes.\n");
> +
> +    gGotAlert = true; // Fake it.

Just move the do_check_true(gGotAlert) out of endTest and into the one place its currently called from.

::: mailnews/test/resources/mailTestUtils.js
@@ +83,5 @@
>    gLocalInboxFolder.setFlag(Ci.nsMsgFolderFlags.Mail);
>  
>    // Force an initialization of the Inbox folder database.
>    var folderName = gLocalInboxFolder.prettiestName;
> +  do_print("[loadLocalMailAccount()] Local Inbox folder path = " +

We don't need this. iirc the profile is already printed at the start of a test or test run, and that's the important info.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 598784 [details] [diff] [review]
(Bv1a) 3 tests which use over 2 GiB files: Fix .diskSpaceAvailable handling

Review of attachment 598784 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/test/unit/test_largeOfflineStore.js
@@ +39,5 @@
>    inboxFile.append("INBOX");
>    if (!inboxFile.exists())
>      inboxFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, parseInt("0644", 8));
>  
> +  let neededFreeSpace = 0x200000000;

This should be a const.

@@ +55,2 @@
>    let isFileSparse = mark_file_region_sparse(inboxFile, 0, 0x10000000f);
> +  let freeDiskSpace = gIMAPIncomingServer.rootMsgFolder.filePath.diskSpaceAvailable;

Does inboxfile QI to nsILocalFile, if so, that might be a nicer way of doing this.

@@ +55,5 @@
>    let isFileSparse = mark_file_region_sparse(inboxFile, 0, 0x10000000f);
> +  let freeDiskSpace = gIMAPIncomingServer.rootMsgFolder.filePath.diskSpaceAvailable;
> +  do_print("Free disk space = " + toMiBString(freeDiskSpace));
> +  if (!isFileSparse && freeDiskSpace < neededFreeSpace) {
> +    dump("TEST-KNOWN-FAIL | test_largeOfflineStore.js | This test needs " +

I think do_print would be better.

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) 3 tests which use over 2 GiB files: Document them better, Improve code (order), Fix nits wrt their endTest(), Improve related utilities

Av1a, with comment 8 suggestion(s).

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 600790 [details] [diff] [review]
(Av2) 3 tests which use over 2 GiB files: Document them better, Improve code (order), Fix nits wrt their endTest(), Improve related utilities

Review of attachment 600790 [details] [diff] [review]:
-----------------------------------------------------------------

This fails due to:

TEST-UNEXPECTED-FAIL | ../../../resources/mailTestUtils.js:338 | Error: get_file_system is only supported on Windows - See following stack:
JS frame :: ../../../resources/mailTestUtils.js :: get_file_system :: line 338
JS frame :: ../../../resources/mailTestUtils.js :: mark_file_region_sparse :: line 428
JS frame :: /Users/moztest/comm/reviewcc/tb/mozilla/_tests/xpcshell/mailnews/local/test/unit/test_over4GBMailboxes.js :: run_test :: line 99
JS frame :: /Users/moztest/comm/reviewcc/src/mozilla/testing/xpcshell/head.js :: _execute_test :: line 326

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to Mark Banner (:standard8) from comment #11)
> TEST-UNEXPECTED-FAIL | ../../../resources/mailTestUtils.js:338 | Error:
> get_file_system is only supported on Windows

D'oh, I missed that! And I hadn't sent this patch to Try yet and it passes on my Windows 2000 :-<

Then:

This function is only used in (2 of the 3) tests this bug is about.
http://mxr.mozilla.org/comm-central/search?string=get_file_system&case=on

Similar AboutSupport.getFileSystemType() can return 'null'.
http://mxr.mozilla.org/comm-central/source/mail/components/test/unit/test_about_support.js#162

I suggest to make get_file_system() return 'null' instead of throwing in this case.
http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/mailTestUtils.js#329)

Do you agree?

Comment 13

[Serge Gautherie (:sgautherie)]

Mark, ping for feedback on comment 12.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 600790 [details] [diff] [review]
(Av2) 3 tests which use over 2 GiB files: Document them better, Improve code (order), Fix nits wrt their endTest(), Improve related utilities

Ok, I think I'm alright with that function returning null, as long as it is documented.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) 3 tests which use over 2 GiB files: Document them better, Improve code (order), Fix nits wrt their endTest(), Improve related utilities [Checked in: Comment 17]

Av2, with comment 12 suggestion(s).

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=a46babcf9da7
is looking good (on 'OS X opt' so far)...

Comment 16

[Serge Gautherie (:sgautherie)]

Ping for review.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 605602 [details] [diff] [review]
(Av3) 3 tests which use over 2 GiB files: Document them better, Improve code (order), Fix nits wrt their endTest(), Improve related utilities
[Checked in: Comment 17]

http://hg.mozilla.org/comm-central/rev/9694fd9d4a77

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) 3 tests which use over 2 GiB files: Fix .diskSpaceAvailable handling, Improve .fileSize handling [Checked in: See comment 23]

Bv1a, with comment 9 suggestion(s),
and more.


(In reply to Mark Banner (:standard8) from comment #9)

> > +  let neededFreeSpace = 0x200000000;
> 
> This should be a const.

The value won't be "const" in patch C ;-)

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 609450 [details] [diff] [review]
(Bv2) 3 tests which use over 2 GiB files: Fix .diskSpaceAvailable handling, Improve .fileSize handling
[Checked in: See comment 23]

Review of attachment 609450 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/local/test/unit/test_over4GBMailboxes.js
@@ +141,5 @@
>      });
>    } catch (ex) {
> +    // XXX How can this happen?
> +    do_print("copyFileMessageInLocalFolder() threw " + ex);
> +    todo_check_true(false);

This is the only file in which copyFileMessageInLocalFolder() is inside a try+catch.
Would you know why it is needed here? Or can it be removed?

http://mxr.mozilla.org/comm-central/search?string=copyFileMessageInLocalFolder&case=on

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 609450 [details] [diff] [review]
(Bv2) 3 tests which use over 2 GiB files: Fix .diskSpaceAvailable handling, Improve .fileSize handling
[Checked in: See comment 23]

Succeeded as
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=ca516ffb1375
(though Windows tests run is missing :-|)

Comment 21

[Serge Gautherie (:sgautherie)]

Ping for review.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 609450 [details] [diff] [review]
(Bv2) 3 tests which use over 2 GiB files: Fix .diskSpaceAvailable handling, Improve .fileSize handling
[Checked in: See comment 23]

Review of attachment 609450 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments fixed.

::: mailnews/local/test/unit/test_over4GBMailboxes.js
@@ +141,5 @@
>      });
>    } catch (ex) {
> +    // XXX How can this happen?
> +    do_print("copyFileMessageInLocalFolder() threw " + ex);
> +    todo_check_true(false);

We're not in a listener here, so I think you can just remove the try/catch for now.

@@ +149,5 @@
> +  // Make sure inbox file did not grow (i.e., no data were appended).
> +  let localInboxSize = gLocalInboxFolder.filePath.fileSize;
> +  do_print("Local inbox size (after copyFileMessageInLocalFolder()) = " +
> +           localInboxSize);
> +  do_check_true(localInboxSize == gLocalInboxSize);

Should be do_check_eq

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 609450 [details] [diff] [review]
(Bv2) 3 tests which use over 2 GiB files: Fix .diskSpaceAvailable handling, Improve .fileSize handling
[Checked in: See comment 23]

http://hg.mozilla.org/comm-central/rev/7fe3e7bebed6
Bv2, with comment 22 suggestion(s).

Comment 24

[Ed Morley [:emorley]]

Resolving WFM keyword:intermittent-failure bugs last modified >3 months ago, whose whiteboard contains none of:
{random,disabled,marked,fuzzy,todo,fails,failing,annotated,time-bomb,leave open}

There will inevitably be some false positives; for that (and the bugspam) I apologise. Filter on orangewfm.