More checks on missing error handling... pretty embarrassing if mailnews code doesn't check for file operation errors ;)

The commits of bug 1296164 provide useful examples.

In m.d.platform they suggested that perhaps we could just accept warnings for now rather than fix these issues. It seems like they are determined to add these must_use features all over the place, so this is going to be a continuing source of make-work for us at a time.

Just the nine failures I'm seeing or more? 5x nsMsgDBFolder.cpp and 4x nsMsgBrkMBoxStore.cpp?

(In reply to Kent James (:rkent) from comment #2) > In m.d.platform they suggested that perhaps we could just accept warnings > for now rather than fix these issues. It seems like they are determined to > add these must_use features all over the place, so this is going to be a > continuing source of make-work for us at a time. It's basically automated bug detection at compile time, at least if you consider not dealing with file i/o errors a bug.

OK, approach taken here is: Unused << GetPersistentDescriptor() Create() + CreateUnique(): NS_ENSURE_SUCCESS(rv, rv); Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3fb59834be25 Hmm, doesn't compile locally on Windows, complains: nsMsgDBFolder.cpp(620): error C2039: 'Unused': is not a member of 'mozilla' and also without the mozilla:: Maybe I need to refresh? That's going to take an hour.

(In reply to Jorg K (GMT+2, PTO during summer) from comment #3) > Just the nine failures I'm seeing or more? > 5x nsMsgDBFolder.cpp and 4x nsMsgBrkMBoxStore.cpp? plus mailnews/imap/src/nsImapMailFolder.cpp line 1738 mail/components/migration/src/nsSeamonkeyProfileMigrator.cpp line 535,542,572,450,558,432 mailnews/base/util/nsMsgIncomingServer.cpp line 965,908 mailnews/local/src/nsMsgMaildirStore.cpp line 726,625,809 mailnews/base/util/nsMsgDBFolder.cpp 3647,620,915,1333,1371

Oops, forgot some rv = ... New try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8cc27090163e https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=53cb74790933

(In reply to aleth [:aleth] from comment #7) > > 5x nsMsgDBFolder.cpp and 4x nsMsgBrkMBoxStore.cpp? > plus > mailnews/imap/src/nsImapMailFolder.cpp line 1738 > mail/components/migration/src/nsSeamonkeyProfileMigrator.cpp line > 535,542,572,450,558,432 > mailnews/base/util/nsMsgIncomingServer.cpp line 965,908 > mailnews/local/src/nsMsgMaildirStore.cpp line 726,625,809 Thanks. > mailnews/base/util/nsMsgDBFolder.cpp 3647,620,915,1333,1371 Got those already, see above. Aleth, does this compile locally? mozilla/mach build binaries should go in five minutes. Much faster than a try push.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=16ff2ecccbd0 Aleth, can you see whether this compiles locally for you, if not, perhaps add/correct some stuff. I didn't see you on IRC.

Grr, forgot rv = again. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bfbd04eff250

Hmm, nsMsgDBFolder.cpp:620:16: error: no member named 'Unused' in namespace 'mozilla' What am I doing wrong?

Unused << instead of mozilla::Unused << . Hmm. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c5a53b2a661f

Doesn't compile on Windows, neither with Unused nor mozilla::Unused.

Needed #include "mozilla/unused.h" Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d3084f909f7c

Handle error from SetPersistentDescriptor(), only ignore return value of GetPersistentDescriptor(). Kent this builds locally and the try (previous comment #15) hasn't failed so far. 1 AM here now, so if you could kindly check the try and then review.

Oops, one more SetPersistentDescriptor(). I forgot to mention: Builds locally on Windows.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d3084f909f7c is OK for Linux so far, orange due to a build problem (also seen on C-C) but no compile error. So should be good for review.

Comment on attachment 8784167 [details] [diff] [review] v2f. Review of attachment 8784167 [details] [diff] [review]: ----------------------------------------------------------------- I don't understand why you are ignoring errors when getting persistent descriptors. Can you explain the reasoning behind that? ::: mail/components/migration/src/nsSeamonkeyProfileMigrator.cpp @@ +448,5 @@ > targetSigFile->Append(leafName); > > // now write out the new descriptor > nsAutoCString descriptorString; > + mozilla::Unused << targetSigFile->GetPersistentDescriptor(descriptorString); If you are going to explicitly mark Unused (ignoring errors) then it is worth a comment saying WHY you don't care about errors. But actually I don't see why you are ignoring errors. The descriptor string is presumably wrong at this point, why do you continue to write it? Is there any reason you are not doing rv = ...; NS_ENSURE_SUCCESS here like you did above? (Assume this comment is relevant for other similar uses of Unused)

(In reply to Kent James (:rkent) from comment #19) > I don't understand why you are ignoring errors when getting persistent > descriptors. Can you explain the reasoning behind that? Frankly, I don't understand it 100% either. As per comment #1 (The commits of bug 1296164 provide useful examples.) I visited that bug and found: https://hg.mozilla.org/mozilla-central/rev/51410d553f6f https://hg.mozilla.org/mozilla-central/rev/193c7e5b2ee4 https://hg.mozilla.org/mozilla-central/rev/ed27be6470e5 In the third changeset I found: https://hg.mozilla.org/mozilla-central/rev/ed27be6470e5#l1.29 Look further down and they pass that into AddDownloadToDB(). I got curious and wanted to know why, so I looked up the call. There are two versions for the three platforms: Windows: https://dxr.mozilla.org/comm-central/rev/24763f58772d45279a935790f732d80851924b46/mozilla/xpcom/io/nsLocalFileWin.cpp#3186 Does not fail. Linux + Mac: https://dxr.mozilla.org/comm-central/rev/24763f58772d45279a935790f732d80851924b46/mozilla/xpcom/io/nsLocalFileUnix.cpp#1885 https://dxr.mozilla.org/comm-central/rev/24763f58772d45279a935790f732d80851924b46/mozilla/xpcom/io/nsLocalFileUnix.cpp#600 Does not fail. Looking at https://dxr.mozilla.org/comm-central/search?q=GetPersistentDescriptor&redirect=true there are two more call sites where they *do* check the return value.

Always checking all statuses.

Oops, forgot to remove the include, since we're not using "Unused" any more. Magnus, maybe you have time for a review. Maybe v3b is better. Please use interdiff with v2f.

Comment on attachment 8784248 [details] [diff] [review] v3a. Review of attachment 8784248 [details] [diff] [review]: ----------------------------------------------------------------- I agree this (checking for errors, not ignoring) is the right approach. r=mkmelin ::: mailnews/base/util/nsMsgDBFolder.cpp @@ +68,5 @@ > #include "nsMsgUtils.h" > #include "nsIMsgFilterService.h" > #include "nsDirectoryServiceUtils.h" > #include "mozilla/Services.h" > +#include "mozilla/unused.h" don't need this anymore ::: mailnews/imap/src/nsImapMailFolder.cpp @@ +1736,5 @@ > parentPathFile->IsDirectory(&isDirectory); > + if (!isDirectory) { > + rv = parentPathFile->Create(nsIFile::DIRECTORY_TYPE, 0700); > + NS_ENSURE_SUCCESS(rv, rv); > + } else please add braces for the else too

Comment on attachment 8784252 [details] [diff] [review] v3b. Review of attachment 8784252 [details] [diff] [review]: ----------------------------------------------------------------- Yup

It looks like this is causing some new mozmill failures?

I don't see how checking errors after file creation would cause such massive failures. Xpcshell is busted big time, too, so I assume we have some more JS changes to deal with.

(In reply to Jorg K (GMT+2, PTO during summer) from comment #27) > I don't see how checking errors after file creation would cause such massive > failures. Xpcshell is busted big time, too, so I assume we have some more JS > changes to deal with. Sure, this is unexpected. But looking at the log, the failures are e.g. JavaScript error: chrome://messenger/content/AccountManager.js, line 1223: NS_ERROR_FILE_ALREADY_EXISTS: Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIMsgIncomingServer.localPath] And this just added file i/o error checks...

Or maybe I'm wrong: 02:24:59 INFO - PROCESS | 6355 | [6355] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520008: file /builds/slave/tb-c-cen-m64-d-000000000000000/build/mailnews/base/util/nsMsgIncomingServer.cpp, line 909 908 rv = localPath->Create(nsIFile::DIRECTORY_TYPE, 0755); 909 NS_ENSURE_SUCCESS(rv, rv); Maybe they skipped a failure here.

OK, let's deal with this: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d0427d9d46dc Meanwhile, I cancelled the dailies.

Looks like crappy code. What's the expected behaviour when the file already exists? Append or overwrite? Or is this something won't happen in real life, but only in tests? Are we supposed to guess?

It's a directory!

This is haunting us. Here a try run (same as in comment #30): https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d0427d9d46dc

I'll take the liberty to add this to more call sites where we call: Create(nsIFile::DIRECTORY_TYPE, ... I have the feeling that it's needed here as well: https://hg.mozilla.org/comm-central/rev/4c8dfbee8681#l3.34 The other call sites check before they call Create().

Yep. That other call site needed the same treatment: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b8c660b718ae Can't be long now ;-)

(In reply to Jorg K (GMT+2, PTO during summer) from comment #32) > It's a directory! Ah, that's not so bad then ;) Thanks for fixing it up.

OK, next try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=44a362f04d8d Had to push an obvious bustage fix in the meantime: https://hg.mozilla.org/comm-central/rev/7a1683fe4e2d Can't be long now, yeah, right. Back in three hours.

I am quite tied up with the preparation of an overseas business trip right now and failed to notice this issue quickly enough, but I have added many error checks (at least print warnings in debug build.) for file I/O operation as much as possible for my enabling buffer code. (For example, Bug 1242030 - Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500 ) I would check if my patches cope with the new error thingy caused by must_use property.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=44a362f04d8d Hmm, still not ready, some Xpcshell test failures now: TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js | copyListener.OnStopCopy - [copyListener.OnStopCopy : 195] 2152857608 == 0 TEST-UNEXPECTED-FAIL | mailnews/local/test/unit/test_nsIMsgLocalMailFolder.js | xpcshell return code: 0 Mozmill failures reported here: Bug 1297761

Turned out that one call site needed the same treatment. Tests pass now.

(In reply to ISHIKAWA, Chiaki from comment #38) > I am quite tied up with the preparation of an overseas business trip right > now and failed to notice this issue quickly enough, but I have added many > error checks (at least print warnings in debug build.) for file I/O > operation as much as possible for my enabling buffer code. (For example, > Bug 1242030 - Consolidated patch set from bug 1122698, bug 1134527, bug > 1134529, bug 1174500 ) > I would check if my patches cope with the new error thingy caused by > must_use property. I need to tweak my patches to accommodate some diff changes, and ran |make mozmill| test after updating the tree locally. I saw many errors from |make mozmill|, 63 to be exact. Usually I get 6-9 (depending on the network issue). I looked at the log and saw these errors: I am omitting the last part of the listing (the decreasing order of occurences.) ======================================== NS_ERROR_ (except for NS_ERROR_FACTORY_NOT_REGISTERED and createKeyedServer duplicate server error) ======================================== 171 JavaScript error: chrome://messenger/content/folderPane.js, line 2250: NS_ERROR_FILE_ALREADY_EXISTS: Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIMsgFolder.abbreviatedName] 43 JavaScript error: chrome://messenger/content/folderWidgets.xml, line 564: NS_ERROR_FILE_ALREADY_EXISTS: Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIMsgFolder.prettyName] 21 JavaScript error: resource://mozmill/modules/frame.js -> file:///NREF-COMM-CENTRAL/comm-central/mail/test/mozmill/shared-modules/test-folder-display-helpers.js -> file:///NREF-COMM-CENTRAL/comm-central/mailnews/test/resources/logHelper.js, line 385: NS_ERROR_FILE_ALREADY_EXISTS: Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIMsgFolder.prettiestName] 10 JavaScript error: chrome://messenger/content/about-support/gfx.js, line 46: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName] 5 JavaScript error: resource:///modules/activity/alertHook.js, line 48: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIMsgMailNewsUrl.server] 5 JavaScript error: chrome://messenger/content/folderWidgets.xml, line 705: NS_ERROR_FILE_ALREADY_EXISTS: Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIMsgFolder.prettyName] 4 JavaScript error: chrome://messenger/content/AccountManager.js, line 1223: NS_ERROR_FILE_ALREADY_EXISTS: Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIMsgIncomingServer.localPath] 3 JavaScript error: chrome://messenger/content/specialTabs.js, line 1220: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver] 3 EXCEPTION: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMsgLocalMailFolder.createLocalSubfolder] 2 EXCEPTION: Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIMsgIncomingServer.localPath] 2 EXCEPTION: Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIMsgFolder.prettiestName] 2 EXCEPTION: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMsgFolder.getChildNamed] 1 [18795] WARNING: NS_ENSURE_SUCCESS(childCV->SetForceCharacterSet(msgCharSet), NS_ERROR_FAILURE) failed with result 0x80070057: file /NREF-COMM-CENTRAL/comm-central/mailnews/compose/src/nsMsgCompose.cpp, line 1618 1 JavaScript error: resource://mozmill/stdlib/EventUtils.js, line 342: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendKeyEvent] Maybe my refreshing has not picked up the latest patch in comment 42?