Open Bug 1409692 Opened 4 years ago Updated 3 years ago

Use _wfopen instead of fopen on windows executed code in js/src/vm/TraceLoggingGraph.cpp

Categories

(Core :: JavaScript Engine, enhancement, P5)

enhancement

Tracking

()

Tracking Status
firefox57 --- wontfix
firefox58 --- affected

People

(Reporter: andi, Assigned: andi)

References

Details

Attachments

(1 file)

because of this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1389160 we have implemented this windows specific checker: https://bugzilla.mozilla.org/show_bug.cgi?id=1391711
In order for this patch to land we need fopen line occurrences to be translated that doesn't suffer of lossy conversion.

Do you think it's convenient for us to cache  AutoFDClose instead of FILE* in:

>>class TraceLoggerGraphState
>>{
>>    uint32_t numLoggers;
>>    uint32_t pid_;
>>
>>    // File pointer to the "tl-data.json" file. (Explained above).
>>    FILE* out;

and used in:

>>    js::UniqueChars filename = AllocTraceLogFilename("tl-data.%u.json", pid_);
>>    out = fopen(filename.get(), "w");
>>    if (!out) {
>>        fprintf(stderr, "warning: failed to create TraceLogger output file >>%s\n", filename.get());
>>        return false;
>>    }
Flags: needinfo?(jdemooij)
Will NSPR IO fix the issue? AFAIK NSPR does not support Unicode paths.
Summary: Use NSPR IO instead of fopen on windows executed code in js/src/vm/TraceLoggingGraph.cpp → Use _wfopen instead of fopen on windows executed code in js/src/vm/TraceLoggingGraph.cpp
After talking with :emk I've decided to use _wfopen.
The attached patch follows the logic from here: https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsXPCOMGlue.cpp#175
Could we use a static function as wrapper around fopen, so we don't have to duplicate the same #ifdef logic at least 5 times?
Flags: needinfo?(jdemooij)
Btw TL is just a developer feature that's not really used atm so I don't think we should spend time on caching things.
Will update the patch Thanks!
I've added a wrapper function in mftb since we will be using it all other the code.
Comment on attachment 8919797 [details]
Bug 1409692 - Use _wfopen instead of fopen on windows executed code in TraceLoggingGraph.

https://reviewboard.mozilla.org/r/190724/#review196274

::: mfbt/FileAccessWrappers.h:24
(Diff revision 2)
> +namespace mozilla {
> +
> +FILE*
> +FileOpen(const char* path, const char* mode)
> +{
> +#ifdef XP_WIN

I think you should explain why you are doing that!
Comment on attachment 8919797 [details]
Bug 1409692 - Use _wfopen instead of fopen on windows executed code in TraceLoggingGraph.

https://reviewboard.mozilla.org/r/190724/#review196308

::: mfbt/FileAccessWrappers.h:32
(Diff revision 3)
> +   * Linux where the 8-bit string is the canonical POSIX filename. */
> +  const size_t wPathSize = strlen(path) + 1;
> +  const size_t wModeSize = strlen(mode) + 1;
> +  wchar_t wPath[wPathSize];
> +  wchar_t wMode[wModeSize];
> +  MultiByteToWideChar(CP_UTF8, 0, path, -1, wPath, wPathSize);

I think we should not add this wrapper because this is very easy to misuse.

AllocTraceLogFilename uses a filename from getenv[1],  so the filename is encoded in ANSI encoding, not in UTF-8.

It is simply wrong to use CP_UTF8 blindly. The correct fix would be taking a Unicode path by _wgetenv and passing it to _wfopen.

[1] https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/js/src/vm/TraceLoggingGraph.cpp#60
Attachment #8919797 - Flags: review?(jdemooij)
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.