1440278 - Port |Bug 1428258 - Move nsIFile::GetNativePath to a different interface (or remove it entirely)|

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Jorg K (CEST = GMT+2)]

mailnews/db/msgdb/src/nsMsgDatabase.cpp(940): error C2039: 'GetNativePath': is not a member of 'nsIFile'

and many more.

https://dxr.mozilla.org/comm-central/search?q=GetNativePath&redirect=false

Looking through
https://hg.mozilla.org/mozilla-central/rev/ffa5068067a7
https://hg.mozilla.org/mozilla-central/rev/c87bbb86f843
https://hg.mozilla.org/mozilla-central/rev/3089af2474ef
https://hg.mozilla.org/mozilla-central/rev/e3b034c3ac97
https://hg.mozilla.org/mozilla-central/rev/e28a37787c93
https://hg.mozilla.org/mozilla-central/rev/ee1367eae850
there seem to be various replacements, amongst them:
file->GetPath(path) or file->HumanReadablePath().

This won't be a quick fix since we need to inspect all call sites individually.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: c-c bustage fix

I only compiled with Win32/opt, but this patch will be a starting point.

Comment 2

[Masatoshi Kimura [:emk]]

I forgot --no-take-bug option...

Comment 3

[Honza Bambas (:mayhemer)]

Thanks!

Comment 4

[Honza Bambas (:mayhemer)]

Confirming that UTD c-c builds with this.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8953088 [details] [diff] [review]
c-c bustage fix

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

Thanks a lot, Masatoshi-san. I see heaps of changes in bug 685236 and comparatively few changes in bug 1428258.

Can you please give me some background. So basically Windows filenames move to 16bit characters which seems to have far-reaching consequences, especially in Mork.

Looking thought he patch, you have eliminated calls to GetNativePath(), as expected, but also 3 calls to InitWithNativePath(). In total we have 10 of these calls, if I counted correctly, 5 compiled for Linux/Mac leaving 2 of those calls in MapiMessage.cpp and msgMapiHook.cpp. Could you please explain the removal/non-removal of these.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ -4358,5 @@
>      if (NS_SUCCEEDED(rv) && attachFile)
>      {
>        rv = identity->GetSignature(getter_AddRefs(sigFile));
>        if (NS_SUCCEEDED(rv) && sigFile) {
> -        rv = sigFile->GetNativePath(sigNativePath);

Nit: sigNativePath is now unused.

Comment 6

[Jorg K (CEST = GMT+2)]

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

Comment 7

[Jorg K (CEST = GMT+2)]

I forgot the debug versions:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=236b68a995dccca41e0b5af35c70e33a17486ccd

Pushed with the unused variable removed to avoid compile errors.

Comment 8

[Jorg K (CEST = GMT+2)]

There is one use of .nativePath in JS:
https://dxr.mozilla.org/comm-central/rev/228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#673
mark_action("fdh", "open_message_from_file", ["file", file.nativePath]);

I suppose that will cause some Mozmill errors on Windows. That's just for logging, so we can just use .path.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1440278-JS-part.patch

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

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8953148 [details] [diff] [review]
1440278-JS-part.patch

Hmm, try push without this patch didn't show any failures. Is .nativePath still available in JS even on Windows?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1580fa402264
port bug 1428258 to mailnews: Stop using GetNativePath(). r=jorgk

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+1) from comment #5)
> Can you please give me some background. So basically Windows filenames move
> to 16bit characters which seems to have far-reaching consequences,
> especially in Mork.

I removed GetNativePath() because people keep misusing it on Windows. English speakers won't notice the misuse as long as they use only ASCII file paths.

> Looking thought he patch, you have eliminated calls to GetNativePath(), as
> expected, but also 3 calls to InitWithNativePath(). In total we have 10 of
> these calls, if I counted correctly, 5 compiled for Linux/Mac leaving 2 of
> those calls in MapiMessage.cpp and msgMapiHook.cpp. Could you please explain
> the removal/non-removal of these.

I removed the GetNativePath() -> InitWithNativePath() pattern which is the most common misuse of these functions. It will break characters thar are not in the "native" charset (ANSI code page on Windows).

MapiMessage.cpp and msgMapiHook.cpp interact with the operating system API, so char* would be encoded in the "native" charset. Ideally we should migrate them to widechar APIs.

(In reply to Jorg K (GMT+1) from comment #8)
> There is one use of .nativePath in JS:
> https://dxr.mozilla.org/comm-central/rev/
> 228f05fcbd6c04911e6e32e4df911ff51dec0fbd/mail/test/mozmill/shared-modules/
> test-folder-display-helpers.js#673
> mark_action("fdh", "open_message_from_file", ["file", file.nativePath]);
> 
> I suppose that will cause some Mozmill errors on Windows. That's just for
> logging, so we can just use .path.

This test is bogus. .nativePath is unavailable even before bug 1428258 because it is [noscript].

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #12)
> I removed the GetNativePath() -> InitWithNativePath() pattern which is the
> most common misuse of these functions. It will break characters thar are not
> in the "native" charset (ANSI code page on Windows).

By the way, I should have used `file->Clone()` instead of `new nsLocalFile(file->NativePath())` if possible. My patch is just a quick bustage fix.

Comment 14

[Jorg K (CEST = GMT+2)]

Thanks again for the patch, looking at it again, I appreciate it even more. It would have taken me days to work all this out. Using GetPersistentDescriptor() for the GetCacheElement() is a good idea, the only caller of AddCacheElement() is in the same function, I wondered why GetPersistentDescriptor() only appears once in the patch.

As far as I know, TB only uses a restricted charset for its folders, like creating a folder named テスト gives a mailbox file named bc7e9936. IMAP folders are are typically MUTF-7 encoded, so the Japanese "test" gives %26MMYwuTDI-.

For local folders that is done in NS_MsgHashIfNecessary() which uses ConvertibleToNative(). Looks like folder names can have ANSI file names on Windows (like äöü), but the attempt to create a folder with a Japanese string creates a hashed name.

Why did you switch MORK_FILEOPEN(file, how) to _wfopen(char16ptr_t(file), NS_ConvertASCIItoUTF16(how).get())? It only appears to be used in morkFile.cpp with a char* argument. The same question applies to all the other Mork changes. Am I missing something?

(In reply to Masatoshi Kimura [:emk] from comment #13)
> By the way, I should have used `file->Clone()` instead of `new
> nsLocalFile(file->NativePath())` if possible. 
Unless I missed something, I didn't see that pattern in the patch.

I'll land the fix for the bogus test now, maybe it will start to do something more useful.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c6418eab10bc
port bug 1428258: Stop using nativePath in JS. r=aceman

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #14)
> The same question applies to all the
> other Mork changes. Am I missing something?
Looks like it all trickles down from morkFactory::CreateNewFile() and morkFactory::OpenOldFile(), but it's too late here now (past 2 AM) to look into it further.

Comment 17

[Jorg K (CEST = GMT+2)]

Sorry, please ignore the questions, too late here now, I can see the
PathChar* name = mFile_Name;
...
file = MORK_FILEOPEN(name, "wb+");

Sorry again.

Comment 18

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+1) from comment #14)
> As far as I know, TB only uses a restricted charset for its folders, like
> creating a folder named テスト gives a mailbox file named bc7e9936. IMAP
> folders are are typically MUTF-7 encoded, so the Japanese "test" gives
> %26MMYwuTDI-.

Thanks. I had no time to check if mailbox:// can contain non-ASCII characters.

> (In reply to Masatoshi Kimura [:emk] from comment #13)
> > By the way, I should have used `file->Clone()` instead of `new
> > nsLocalFile(file->NativePath())` if possible. 
> Unless I missed something, I didn't see that pattern in the patch.
> 
> I'll land the fix for the bogus test now, maybe it will start to do
> something more useful.

Maybe I over-simplified the example. I intended to say something like:
```
PathString path = file->NativePath();
someFunction(path);

nsresult someFunction(const PathString& path)
{
  RefPtr<nsLocalFile> file = new nsLocalFile(path);
}
```
For example, OpenMDB has this pattern if I read the code correctly.

It should be:
```
someFunction(file);

nsresult someFunction(nsIFile* aFile)
{
  nsCOMPtr<nsIFile> file;
  nsresult rv = aFile->Clone(getter_AddRefs(file));
  NS_ENSURE_SUCCESS(rv, rv);
}
```

But such changes will require more careful inspection, so I took a more mechanical way.

Comment 19

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+1) from comment #14) 
> For local folders that is done in NS_MsgHashIfNecessary() which uses
> ConvertibleToNative(). Looks like folder names can have ANSI file names on
> Windows (like äöü), but the attempt to create a folder with a Japanese
> string creates a hashed name.

It depends on the current system locale. On Japanese Windows whose system local is code page 932, "äöü" will be hashed but Japanese characters will not. In any case, nsMsgProtocol::GetFileFromURL will handle UTF-8 paths correctly.

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to Masatoshi Kimura [:emk] from comment #18)
> Thanks. I had no time to check if mailbox:// can contain non-ASCII
> characters.
Do you think it makes sense to revert the Mork changes? Last night/early morning I tried to compile with only the Mork changes backed out and ran into problems in the morkFactory::CreateNewFile() and morkFactory::OpenOldFile() calls, since the parameters passed to them in were no longer char*. With some #ifdef-s at those call sites or higher up (in the three OpenMDB() functions) one could squeeze the Windows paths back into 8bit characters with a comment to that effect. But maybe/most likely that's not a good idea.

Comment 21

[Masatoshi Kimura [:emk]]

(In reply to Jorg K (GMT+1) from comment #20)
> (In reply to Masatoshi Kimura [:emk] from comment #18)
> > Thanks. I had no time to check if mailbox:// can contain non-ASCII
> > characters.
> Do you think it makes sense to revert the Mork changes? Last night/early
> morning I tried to compile with only the Mork changes backed out and ran
> into problems in the morkFactory::CreateNewFile() and
> morkFactory::OpenOldFile() calls, since the parameters passed to them in
> were no longer char*. With some #ifdef-s at those call sites or higher up
> (in the three OpenMDB() functions) one could squeeze the Windows paths back
> into 8bit characters with a comment to that effect. But maybe/most likely
> that's not a good idea.

Why do you want to revert Mork changes? We can't use 8-bit functions on Windows because the profile path can contain any character.

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to Masatoshi Kimura [:emk] from comment #21)
> Why do you want to revert Mork changes? We can't use 8-bit functions on
> Windows because the profile path can contain any character.
Right. I was contemplating it for a moment since we usually don't touch Mork. I didn't consider the profile path, sorry, thinking too Euro-centric ;-) Thanks again for the patch!

When I find some time, I'll attach a patch for what you described at the end of comment #18.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1440278-OpenMDB.patch (v1)

(In reply to Jorg K (GMT+1) from comment #22)
> When I find some time, I'll attach a patch for what you described at the end
> of comment #18.
If I understood your comment correctly, the approach was instead of passing a character path a function, passing nsIFile*.

As candidates you mentioned the OpenMDB() functions of which we have three:
nsAddrDatabase::OpenMDB() already takes an nsIFile*.

nsMsgFolderCache::OpenMDB() takes a path, but the first thing it does is to call mdbFactory->OpenOldFile() with that path:
https://dxr.mozilla.org/comm-central/rev/c6418eab10bc0b2bf89e11427786a7b5cb0b4bb2/mailnews/base/src/nsMsgFolderCache.cpp#151
So since we need the path anyway, I don't see this as a candidate.

That leaves nsMsgDatabase::OpenMDB() which is covered by the enclosed follow-up patch.

Comment 24

[Masatoshi Kimura [:emk]]

Comment on attachment 8953595 [details] [diff] [review]
1440278-OpenMDB.patch (v1)

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

::: mailnews/db/msgdb/public/nsMsgDatabase.h
@@ -164,5 @@
>                                 nsIDBChangeListener *instigator);
>    nsresult OpenInternal(nsMsgDBService *aDBService, nsIFile *aFolderName,
>                          bool aCreate, bool aLeaveInvalidDB, bool sync);
>    nsresult CheckForErrors(nsresult err, bool sync, nsMsgDBService *aDBService, nsIFile *summaryFile);
> -  virtual nsresult OpenMDB(const PathChar *dbName, bool create, bool sync);

Please remove `using PathChar` because it is no longer used in this class.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +1319,5 @@
>        nsIMdbHeap* dbHeap = nullptr;
>  
>        if (m_mdbEnv)
>          m_mdbEnv->SetAutoClear(true);
> +      m_dbName = dbFile->NativePath();

Can you change the m_dbName type to nsCOMPtr<nsIFile>? I found some logging code using m_dbName.get() to get a filename to log. It should be changed to m_dbName->HumanReadablePath().get(). Otherwise logging will not output the filename correctly on Windows.

(I'm surprised builders did not catch this error. Is c-c disabling warnings-as-errors?)

Comment 25

[:aceman]

(In reply to Masatoshi Kimura [:emk] from comment #24)
> (I'm surprised builders did not catch this error. Is c-c disabling
> warnings-as-errors?)

warnings-as-errors is being forced on c-c from m-c (even though m-c has tons of warnings) so whole c-c is warning free now. Unless there is an exception for mork directory, but otherwise we had to fix warnings in many of c-c directories.

Comment 26

[Masatoshi Kimura [:emk]]

Ah,
>  MOZ_LOG(DBLog, LogLevel::Info, ("closing database    %s\n", m_dbName.get()));
did not warn (and break c-c builders) because MOZ_FORMAT_PRINTF has no effect on MSVC and m_dbName uses 8-bit character on non-Windows. On m-c, it would have broken MinGW builds.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1440278-OpenMDB.patch (v2)

Here comes v2 which addresses the review comments. Remembering the nsIFile* instead of the native name causes some more reshuffling.

Comment 28

[Masatoshi Kimura [:emk]]

Comment on attachment 8953721 [details] [diff] [review]
1440278-OpenMDB.patch (v2)

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

r=me with the following points fixed.

Please revert `#include "nsLocalFile.h"` back to `#include "nsIFile.h"` at the top of nsMsgDatabase.cpp.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +82,5 @@
>    for (uint32_t i = 0; i < m_dbCache.Length(); i++)
>    {
>      nsMsgDatabase* pMessageDB = m_dbCache.ElementAt(i);
>      if (pMessageDB)
> +      printf("db left open %s\n", (const char *) pMessageDB->m_dbFile->HumanReadablePath().get());

Please remove the redundant cast. This cast masked the warning. MSVC2015+ would have warned if this cast is not present.

@@ +612,5 @@
>    if (m_headersInUse && m_headersInUse->EntryCount() > 0)
>    {
>      NS_ASSERTION(false, "leaking headers");
> +    printf("leaking %d headers in %s\n", m_headersInUse->EntryCount(),
> +           (const char *) m_dbFile->HumanReadablePath().get());

Ditto.

@@ +1318,5 @@
>  
>        if (m_mdbEnv)
>          m_mdbEnv->SetAutoClear(true);
> +      PathString dbName = dbFile->NativePath();
> +      m_dbFile = dbFile;

ret = dbFile->Clone(getter_AddRefs(m_dbFile));
Callers would not expect that the parameter is modified.

Comment 29

[Masatoshi Kimura [:emk]]

Comment on attachment 8953721 [details] [diff] [review]
1440278-OpenMDB.patch (v2)

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

Ah, are you sure that m_dbFile will never be null when dereferenced?

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 1440278-OpenMDB.patch (v3)

I assume we want to check the status after the clone.

> Ah, are you sure that m_dbFile will never be null when dereferenced?
I'll look into that.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1440278-OpenMDB.patch (v4)

Sorry to trouble you with another review. m_dbFile is dereferenced 8 times, 4 of which with ->HumanReadablePath(). Those 4 dereferenced m_dbName before and to my knowledge didn't crash. m_dbName was set at the same spot in the code where m_dbFile is now set.

The remaining four call sites are now protected.

Comment 32

[Masatoshi Kimura [:emk]]

Comment on attachment 8953743 [details] [diff] [review]
1440278-OpenMDB.patch (v4)

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

LGTM

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b11713425a9c
Follow-up: Change nsMsgDatabase::OpenMDB() to take an nsIFile* to avoid unnecessary file operations. r=emk CLOSED TREE