The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 51.0

Status

Thunderbird
General
--
blocker
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: aleth, Assigned: Jorg K (GMT+1))

Tracking

51 Branch
Thunderbird 51.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

(Reporter)

Description

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

Updated

7 months ago
Severity: normal → blocker
(Reporter)

Comment 1

7 months ago
The commits of bug 1296164 provide useful examples.

Comment 2

7 months ago
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.
(Assignee)

Comment 3

7 months ago
Just the nine failures I'm seeing or more?
5x nsMsgDBFolder.cpp and 4x nsMsgBrkMBoxStore.cpp?
(Reporter)

Comment 4

7 months ago
(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.
(Assignee)

Comment 5

7 months ago
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.
(Assignee)

Comment 6

7 months ago
Created attachment 8784108 [details] [diff] [review]
v1a.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
(Reporter)

Comment 7

7 months ago
(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
(Assignee)

Comment 8

7 months ago
Created attachment 8784110 [details] [diff] [review]
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
(Assignee)

Updated

7 months ago
Attachment #8784108 - Attachment is obsolete: true
(Assignee)

Comment 9

7 months ago
Created attachment 8784114 [details] [diff] [review]
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.
Attachment #8784110 - Attachment is obsolete: true
(Assignee)

Comment 10

7 months ago
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.
Flags: needinfo?(aleth)
(Assignee)

Comment 11

7 months ago
Created attachment 8784117 [details] [diff] [review]
v2b.

Grr, forgot rv = again.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bfbd04eff250
Attachment #8784114 - Attachment is obsolete: true
(Assignee)

Comment 12

7 months ago
Hmm, nsMsgDBFolder.cpp:620:16: error: no member named 'Unused' in namespace 'mozilla'
What am I doing wrong?
(Assignee)

Comment 13

7 months ago
Created attachment 8784128 [details] [diff] [review]
v2c.

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

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c5a53b2a661f
Attachment #8784117 - Attachment is obsolete: true
(Assignee)

Comment 14

7 months ago
Doesn't compile on Windows, neither with Unused nor mozilla::Unused.
(Assignee)

Comment 15

7 months ago
Created attachment 8784149 [details] [diff] [review]
v2d.

Needed
#include "mozilla/unused.h"

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d3084f909f7c
Attachment #8784128 - Attachment is obsolete: true
(Assignee)

Comment 16

7 months ago
Created attachment 8784165 [details] [diff] [review]
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.
Attachment #8784149 - Attachment is obsolete: true
Flags: needinfo?(aleth)
Attachment #8784165 - Flags: review?(rkent)
(Assignee)

Comment 17

7 months ago
Created attachment 8784167 [details] [diff] [review]
v2f.

Oops, one more SetPersistentDescriptor().

I forgot to mention: Builds locally on Windows.
Attachment #8784165 - Attachment is obsolete: true
Attachment #8784165 - Flags: review?(rkent)
Attachment #8784167 - Flags: review?(rkent)
(Assignee)

Comment 18

7 months ago
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

7 months ago
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)
(Assignee)

Comment 20

7 months ago
(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.
(Assignee)

Comment 21

7 months ago
Created attachment 8784248 [details] [diff] [review]
v3a.

Always checking all statuses.
Attachment #8784248 - Flags: review?(rkent)
(Assignee)

Comment 22

7 months ago
Created attachment 8784252 [details] [diff] [review]
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.
Attachment #8784248 - Attachment is obsolete: true
Attachment #8784248 - Flags: review?(rkent)
Attachment #8784252 - Flags: review?(rkent)
Attachment #8784252 - Flags: review?(mkmelin+mozilla)

Comment 23

7 months ago
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
Attachment #8784248 - Attachment is obsolete: false

Comment 24

7 months ago
Comment on attachment 8784252 [details] [diff] [review]
v3b.

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

Yup
Attachment #8784252 - Flags: review?(rkent)
Attachment #8784252 - Flags: review?(mkmelin+mozilla)
Attachment #8784252 - Flags: review+
(Assignee)

Comment 25

7 months ago
https://hg.mozilla.org/comm-central/rev/4c8dfbee8681
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
(Assignee)

Updated

7 months ago
Attachment #8784248 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8784167 - Attachment is obsolete: true
Attachment #8784167 - Flags: review?(rkent)
(Reporter)

Comment 26

7 months ago
It looks like this is causing some new mozmill failures?
(Assignee)

Comment 27

7 months ago
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.
(Reporter)

Comment 28

7 months ago
(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...
(Assignee)

Comment 29

7 months ago
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.
(Assignee)

Comment 30

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

Meanwhile, I cancelled the dailies.
(Reporter)

Comment 31

7 months ago
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?
(Assignee)

Comment 32

7 months ago
It's a directory!
(Assignee)

Updated

7 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 33

7 months ago
Created attachment 8784326 [details] [diff] [review]
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
Attachment #8784326 - Flags: review?(mkmelin+mozilla)

Updated

7 months ago
Attachment #8784326 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 34

7 months ago
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().
(Assignee)

Comment 35

7 months ago
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 ;-)
(Reporter)

Comment 36

7 months ago
(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.
(Assignee)

Comment 37

7 months ago
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

7 months ago
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.
(Assignee)

Comment 39

7 months ago
Created attachment 8784432 [details] [diff] [review]
1297460-follow.patch
Attachment #8784326 - Attachment is obsolete: true
Attachment #8784432 - Flags: review+
(Assignee)

Comment 40

7 months ago
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
(Assignee)

Comment 41

7 months ago
Created attachment 8784495 [details] [diff] [review]
Deal with folder already existing

Turned out that one call site needed the same treatment. Tests pass now.
Attachment #8784432 - Attachment is obsolete: true
Attachment #8784495 - Flags: review+
(Assignee)

Comment 42

7 months ago
https://hg.mozilla.org/comm-central/rev/4a75a6a737c7
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago7 months ago
Resolution: --- → FIXED

Comment 43

7 months ago
(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?
You need to log in before you can comment on or make changes to this bug.