1297460 - Port Bug 1296164 - Use the [must_use] property in nsIFile.idl

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[aleth [:aleth]]

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

Comment 1

[aleth [:aleth]]

The commits of bug 1296164 provide useful examples.

Comment 2

[Kent James (:rkent)]

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.

Comment 3

[Jorg K (CEST = GMT+2)]

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

Comment 4

[aleth [:aleth]]

(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.

Comment 5

[Jorg K (CEST = GMT+2)]

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.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: v1a.

Comment 7

[aleth [:aleth]]

(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

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: v1b.

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

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: v2.

(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.

Comment 10

[Jorg K (CEST = GMT+2)]

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.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: v2b.

Grr, forgot rv = again.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bfbd04eff250

Comment 12

[Jorg K (CEST = GMT+2)]

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

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: v2c.

Unused << instead of mozilla::Unused << . Hmm.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c5a53b2a661f

Comment 14

[Jorg K (CEST = GMT+2)]

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

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: v2d.

Needed
#include "mozilla/unused.h"

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

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: v2e.

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.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: v2f.

Oops, one more SetPersistentDescriptor().

I forgot to mention: Builds locally on Windows.

Comment 18

[Jorg K (CEST = GMT+2)]

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 19

[Kent James (:rkent)]

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)

Comment 20

[Jorg K (CEST = GMT+2)]

(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.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: v3a.

Always checking all statuses.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: v3b.

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 23

[Magnus Melin [:mkmelin]]

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 24

[Magnus Melin [:mkmelin]]

Comment on attachment 8784252 [details] [diff] [review]
v3b.

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

Yup

Comment 25

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4c8dfbee8681

Comment 26

[aleth [:aleth]]

It looks like this is causing some new mozmill failures?

Comment 27

[Jorg K (CEST = GMT+2)]

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.

Comment 28

[aleth [:aleth]]

(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...

Comment 29

[Jorg K (CEST = GMT+2)]

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.

Comment 30

[Jorg K (CEST = GMT+2)]

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

Meanwhile, I cancelled the dailies.

Comment 31

[aleth [:aleth]]

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?

Comment 32

[Jorg K (CEST = GMT+2)]

It's a directory!

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: 1297460-follow.patch

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

Comment 34

[Jorg K (CEST = GMT+2)]

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().

Comment 35

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 36

[aleth [:aleth]]

(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.

Comment 37

[Jorg K (CEST = GMT+2)]

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.

Comment 38

[ISHIKAWA, Chiaki]

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.

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: 1297460-follow.patch

Comment 40

[Jorg K (CEST = GMT+2)]

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

Comment 41

[Jorg K (CEST = GMT+2)]

Attachment: Deal with folder already existing

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

Comment 42

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4a75a6a737c7

Comment 43

[ISHIKAWA, Chiaki]

(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?