1626514 - Ensure there won't end up with many "\\?\" in everywhere

Status: NEW

(Core :: XPCOM, enhancement, P3)

Description

[Tom Tung [:tt, :ttung]]

It should be put in InitWithPath. Ideally, it should look like:

\\ InitWithPath
MOZ_ASSERT_IF(!mUseDOSDevicePathSyntax, StringBeginsWith(mWorkingPath, kDevicePathSpecifier));

We couldn't do that now because the current consumer (QM_NewLocalFile) initializes the file and set useDOSDevicePathSyntax. If we can somehow change this, we can add the assertion.

Comment 1

[Jan Varga [:janv]]

Are you worried that InitWithPath can't catch it because QM_NewLocalFile sets the attribute after calling InitWithPath ?

SetUseDOSDevicePathSyntax can also do something to avoid prefixing an already prefixed path.
Basically, all the places in nsLocalFileWin.cpp that add the prefix should handle the case when the prefix was already added by the consumer. There are only 2 places like that right now, InitWithPath and SetUseDOSDevicePathSyntax.

Comment 2

[Tom Tung [:tt, :ttung]]

I talked this with Jan via private message on Slack yesterday.

So, the idea of this bug is to ensure that there won't have a caller in a module and it prepends the prefix (\\?\) by itself rather than using useDOSDevicePathSytax attribute.

I had tried to add a debug assertion in InitWithPath like:

 MOZ_ASSERT(!StringBeginsWith(mWorkingPath, kDevicePathSpecifier));

, but it had caused test failures on a try push.

The issue which causes this not easy to solve is that QM_NewLocalFile currently calls InitWithPath and then SetUseDOSDevicePathSyntax by querying interface.

Also, for some cases, QuotaMananger or its client reuse file paths from another nsIFile that was created by QM_NewLocalFile. That means we would init a file with a file path that has already prepended the prefix.

Here is an example where QuotaManager reuses file path from others.

To avoid this issue and to ensure that there won't have many callers prepend the prefix by themselves, a quick solution is to have another NS_NewLocalFile (e.g. NS_NewLocalFileWin(const nsAString& aPath, bool aFollowLinks, bool, aUseDOSDevicePathSyntax, nsILocalFileWin** aResult)) or something similar to differentiate the code path, So that QM_NewLocalFile won't have to query the interface and thus have to InitWithPath then SetUseDOSDevicePathSyntax.

Therefore, we can put a debug assertion to ensure that the caller for InitWithPath won't prepend the prefix by themselves unless they really want to do that.

There was a similar discussion that is related to this:
Starts from
https://bugzilla.mozilla.org/show_bug.cgi?id=1536796#c97
To
https://bugzilla.mozilla.org/show_bug.cgi?id=1536796#c101

I will check if the plan is the best way that I can think of. If it is, I will ask Nathan's feedback on that.

Comment 3

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #2)

I will check if the plan is the best way that I can think of. If it is, I will ask Nathan's feedback on that

The gut feeling is to always check if the mDOSDevicePathSyntax is true when mWorkingPath starts with the \\?\ prefix because InitWithPath is the only place where we can set a path for the file.

I remember that Nathan didn't like the idea of adding a variable (aUseDOSDevicePathSyntax) into NS_NewLocalFile since only Window needs to check this variable.

If we introduce NS_NewLocalFileWin, we can avoid that issue?
The #ifdef XP_WIN #endif stuff is needed because we need to query interface to set the attribute anyway. And the advantage is that we can have an assertion to ensure all nsIFile's call sites over the modules use useDOSDevicePathSyntax instead of prepending the prefix by their own self.
This would look like:

// aPath
nsresult rv;
#ifdef XP_WIN
  rv = NS_NewLocalFileWin(aPath, /* aFollowLinks */ false, true, getter_AddRefs(file));
#else
  rv = NS_NewLocalFile(aPath, /* aFollowLinks */ false, getter_AddRefs(file));
#endif

The alternative option is to change current \\?\ call sites to:

// aPath
nsCOMPtr<nsIFile> file(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv));
// check rv

#ifdef XP_WIN
  nsCOMPtr<nsILocalFileWin> winFile = do_QueryInterface(file, &rv);
  // check rv 

  winFile->SetUseDOSDevicePathSyntax(true);
#endif

file->InitWithPath(aPath);

Compare to NS_NewLocalFile, there much less call sites using this pattern (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv); and InitWithPath). So I guess NS_NewLocalFile is suggested to be used?

[Side note] Places which might need to change if we add the assertion in InitWithPath:

• FileCreatorParent::CreateBlobImpl which is called from File.createFromNsIFile or File.createFromFileName
• The call sites that call [InitWithPath] (e.g. [1], [2], the callsite from sqlite)
• xpcom/tests/gtest/TestFile.cpp

Comment 4

[Jan Varga [:janv]]

I prefer NS_NewLocalFileWin, because it avoids extra hash lookups nad QIs.

Not sure about "Side notes", we will see once you have a patch for NS_NewLocalFileWin.

Anyway, it seems that any place in dom/quota, dom/indexedDB, dom/cache and dom/localstorage which uses do_CreateInstance(NS_LOCAL_FILE_CONTRACTID) should be converted to QM_NewLocalFile.
It seems there's only one place in dom/quota/ActorsParent.cpp, and I'm surprised that we didn't catch it sooner.
I'm actually finishing work on bug 1624802.

Comment 5

[Tom Tung [:tt, :ttung]]

Dropping myself from the assignee since I won't work on this in the short term.