Closed Bug 1428557 Opened 6 years ago Closed 6 years ago

Stop using GetNativePath in xpcom/

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(2 files)

(In reply to Nathan Froyd [:froydnj] from bug 685236 comment #70)
> 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.

I don't think nsIFile is available at such late stage:
https://dxr.mozilla.org/mozilla-central/rev/81362f7306fe413b19fdba27cd0e9a5525d902e1/xpcom/build/XPCOMInit.cpp#977,981
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
(In reply to Masatoshi Kimura [:emk] from comment #0)
> (In reply to Nathan Froyd [:froydnj] from bug 685236 comment #70)
> > 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.
> 
> I don't think nsIFile is available at such late stage:
> https://dxr.mozilla.org/mozilla-central/rev/
> 81362f7306fe413b19fdba27cd0e9a5525d902e1/xpcom/build/XPCOMInit.cpp#977,981

That is unfortunate, even if I think we wouldn't be using the functionality that's getting shut down here.
(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Masatoshi Kimura [:emk] from comment #0)
> > (In reply to Nathan Froyd [:froydnj] from bug 685236 comment #70)
> > > 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.
> > 
> > I don't think nsIFile is available at such late stage:
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 81362f7306fe413b19fdba27cd0e9a5525d902e1/xpcom/build/XPCOMInit.cpp#977,981
> 
> That is unfortunate, even if I think we wouldn't be using the functionality
> that's getting shut down here.

Maybe I can separate OpenDir/ReadDir/CloseDir from nsLocalFileWin.h and use it from LateWriteObserver.
Comment on attachment 8940689 [details]
Bug 1428557 - Stop using GetNativePath in xpcom/.

https://reviewboard.mozilla.org/r/210948/#review221508

Clearing the review request until I update the patch.
Attachment #8940689 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #3)
> That is unfortunate, even if I think we wouldn't be using the functionality
> that's getting shut down here.

nsLocalFile::Remove calls IsSymlink > ResolveAndStat > ResolveShortcut that depends on gResolver that will be destroyed in nsLocalFile::GlobalShutdown(). So we can't actually use nsLocalFile::Remove here.
The latest patch defines mozilla::Rename and mozilla::Unlink in FileUtils that will be used in subsequent patches.
Comment on attachment 8940690 [details]
Bug 1428557 - Implement wrappers in FileUtils.

https://reviewboard.mozilla.org/r/210950/#review222048

I guess we can do this, but I wonder whether we should put all this in FileUtils.cpp, or we should just stick the small wrappers in LateWriteChecks.cpp, and pay attention for opportunities to move that code out of LateWriteChecks.cpp.  WDYT?

::: config/external/nspr/pr/moz.build:87
(Diff revision 1)
>          # For historical reasons we use the WIN95 NSPR target instead of
>          # WINNT.
>          WIN95=True,
>          WINNT=False,
>          _PR_GLOBAL_THREADS_ONLY=True,
> +        MOZ_UNICODE = True,

I'm not very familiar with `MOZ_UNICODE` and what it enables.  It looks to provide some structure definitions and function prototypes in NSPR, but it also turns on some definitions of functionality in NSPR as well.  What happens if we build Firefox against a system NSPR that doesn't have `MOZ_UNICODE` turned on after these patches?  Does the build just fail, or does Firefox run with reduced or wrong functionality?

I guess it would be very unusual to use `--with-system-nspr` on Windows, so perhaps this define is harmless enough, especially since we already set the same define in `widget/windows/`.  Could you file a follow-up bug to forbid using `--with-system-nspr` and similar on Windows?

::: xpcom/glue/FileUtils.h:30
(Diff revision 1)
>  #include <limits.h>
>  
>  namespace mozilla {
>  
>  #if defined(XP_WIN)
> +#define NS_T(str) L##str

This macro doesn't appear to be used anywhere, we should just delete it entirely.

::: xpcom/glue/FileUtils.h:33
(Diff revision 1)
> +#ifdef MOZ_UNICODE
> +using Dir = PRDirUTF16;
> +using DirEntry = PRDirEntryUTF16;
> +#endif

Where does `MOZ_UNICODE` get defined prior to including this file?

I only see `MOZ_UNICODE` getting set for NSPR compilation-related things, and not any place where this file might be included.  Should this file be setting `MOZ_UNICODE` prior to including `prio.h` and similar, or should we be setting `MOZ_UNICODE` more widely?  (If we are setting `MOZ_UNICODE` more widely already, my apologies, but I can't seem to find any references to doing so in my searches.)

::: xpcom/glue/FileUtils.h:38
(Diff revision 1)
> +#ifdef MOZ_UNICODE
> +using Dir = PRDirUTF16;
> +using DirEntry = PRDirEntryUTF16;
> +#endif
>  #else
> +#define NS_T(str) str

Likewise for this.

::: xpcom/glue/FileUtils.h:139
(Diff revision 1)
> -inline void Rename(pathstr_t aOldPath, pathstr_t aNewPath);
> +void Rename(pathstr_t aOldPath, pathstr_t aNewPath);
> +int OSRename(pathstr_t aOldPath, pathstr_t aNewPath);

Why should we have two of these?  If we really do need two, we need some documentation on what the difference in behavior is and why you would use one versus the other.

::: xpcom/glue/FileUtils.h:141
(Diff revision 1)
> +PathString GetLibraryName(pathstr_t aPath, const char* aLib);
> +PathString GetLibraryFilePathname(pathstr_t aName, PRFuncPtr aAddr);

Some documentation on these, even just pointing at the relevant NSPR functions, would be nice.

::: xpcom/glue/FileUtils.h:141
(Diff revision 1)
>   * Wrappers for some NSPR/C runtime functions.
>   */
> -inline void Unlink(pathstr_t aFilePath);
> -inline void Rename(pathstr_t aOldPath, pathstr_t aNewPath);
> +void Unlink(pathstr_t aFilePath);
> +void Rename(pathstr_t aOldPath, pathstr_t aNewPath);
> +int OSRename(pathstr_t aOldPath, pathstr_t aNewPath);
> +PathString GetLibraryName(pathstr_t aPath, const char* aLib);

`aPath` should probably be called `aDirectory`.

::: xpcom/glue/FileUtils.h:242
(Diff revision 1)
> +#else
> +  return strlen(aString);
> +#endif
> +}
> +
> +inline int Strncmp(pathstr_t aStrA, pathstr_t aStrB, size_t aLen)

WDYT about just making `char*` overloads for `NS_strlen` and `NS_strncmp` instead of adding these functions?  We already do the overload for `NS_strndup`, and I think the overload would be preferable to adding functions that are very similarly named to their standard library counterparts.
Attachment #8940690 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #10)
> Comment on attachment 8940690 [details]
> Bug 1428557 - Implement wrappers in FileUtils.
> 
> https://reviewboard.mozilla.org/r/210950/#review222048
> 
> I guess we can do this, but I wonder whether we should put all this in
> FileUtils.cpp, or we should just stick the small wrappers in
> LateWriteChecks.cpp, and pay attention for opportunities to move that code
> out of LateWriteChecks.cpp.  WDYT?

These wrappers are not only for LastWriteChecks.cpp. Some other reviewers in bug 685236 disliked #ifdef clutter, so I'm adding wrappers for subsequent patches to be attached in bug 1428258. Would you like to see those patches now?

> ::: config/external/nspr/pr/moz.build:87
> (Diff revision 1)
> >          # For historical reasons we use the WIN95 NSPR target instead of
> >          # WINNT.
> >          WIN95=True,
> >          WINNT=False,
> >          _PR_GLOBAL_THREADS_ONLY=True,
> > +        MOZ_UNICODE = True,
> 
> I'm not very familiar with `MOZ_UNICODE` and what it enables.  It looks to
> provide some structure definitions and function prototypes in NSPR, but it
> also turns on some definitions of functionality in NSPR as well.  What
> happens if we build Firefox against a system NSPR that doesn't have
> `MOZ_UNICODE` turned on after these patches?  Does the build just fail, or
> does Firefox run with reduced or wrong functionality?
> 
> I guess it would be very unusual to use `--with-system-nspr` on Windows, so
> perhaps this define is harmless enough, especially since we already set the
> same define in `widget/windows/`.  Could you file a follow-up bug to forbid
> using `--with-system-nspr` and similar on Windows?

Firefox will not run with error "Could not find XPCOM".
Another idea is fix 1433265 to make nsIFile available even after XPCOM shutdown and use nsIFile.

> ::: xpcom/glue/FileUtils.h:30
> (Diff revision 1)
> >  #include <limits.h>
> >  
> >  namespace mozilla {
> >  
> >  #if defined(XP_WIN)
> > +#define NS_T(str) L##str
> 
> This macro doesn't appear to be used anywhere, we should just delete it
> entirely.

This macro will be used in subsequent patches.

> ::: xpcom/glue/FileUtils.h:33
> (Diff revision 1)
> > +#ifdef MOZ_UNICODE
> > +using Dir = PRDirUTF16;
> > +using DirEntry = PRDirEntryUTF16;
> > +#endif
> 
> Where does `MOZ_UNICODE` get defined prior to including this file?
> 
> I only see `MOZ_UNICODE` getting set for NSPR compilation-related things,
> and not any place where this file might be included.  Should this file be
> setting `MOZ_UNICODE` prior to including `prio.h` and similar, or should we
> be setting `MOZ_UNICODE` more widely?  (If we are setting `MOZ_UNICODE` more
> widely already, my apologies, but I can't seem to find any references to
> doing so in my searches.)

I considered to add MOZ_UNICODE under config/external/nspr/**/moz.build, but see also the above comment for another idea.

> ::: xpcom/glue/FileUtils.h:139
> (Diff revision 1)
> > -inline void Rename(pathstr_t aOldPath, pathstr_t aNewPath);
> > +void Rename(pathstr_t aOldPath, pathstr_t aNewPath);
> > +int OSRename(pathstr_t aOldPath, pathstr_t aNewPath);
> 
> Why should we have two of these?  If we really do need two, we need some
> documentation on what the difference in behavior is and why you would use
> one versus the other.

OSRename was removed from the latest patch.

> ::: xpcom/glue/FileUtils.h:141
> (Diff revision 1)
> > +PathString GetLibraryName(pathstr_t aPath, const char* aLib);
> > +PathString GetLibraryFilePathname(pathstr_t aName, PRFuncPtr aAddr);
> 
> Some documentation on these, even just pointing at the relevant NSPR
> functions, would be nice.

OK.

> ::: xpcom/glue/FileUtils.h:141
> (Diff revision 1)
> >   * Wrappers for some NSPR/C runtime functions.
> >   */
> > -inline void Unlink(pathstr_t aFilePath);
> > -inline void Rename(pathstr_t aOldPath, pathstr_t aNewPath);
> > +void Unlink(pathstr_t aFilePath);
> > +void Rename(pathstr_t aOldPath, pathstr_t aNewPath);
> > +int OSRename(pathstr_t aOldPath, pathstr_t aNewPath);
> > +PathString GetLibraryName(pathstr_t aPath, const char* aLib);
> 
> `aPath` should probably be called `aDirectory`.
> 
> ::: xpcom/glue/FileUtils.h:242
> (Diff revision 1)
> > +#else
> > +  return strlen(aString);
> > +#endif
> > +}
> > +
> > +inline int Strncmp(pathstr_t aStrA, pathstr_t aStrB, size_t aLen)
> 
> WDYT about just making `char*` overloads for `NS_strlen` and `NS_strncmp`
> instead of adding these functions?  We already do the overload for
> `NS_strndup`, and I think the overload would be preferable to adding
> functions that are very similarly named to their standard library
> counterparts.

Hm. I wanted to avoid `char*` overloads was used instead of standard library as much as possible, but it would not be a big deal.
Attachment #8940689 - Flags: review?(nfroyd)
Depends on: 1433265
Comment on attachment 8940690 [details]
Bug 1428557 - Implement wrappers in FileUtils.

https://reviewboard.mozilla.org/r/210950/#review222048

Now this patch is much simpler thanks to bug 1433265.

> I'm not very familiar with `MOZ_UNICODE` and what it enables.  It looks to provide some structure definitions and function prototypes in NSPR, but it also turns on some definitions of functionality in NSPR as well.  What happens if we build Firefox against a system NSPR that doesn't have `MOZ_UNICODE` turned on after these patches?  Does the build just fail, or does Firefox run with reduced or wrong functionality?
> 
> I guess it would be very unusual to use `--with-system-nspr` on Windows, so perhaps this define is harmless enough, especially since we already set the same define in `widget/windows/`.  Could you file a follow-up bug to forbid using `--with-system-nspr` and similar on Windows?

MOZ_UNICODE is no longer used.

> This macro doesn't appear to be used anywhere, we should just delete it entirely.

Removed this macro.

> Where does `MOZ_UNICODE` get defined prior to including this file?
> 
> I only see `MOZ_UNICODE` getting set for NSPR compilation-related things, and not any place where this file might be included.  Should this file be setting `MOZ_UNICODE` prior to including `prio.h` and similar, or should we be setting `MOZ_UNICODE` more widely?  (If we are setting `MOZ_UNICODE` more widely already, my apologies, but I can't seem to find any references to doing so in my searches.)

Removed the entire block along with MOZ_UNICODE.

> Why should we have two of these?  If we really do need two, we need some documentation on what the difference in behavior is and why you would use one versus the other.

These functions are no longer added.

> Some documentation on these, even just pointing at the relevant NSPR functions, would be nice.

Added a comment.

> `aPath` should probably be called `aDirectory`.

Done.

> WDYT about just making `char*` overloads for `NS_strlen` and `NS_strncmp` instead of adding these functions?  We already do the overload for `NS_strndup`, and I think the overload would be preferable to adding functions that are very similarly named to their standard library counterparts.

This function is no longer added (StringBeginsWith will be used in the subsequent patch).
Comment on attachment 8940690 [details]
Bug 1428557 - Implement wrappers in FileUtils.

https://reviewboard.mozilla.org/r/210950/#review225374
Attachment #8940690 - Flags: review?(nfroyd) → review+
Comment on attachment 8940689 [details]
Bug 1428557 - Stop using GetNativePath in xpcom/.

https://reviewboard.mozilla.org/r/210948/#review225378

Thank you!
Attachment #8940689 - Flags: review?(nfroyd) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/badef7a8ef70
Stop using GetNativePath in xpcom/. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/3fd159786be1
Implement wrappers in FileUtils. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/badef7a8ef70
https://hg.mozilla.org/mozilla-central/rev/3fd159786be1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.