Use nsIFile.exists() instead of stat to check the existence of the file

RESOLVED FIXED in Thunderbird 40.0

Status

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: hiro, Assigned: aceman)

Tracking

Trunk
Thunderbird 40.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted patch Fix (obsolete) — Splinter Review
I am not sure why the stat is there.
Attachment #655853 - Flags: review?(mbanner)
Comment on attachment 655853 [details] [diff] [review]
Fix

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

Sorry for the delay in looking at this. I think the general idea is fine, we just need some tweaks.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +1240,5 @@
>        if (m_mdbEnv)
>          m_mdbEnv->SetAutoClear(true);
> +      dbFile->GetNativePath(m_dbName);
> +      bool exists = false;
> +      dbFile->Exists(&exists);

There's a known bug on windows that Exists can cache the status of the file producing an out of date result.

So I think we either need to clone the nsIFile (which we've done in other places) or keep passing the string and create a new nsIFile for this Exists test.
Attachment #655853 - Flags: review?(mbanner)
Attachment #655853 - Flags: review-
Attachment #655853 - Flags: feedback+
(In reply to Mark Banner (:standard8) from comment #1)
> ::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
> @@ +1240,5 @@
> >        if (m_mdbEnv)
> >          m_mdbEnv->SetAutoClear(true);
> > +      dbFile->GetNativePath(m_dbName);
> > +      bool exists = false;
> > +      dbFile->Exists(&exists);
> 
> There's a known bug on windows that Exists can cache the status of the file
> producing an out of date result.
> 
> So I think we either need to clone the nsIFile (which we've done in other
> places) or keep passing the string and create a new nsIFile for this Exists
> test.
Assignee: nobody → hiikezoe
Flags: needinfo?(hiikezoe)
Posted patch Revised patch (obsolete) — Splinter Review
Create a new nsIFile instance in OpenMDB to check existence of the nsIFile as mark suggested in comment 1.
Attachment #655853 - Attachment is obsolete: true
Attachment #8374509 - Flags: review?(mbanner)
Flags: needinfo?(hiikezoe)
Try server results:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b9ff9288a76b

There are lots of oranges but not related to this patch, I think.
Comment on attachment 8374509 [details] [diff] [review]
Revised patch

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

Heading along the right lines, but I'd like a couple of additional checks added in please.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +1336,5 @@
>  
>        if (m_mdbEnv)
>          m_mdbEnv->SetAutoClear(true);
>        m_dbName = dbName;
> +      nsCOMPtr<nsIFile> dbFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);

Should be checking that this gets created.

@@ +1337,5 @@
>        if (m_mdbEnv)
>          m_mdbEnv->SetAutoClear(true);
>        m_dbName = dbName;
> +      nsCOMPtr<nsIFile> dbFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
> +      dbFile->InitWithNativePath(nsDependentCString(dbName));

dbName should probably get a null-check as otherwise this could crash.
Attachment #8374509 - Flags: review?(mbanner)
Attachment #8374509 - Flags: review-
Attachment #8374509 - Flags: feedback+
(In reply to Mark Banner (:standard8) from comment #5)
> Comment on attachment 8374509 [details] [diff] [review]
> Revised patch
> 
> Review of attachment 8374509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Heading along the right lines, but I'd like a couple of additional checks
> added in please.
> ...

Is this also a minor perf improvement?  (consider, some users have many hundreds of folder)
Status: NEW → ASSIGNED
Flags: needinfo?(hiikezoe)
kent, it seems to me like this is an important patch but hasn't gotten the recognition. Am I wrong?
And is there a better choice than you to finish it off?
Status: ASSIGNED → NEW
Flags: needinfo?(hiikezoe) → needinfo?(rkent)
I'm not much of a file system expert, but it is not clear to me that stat is a real improvement over Exists. Maybe Chiaki has some thoughts?
Flags: needinfo?(rkent) → needinfo?(ishikawa)
(In reply to Kent James (:rkent) from comment #8)
> I'm not much of a file system expert, but it is not clear to me that stat is
> a real improvement over Exists. Maybe Chiaki has some thoughts?

The patch is the other way round. Removing stat in favour of nsIFile.exists(). Which is probably more preferred option. This is the single occurence of stat() in c-c. m-c has more of them so I don't know.

Neil, what do you think?
Flags: needinfo?(neil)
(In reply to aceman from comment #9)
> (In reply to Kent James from comment #8)
> > I'm not much of a file system expert, but it is not clear to me that stat is
> > a real improvement over Exists. Maybe Chiaki has some thoughts?
> 
> The patch is the other way round. Removing stat in favour of
> nsIFile.exists(). Which is probably more preferred option. This is the
> single occurence of stat() in c-c. m-c has more of them so I don't know.
> 
> Neil, what do you think?

Exists() should be an improvement since it calls access rather than a full stat. (Even on Windows it should be better because it calls GetFileAttributesW rather than a full stat (stat is emulated by the CRT on Windows).)
Flags: needinfo?(neil)
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #7)
> kent, it seems to me like this is an important patch but hasn't gotten the
> recognition. Am I wrong?

This doesn't seem important to me, it does not change any functionality or fix a problem.

But it seems simple enough so I can take it. It seems just solving nits from comment 5 is needed here.
Assignee: hiikezoe → acelists
Status: NEW → ASSIGNED
(In reply to Kent James (:rkent) from comment #8)
> I'm not much of a file system expert, but it is not clear to me that stat is
> a real improvement over Exists. Maybe Chiaki has some thoughts?

Sorry, I missed the boat, so to speak.

I think this change has more to do with the principle of using portable library 
instead of
invoking bare system calls in the main code.
As far as I can tell, TB and FF code adheres to this very well.
Most developers won't bother even if Exists() eventually is implemented by |stat| under POSIX OS, or access.
(Seeing stat() in the main code
is like seeing unwrapped |errno| being accessed directly in
high-level main code. Sometimes I wish I could, but knowing that semantics of |errno| may not work well in Windows binary, I have to go through the portable library's error handling framework, which I feel does not seem to address error handling very well.)

I doubt if there is much performance difference between stat() and access().
They are used for different purposes to begin with.

For example, if, say, one checks the access right of a file/directory based
on the access bits returned by stat() by a series of if (bit-manipulation-expression),
then using access() in the first place is straight-forward and appropriate. 

But of course, access() doesn't give you all the detailed information which stat() returns.
If Exists() only checks for existence, I think stat() is just fine as access().
The difference of performance would probably be important only when you need to check for 10,000 files.

Re cache:
However, if |Exists| uses its own idea of caching of "known" files/folder (see Comment 1: "There's a known bug on windows that Exists can cache the status of the file producing an out of date result. So I think we either need to clone the nsIFile (which we've done in other places) or keep passing the string and create a new nsIFile for this Exists test."), 
then there *may* be performance improvement. 
However, the windows bug needs to be avoided, if it is still there.
Also, I tend to add/modify/delete files (folder as well as .msf files) during tests,
and such out-of-band file modifications may need to be handled properly
by such "cache" mechanism. At least, TB should not crash even if file is removed
while TB is not running.

But this caching issue with its own bug is orthogonal to this bug entry, and
so it is a good idea to remove the mention of "stat" in the code.

TIA
Flags: needinfo?(ishikawa)
Posted patch updated patch (obsolete) — Splinter Review
This should solve requests from standard8. Aryx, please push to try server.
Attachment #8374509 - Attachment is obsolete: true
Flags: needinfo?(archaeopteryx)
Attachment #8596718 - Attachment filename: nsfile → 786141.patch
Attachment #8596718 - Attachment is patch: true
Comment on attachment 8596718 [details] [diff] [review]
updated patch

Thanks, I do not see any new failures in that try run.
Attachment #8596718 - Flags: review?(neil)
Comment on attachment 8596718 [details] [diff] [review]
updated patch

>       m_dbName = dbName;
>-      if (stat(dbName, &st))
>+      bool exists = false;
>+      nsCOMPtr<nsIFile> dbFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &ret);
>+      if (NS_SUCCEEDED(ret) && dbFile) {
>+        ret = dbFile->InitWithNativePath(nsDependentCString(dbName));
Nit: Use m_dbName, it's already an nsCString.
Attachment #8596718 - Flags: review?(neil) → review+
Thanks.
Attachment #8596718 - Attachment is obsolete: true
Attachment #8597745 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Sorry for the incorrect "/suite part" appended to the commit message by mistake.
You need to log in before you can comment on or make changes to this bug.