Closed Bug 685236 Opened 13 years ago Closed 6 years ago

Move nsIFile::GetNativePath to a different interface (or remove it entirely)

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: joe, Assigned: emk)

References

Details

(Keywords: addon-compat)

Attachments

(9 files, 22 obsolete files)

59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
jimm
: review+
Details
59 bytes, text/x-review-board-request
keeler
: review+
Details
59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
jwatt
: review+
Details
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
nsIFile::GetNativePath is a footgun, because it returns a char* and that breaks on Unicode paths.

We should move it to a new interface (nsIFileTotallyUnsafeYouShouldntUseIt) or remove it entirely if it's possible.
We have tons of uses of this method in our tree :(

On Windows, basically anything which doesn't use _wopen/CreateFileW to open files is going to be unsuccessful on files with Unicode characters in their path names.  In other words, the native path name representation of files on Windows should be a wide char string.  Which means that the existing GetNativePath abstraction is completely broken.  I think we should just change all of the call sites of this method in our tree, and remove it completely, as it leads to broken code being written, no matter what interface it's defined on.

Does that make sense?
Note that GetNativePath can be correct for the non-windows half of ifdefed code. But I think it does make sense to hide it somewhere much less accessible. I have a nsIFile refactoring in mind, maybe this should wait to roll up as part of that (get rid of nsIFile, or maybe file interfaces altogether in favor of a much simpler API in both C++ and JS, bug 503963).
Fixing bug 960964 and bug 960957 would make the "native" part of nsIFile less dangerous until we get around to getting rid of the "native" part of nsIFile.
See Also: → 960964
Depends on: 1420427
Depends on: 1421123
Depends on: 1422856
Depends on: 1425219
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment on attachment 8939822 [details]
Bug 685236 - Workaround Skia file path issue.

https://reviewboard.mozilla.org/r/210146/#review215786
Attachment #8939822 - Flags: review?(jwatt) → review+
Attachment #8939809 - Flags: review?(honzab.moz)
Comment on attachment 8939813 [details]
Bug 685236 - Stop using GetNativePath in ffvpx.

I get a 500 error in reviewboard so doing so here:

there's no commit message.

Additionally, the windows replacement for PR_GetLibraryName shouldn't be found in this file, it's totally out of scope.

Please, create a generic PR_GetLibraryName that will work on windows as it should rather than having multiple implementations wherever it's used
Attachment #8939813 - Flags: review?(jyavenard) → review-
(In reply to Jean-Yves Avenard [:jya] from comment #50)
> Comment on attachment 8939813 [details]
> Bug 685236 - Stop using GetNativePath in ffvpx.
> 
> I get a 500 error in reviewboard so doing so here:

Yeah, reviewboard is broken in this bug for some reason...
> #ifdef __MINGW32__
>  int fd = _wopen(reinterpret_cast<const wchar_t*>(aFilename),
>                  _O_WRONLY | _O_BINARY | _O_CREAT | _O_TRUNC);
>  mFileBuf.reset(new __gnu_cxx::stdio_filebuf<char>(fd, std::ios::out |
>                                                        std::ios::binary |
>                                                        std::ios::trunc));
>  mOutputStream.rdbuf(mFileBuf.get());
> #elif defined(XP_WIN)
>  mOutputStream.open(reinterpret_cast<const wchar_t*>(aFilename), ofstream::binary);

Can you explain what's going on there and why we need to special case mingw?
Flags: needinfo?(joe)
Attachment #8939807 - Flags: review?(mcmanus) → review?(honzab.moz)
Attachment #8939829 - Flags: review?(mcmanus) → review?(honzab.moz)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #52)
> > #ifdef __MINGW32__
> >  int fd = _wopen(reinterpret_cast<const wchar_t*>(aFilename),
> >                  _O_WRONLY | _O_BINARY | _O_CREAT | _O_TRUNC);
> >  mFileBuf.reset(new __gnu_cxx::stdio_filebuf<char>(fd, std::ios::out |
> >                                                        std::ios::binary |
> >                                                        std::ios::trunc));
> >  mOutputStream.rdbuf(mFileBuf.get());
> > #elif defined(XP_WIN)
> >  mOutputStream.open(reinterpret_cast<const wchar_t*>(aFilename), ofstream::binary);
> 
> Can you explain what's going on there and why we need to special case mingw?

MSVC has wchar_t overloads of std::ofstream constructor and std::ofstream::open, but they were MSVC proprietary extension until C++17. So MinGW does not support them. So I'm using a GCC extension to deal with the lack of wchar_t overloads.
Comment on attachment 8939817 [details]
Bug 685236 - Stop using GetNativePath in Telemetry.cpp.

https://reviewboard.mozilla.org/r/210136/#review215874

I see multiple platform-specific code-paths in this patch.
Telemetry.cpp is probably not the only place that uses this kind of platform specific file access?
Can't we abstract away the platform-specific code in some other module?
Comment 50 seems to ask for this too.
Attachment #8939817 - Flags: review?(gfritzsche)
Yeah, we shouldn't be open coding all of the handling of mingw/windows. For std::[io]streams it probably makes sense to use a wrapper class that acts something like: http://www.boost.org/doc/libs/1_66_0/boost/filesystem/fstream.hpp
Attachment #8939810 - Flags: review?(l10n) → review+
Attachment #8939832 - Flags: review?(l10n) → review+
Comment on attachment 8939810 [details]
Bug 685236 - Stop using GetNativePath in rdf/.

Note, we're removing the RDF file datasource in bug 1412978.
Comment on attachment 8939821 [details]
Bug 685236 - Workaround for WebRTC.

https://reviewboard.mozilla.org/r/210144/#review215886
Attachment #8939821 - Flags: review?(rjesup) → review+
Comment on attachment 8939815 [details]
Bug 685236 - Stop using GetNativePath in dom/xul.

https://reviewboard.mozilla.org/r/210132/#review215908
Attachment #8939815 - Flags: review?(bzbarsky) → review+
Comment on attachment 8939811 [details]
Bug 685236 - Stop using GetNativePath in uriloader/exthandler.

r=me, but this doesn't really help get rid of the "native path" concept...

(mozreview is being broken with 500 errors, hence the direct review here).
Attachment #8939811 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #59)
> Comment on attachment 8939811 [details]
> Bug 685236 - Stop using GetNativePath in uriloader/exthandler.
> 
> r=me, but this doesn't really help get rid of the "native path" concept...
> 
> (mozreview is being broken with 500 errors, hence the direct review here).

NativePath() function returns paths in native wide encoding while GetNativePath() returned paths in native narrow encoding. I tried to remove the latter because it is lossy on Windows. I admit the name is a bit confusing.
(In reply to Masatoshi Kimura [:emk] from comment #60)
> NativePath() function returns paths in native wide encoding

More precisely, NativePath() returns in native encoding that is used in C++17 std::filesystem::path. It is native wide encoding on Windows. It is still native narrow encoding on other (POSIX-based) platforms. I introduced mozilla::filesystem::Path for type aliases of the native encoding.

Strictly speaking, the Windows native wide encoding is NOT UTF-16. Windows file paths allows lone surrogates. But we have never cared about the difference. So mozilla::filesystem::Path is char16_t on Windows.
Comment on attachment 8939812 [details]
Bug 685236 - Stop using GetNativePath in gfx/.

Tom, could you review the MinGW specific part?
Attachment #8939812 - Flags: review?(tom)
Blocks: 1428258
I cloned this bug because I could not even update patches on MozReview in this bug.

I'll move not-yet-r+'ed patches to the new bug.
Comment on attachment 8939802 [details]
Bug 685236 - Make char16ptr_t::operator bool() explicit.

It looks like some of these patches got duplicated, so I'm going to close review requests on the patches that were posted earlier and mark them as obsolete.  Please correct me if I have misunderstood the situation.
Attachment #8939802 - Attachment is obsolete: true
Attachment #8939802 - Flags: review?(nfroyd)
Attachment #8939803 - Attachment is obsolete: true
Attachment #8939803 - Flags: review?(nfroyd)
Attachment #8939804 - Attachment is obsolete: true
Attachment #8939804 - Flags: review?(nfroyd)
Attachment #8939805 - Attachment is obsolete: true
Attachment #8939805 - Flags: review?(nfroyd)
Attachment #8939806 - Attachment is obsolete: true
Attachment #8939806 - Flags: review?(nfroyd)
Attachment #8939823 - Attachment is obsolete: true
Attachment #8939823 - Flags: review?(nfroyd)
Comment on attachment 8939809 [details]
Bug 685236 - Stop using GetNativePath in libjar.

HTTP 500 INTERNAL SERVER ERROR when publishing r:

This looks good, but I want to make sure the comments are really fixed and not lost among such huge number of patches.  Otherwise this would be "r+ with comments fixed".

thanks!

Two comments:

> if (rv == NS_OK || !outFile)
>    return rv;

if (...) { ... } please, when you are here

> #ifdef XP_WIN
>     DeleteFileW(outFile->NativePath().get());
> #else
>     PR_Delete(outFile->NativePath().get());
> #endif
> 

I think deleting |outname| always was a huge and probably even potentially security sensitive bug!

I don't understand why it's even passed in here, since it always has been a const c-string held by nsAutoCString.  That must never be deleted directly!

This weird code has been introduced in bug 214672 [1][2], probably slipped among those twenty three versions of that huge patch...

hence, please remove passing the path down as an arg to ExtractFile altogether.

[1] https://github.com/ehsan/mozilla-cvs-history/commit/cd30ab01fab40b7b0e1d8b2f7be26e0e4b0b3ff1#diff-88f33d64ce7bd4331488acacbec5cbf5R658
[2] https://github.com/ehsan/mozilla-cvs-history/commit/cd30ab01fab40b7b0e1d8b2f7be26e0e4b0b3ff1#diff-2dbe4b5e3e160b25d1645300d75515a4R255
Attachment #8939809 - Flags: review?(honzab.moz) → review-
Comment on attachment 8939824 [details]
Bug 685236 - Make char16ptr_t::operator bool() explicit.

I think this makes sense regardless of what happens elsewhere in this bug.  Perhaps it should be landed in its own bug?
Attachment #8939824 - Flags: review?(nfroyd) → review+
Attachment #8939807 - Flags: review?(honzab.moz) → review+
Comment on attachment 8939825 [details]
Bug 685236 - Add mozilla::filesystem::Path and use it in nsIFile.

This is going to be a little haphazard, because mozreview is apparently out of commission on this bug.

We tend to discourage namespaces underneath mozilla::, and the usual mozilla::filesystem::Path::value_type feels like a mouthful.  Do you think it'd be reasonable to just have mozilla::Path, or were you trying to save that name for a more fully-featured Path class?

WDYT about calling Path PathTraits or similar, since there's no actual data storage in this class?

I realize Path::value_type is carried over here from nsTString.  But I don't know that it's gaining us anything having it here as a public API, or that it's a reasonable name for a public API in this case.

WDYT about making value_type private to Path, and then requiring people to use Path::string_type::value_type if they need to?  I realize that's more typing, but I don't expect it to be used very much, and I think it's slightly more obvious what's being referenced in that case.

Referencing nsTString and similar from MFBT is not really allowed; you're depending on users including some string header prior to this, which is not the typical way we structure headers in Gecko.  Why are we putting this in MFBT rather than in XPCOM?
Attachment #8939825 - Flags: review?(nfroyd)
Comment on attachment 8939827 [details]
Bug 685236 - Implement FileDescriptorFile::GetPersistentDescriptor.

It looks like this is primarily used to replace GetNativePath usages in subsequent patches.

1) Why is this a reasonable transformation?  (Are we sure that a "persistent descriptor" is really what we want?)
2) Why are we not simply using GetPersistentDescriptor everywhere that we were using GetNativePath?

The code of the patch is obviously OK, but I am unsure of the semantics that we're trying to implement here...
Attachment #8939827 - Flags: review?(nfroyd)
Comment on attachment 8939826 [details]
Bug 685236 - Add nsIFile::DisplayPath.

`DisplayPath` reads ambiguously to me: is the method displaying a path, or is it returning a path for display?  Maybe PathForHumanConsumption? :)  (Not a great name, better suggestions welcome!)

I am a little concerned that we are adding more API surface to nsIFile.  Would it not be better to add a standalone function here that we implement differently for Windows and Unix?
Attachment #8939826 - Flags: review?(nfroyd)
Comment on attachment 8939828 [details]
Bug 685236 - Stop using GetNativePath in xpcom/.

Is it possible that we could use nsIFile itself to help cut down on #ifdefs in the LateWriteObserver::Observe code?  Everything else seems mostly reasonable, subject to whatever API changes we decide on with previous patches.
Attachment #8939828 - Flags: review?(nfroyd)
Comment on attachment 8939812 [details]
Bug 685236 - Stop using GetNativePath in gfx/.

I'm r+-ing it, it seemed okay to me but I wasn't very confident. I asked Jacek to look at it and he said looked correct to him, so this is really a proxy r+ from him.
Attachment #8939812 - Flags: review?(tom) → review+
Comment on attachment 8939820 [details]
Bug 685236 - Stop using GetNativePath in PSM.

https://reviewboard.mozilla.org/r/210142/#review216398
Attachment #8939820 - Flags: review?(dkeeler) → review+
Depends on: 1428541
(In reply to Nathan Froyd [:froydnj] from comment #66)
> Comment on attachment 8939824 [details]
> Bug 685236 - Make char16ptr_t::operator bool() explicit.
> 
> I think this makes sense regardless of what happens elsewhere in this bug. 
> Perhaps it should be landed in its own bug?

Filed bug 1428541.
Comment on attachment 8939824 [details]
Bug 685236 - Make char16ptr_t::operator bool() explicit.

Moved to bug 1428541.
Attachment #8939824 - Attachment is obsolete: true
I'll move discussions about comment #67 to #69 to another bug because they will affect all subsequent patches.
Depends on: 1428543
Depends on: 1428557
Depends on: 1428568
Depends on: 1428614
Mozreview isn't up this morning.

 auto filename = Substring(path, path.RFindChar('\\') + 1);
  if (filename.Length() < 7) {
    return false;

Seems odd to have a method IsPluginFile that returns false if the passed in string is 7 chars or less.
Comment on attachment 8939816 [details]
Bug 685236 - Stop using GetNativePath in widget/windows.

https://reviewboard.mozilla.org/r/210134/#review216716
Attachment #8939816 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #76)
> Mozreview isn't up this morning.

Yeah, Mozreview is broken in this bug. I'll attach an update patch in bug 1428258.

>  auto filename = Substring(path, path.RFindChar('\\') + 1);
>   if (filename.Length() < 7) {
>     return false;
> 
> Seems odd to have a method IsPluginFile that returns false if the passed in
> string is 7 chars or less.

I guess this function requires at least one character between "np" and ".dll".
I'll update a comment explaining about 7.
Flags: needinfo?(joe)
Comment on attachment 8939823 [details]
Bug 685236 - Disallow nsIFile::GetNativeTarget on Windows.

https://reviewboard.mozilla.org/r/210148/#review217018
Comment on attachment 8939829 [details]
Bug 685236 - Stop using GetNativePath in netwerk/.

rb rb...

- please always use get() with DisplayPath()
- please when you modify an if() block, make sure the body is embraced with { }
Attachment #8939829 - Flags: review?(honzab.moz) → review+
Comment on attachment 8939812 [details]
Bug 685236 - Stop using GetNativePath in gfx/.

r- for now based on my earlier comments.
Attachment #8939812 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8939803 [details]
Bug 685236 - Add mozilla::filesystem::Path and use it in nsIFile.

https://reviewboard.mozilla.org/r/210108/#review217404

test review; please ignore.
Comment on attachment 8939803 [details]
Bug 685236 - Add mozilla::filesystem::Path and use it in nsIFile.

https://reviewboard.mozilla.org/r/210108/#review216186

Testing that posting reviews works.  MozReview even remembered my comments!

::: mfbt/Path.h:13
(Diff revision 1)
> +
> +#ifndef mozilla_Path_h
> +#define mozilla_Path_h
> +
> +namespace mozilla {
> +namespace filesystem {

We tend to discourage namespaces underneath `mozilla::`, and the usual `mozilla::filesystem::Path::value_type` feels like a mouthful.  Do you think it'd be reasonable to just have `mozilla::Path`, or were you trying to save that name for a more fully-featured `Path` class?

::: mfbt/Path.h:15
(Diff revision 1)
> +#define mozilla_Path_h
> +
> +namespace mozilla {
> +namespace filesystem {
> +
> +class Path

WDYT about calling this `PathTraits` or similar, since there's no actual data storage in this class?

::: mfbt/Path.h:18
(Diff revision 1)
> +#ifdef XP_WIN
> +  using value_type = char16_t;
> +#else
> +  using value_type = char;
> +#endif

I realize `value_type` is carried over here from `nsTString`.  But I don't know that it's gaining us anything having it here as a public API, or that it's a reasonable name for a public API in this case.

WDYT about making `value_type` private to `Path`, and then requiring people to use `Path::string_type::value_type` if they need to?  I realize that's more typing, but I don't expect it to be used very much, and I think it's slightly more obvious what's being referenced in that case.

::: mfbt/Path.h:23
(Diff revision 1)
> +  using string_type = nsTString<value_type>;
> +  using substring_type = nsTSubstring<value_type>;

Referencing these from MFBT is not really allowed; you're depending on users including some string header prior to this, which is not the typical way we structure headers in Gecko.  Why are we putting this in MFBT rather than in XPCOM?
Attachment #8939803 - Flags: review-
Attachment #8939814 - Flags: review?(jmathies) → review+
Attachment #8939825 - Attachment is obsolete: true
Attachment #8939826 - Attachment is obsolete: true
Attachment #8939827 - Attachment is obsolete: true
Attachment #8939828 - Attachment is obsolete: true
Attachment #8939808 - Attachment is obsolete: true
Attachment #8939830 - Attachment is obsolete: true
Attachment #8939809 - Attachment is obsolete: true
Attachment #8939831 - Attachment is obsolete: true
Attachment #8939810 - Attachment is obsolete: true
Attachment #8939832 - Attachment is obsolete: true
Attachment #8939812 - Attachment is obsolete: true
Attachment #8939813 - Attachment is obsolete: true
Attachment #8939817 - Attachment is obsolete: true
Attachment #8939818 - Attachment is obsolete: true
Attachment #8939819 - Attachment is obsolete: true
Attachment #8939807 - Flags: review?(honzab.moz)
Attachment #8939811 - Flags: review?(bzbarsky)
Attachment #8939814 - Flags: review?(jmathies)
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e11cfb3b5a
Stop using GetNativePath in netwerk/. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cd4dae63dd0
Stop using GetNativePath in uriloader/exthandler. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/eadb9e8fa467
Stop using GetNativePath in nsPluginDirWin.cpp. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/abe3ba8d9bc2
Stop using GetNativePath in dom/xul. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/6baff8c16157
Stop using GetNativePath in widget/windows. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/6764713f1dea
Stop using GetNativePath in PSM. r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/e734b0be9a1c
Workaround for WebRTC. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d33cca73ff0
Workaround Skia file path issue. r=jwatt
No longer depends on: 1428568
Flags: needinfo?(VYV03354)
I attached a patch in bug 1440278.
Flags: needinfo?(VYV03354)
Sorry, bug cloning is so confusing when the name is left unchanged.  Bug 1428258 is causing a break, tho.
You need to log in before you can comment on or make changes to this bug.