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

Status: NEW

(Core :: JavaScript Engine, enhancement, P5)

Description

[Andi [:andi]]

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;
>>    }

Comment 1

[Masatoshi Kimura [:emk]]

Will NSPR IO fix the issue? AFAIK NSPR does not support Unicode paths.

Comment 2

[Andi [:andi]]

After talking with :emk I've decided to use _wfopen.

Comment 3

[Andi [:andi]]

Attachment: Bug 1409692 - Use _wfopen instead of fopen on windows executed code in TraceLoggingGraph.

Review commit: https://reviewboard.mozilla.org/r/190724/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190724/

Comment 4

[Andi [:andi]]

The attached patch follows the logic from here: https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsXPCOMGlue.cpp#175

Comment 5

[Jan de Mooij [:jandem]]

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?

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Andi [:andi]]

Will update the patch Thanks!

Comment 8

[Andi [:andi]]

Comment on attachment 8919797 [details]
Bug 1409692 - Use _wfopen instead of fopen on windows executed code in TraceLoggingGraph.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190724/diff/1-2/

Comment 9

[Andi [:andi]]

I've added a wrapper function in mftb since we will be using it all other the code.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

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 11

[Andi [:andi]]

Comment on attachment 8919797 [details]
Bug 1409692 - Use _wfopen instead of fopen on windows executed code in TraceLoggingGraph.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190724/diff/2-3/

Comment 12

[Masatoshi Kimura [:emk]]

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