Open
Bug 1409692
Opened 7 years ago
Updated 2 years ago
Use _wfopen instead of fopen on windows executed code in js/src/vm/TraceLoggingGraph.cpp
Categories
(Core :: JavaScript Engine, enhancement, P5)
Core
JavaScript Engine
Tracking
()
NEW
People
(Reporter: andi, Assigned: andi)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Details |
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; >> }
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jdemooij)
Comment 1•7 years ago
|
||
Will NSPR IO fix the issue? AFAIK NSPR does not support Unicode paths.
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Comment 2•7 years ago
|
||
After talking with :emk I've decided to use _wfopen.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
The attached patch follows the logic from here: https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsXPCOMGlue.cpp#175
Comment 5•7 years ago
|
||
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)
Comment 6•7 years ago
|
||
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.
Assignee | ||
Comment 7•7 years ago
|
||
Will update the patch Thanks!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
I've added a wrapper function in mftb since we will be using it all other the code.
Comment 10•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
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
Assignee | ||
Updated•7 years ago
|
Attachment #8919797 -
Flags: review?(jdemooij)
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Priority: -- → P5
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•