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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

We have plenty of missing checks for nsIFile operations. Using [must_use] will find them.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Use [must_use] on nsIFile.{create,createUnique}

And fix numerous missing checks that this change identifies.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use [must_use] on nsIFile.open{NSPR,ANSI}FileDesc()

The patch also changes RemoteOpenFileChild::OpenNSPRFileDesc() so that it
cannot succeed with a null fd, so that checking just the return value is
sufficient.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use [must_use] on more nsIFile things

These ones don't require any extra checks to be added.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8782283 [details] [diff] [review]
(part 2) - Use [must_use] on nsIFile.open{NSPR,ANSI}FileDesc()

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

We really should make these sorts of methods result a wrapped PRFileDesc* pointer ala Rust that you have to check prior to unwrapping, rather than futzing around with nsresult.

Comment 5

[Nicholas Nethercote [inactive]]

> We really should make these sorts of methods result a wrapped PRFileDesc*
> pointer ala Rust that you have to check prior to unwrapping, rather than
> futzing around with nsresult.

That would be nice... but isn't it prevented by the fact that modifying xptcall is almost impossible?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> > We really should make these sorts of methods result a wrapped PRFileDesc*
> > pointer ala Rust that you have to check prior to unwrapping, rather than
> > futzing around with nsresult.
> 
> That would be nice... but isn't it prevented by the fact that modifying
> xptcall is almost impossible?

This is completely true!  Fortunately, these particular methods are [noscript], so we could at least try doing something intelligent with them.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8782258 [details] [diff] [review]
(part 1) - Use [must_use] on nsIFile.{create,createUnique}

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

jduell: the changes to nsDiskCacheDeviceSQL.cpp in this patch caused test failures, so I reverted the |rv| checks and just used |Unused| to explicitly ignore the results of the two Create() calls. Do you understand returning early when either of these calls fails would cause these failures?

In the failing test that I looked at, generation==-1 was false in the cases when the Create() calls failed, and the file->AppendNative(nsDependentCString(leaf)) call in the |else| branch succeeded.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to Nicholas Nethercote [:njn] from comment #5)
> > > We really should make these sorts of methods result a wrapped PRFileDesc*
> > > pointer ala Rust that you have to check prior to unwrapping, rather than
> > > futzing around with nsresult.
> > 
> > That would be nice... but isn't it prevented by the fact that modifying
> > xptcall is almost impossible?
> 
> This is completely true!  Fortunately, these particular methods are
> [noscript], so we could at least try doing something intelligent with them.

we have noscript methods that return already_AddRefed<> so Maybe<PRFileDesc> should work too.  iirc the trick is |native MaybeFileDesc(Maybe<PRFileDesc>);|

Comment 9

[Nicholas Nethercote [inactive]]

> jduell: the changes to nsDiskCacheDeviceSQL.cpp in this patch caused test
> failures, so I reverted the |rv| checks and just used |Unused| to explicitly
> ignore the results of the two Create() calls. Do you understand returning
> early when either of these calls fails would cause these failures?

Ah, the failing rv values were 80520008, which is NS_ERROR_FILE_ALREADY_EXISTS.

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51410d553f6f0c1c260f3e63012b024484dfe67a
Bug 1296164 (part 1) - Use [must_use] on nsIFile.{create,createUnique}. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/193c7e5b2ee4355f70f1d1bf10b953bd5cbdd8c8
Bug 1296164 (part 2) - Use [must_use] on nsIFile.open{NSPR,ANSI}FileDesc(). r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed27be6470e5690889079fc0bf5c0ed9626f9eb3
Bug 1296164 (part 3) - Use [must_use] on more nsIFile things. r=froydnj.

Comment 11

[Nicholas Nethercote [inactive]]

I'm marking this leave-open because there is more to be done.

Comment 12

[Jason Duell]

Comment on attachment 8782258 [details] [diff] [review]
(part 1) - Use [must_use] on nsIFile.{create,createUnique}

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

Honza is our expert on the appcache, so he's the right person for the nsDiskCacheDeviceSQL.cpp question (Honza, see comment 7)

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Comment on attachment 8782258 [details] [diff] [review]
> (part 1) - Use [must_use] on nsIFile.{create,createUnique}
> 
> Review of attachment 8782258 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> jduell: the changes to nsDiskCacheDeviceSQL.cpp in this patch caused test
> failures, so I reverted the |rv| checks and just used |Unused| to explicitly
> ignore the results of the two Create() calls. Do you understand returning
> early when either of these calls fails would cause these failures?
> 
> In the failing test that I looked at, generation==-1 was false in the cases
> when the Create() calls failed, and the
> file->AppendNative(nsDependentCString(leaf)) call in the |else| branch
> succeeded.

This is an "ensure directory exists" code.  It probably already exists.  I think you can freely ignore the failures here, if there is anything wrong with the storage I believe we fail later when creating the leaf cache file gracefully.

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8782258 [details] [diff] [review]
(part 1) - Use [must_use] on nsIFile.{create,createUnique}

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

::: netwerk/cache/nsDiskCacheDeviceSQL.cpp
@@ +384,5 @@
>                                const nsCString *fullKey,
>                                int generation)
>  {
> +  nsresult rv;
> +

I like this! ;)

@@ +407,5 @@
>  
>    file->AppendNative(nsPrintfCString("%X", dir1));
> +  rv = file->Create(nsIFile::DIRECTORY_TYPE, 00700);
> +  if (NS_FAILED(rv))
> +    return nullptr;

ideally we should do something like "if not exists -> Create()" but I don't think there is a need for additional IO.  this code is deprecated, hence, do minimal changes to it.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51410d553f6f
https://hg.mozilla.org/mozilla-central/rev/193c7e5b2ee4
https://hg.mozilla.org/mozilla-central/rev/ed27be6470e5

Comment 16

[Jorg K (CEST = GMT+2)]

Why do you ignore the return value here?
https://hg.mozilla.org/mozilla-central/rev/ed27be6470e5#l1.29
Further down persistentDescriptor is passed into AddDownloadToDB().
Never fails under the given circumstances?

GetPersistentDescriptor() doesn't seem to fail in general, but there are call sites in M-C which check the return value.

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.

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #16)
> Why do you ignore the return value here?
> https://hg.mozilla.org/mozilla-central/rev/ed27be6470e5#l1.29
> Further down persistentDescriptor is passed into AddDownloadToDB().
> Never fails under the given circumstances?

There was already a cast to void. That's not enough to satisfy GCC when a MOZ_MUST_USE annotation is present, so I changed it to |Unused <<|. I assumed that the existing cast to void was legitimate.

> GetPersistentDescriptor() doesn't seem to fail in general, but there are
> call sites in M-C which check the return value.
> 
> 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.

The RemoteOpenFileChild::GetPersistentDescriptor() implementation can fail. I don't know how often that implementation is used, so I erred on the side of conservatism, i.e. over-checking. If you know this is wrong, please file a follow-up bug!

Comment 18

[Nicholas Nethercote [inactive]]

I will continue this in bug 1297950.