Closed Bug 1098967 Opened 5 years ago Closed 5 years ago

LogAlloc has mangled output with e10s on Windows

Categories

(Core :: Memory Allocator, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s + ---

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

Very usefully, open(O_APPEND), on Windows, is not multi-process safe. But guess what? The native WIN32 equivalent is multi-process safe (for small writes, like on unix).
OS: All → Windows 7
Summary: LogAlloc has mangled output with e10s → LogAlloc has mangled output with e10s on Windows
Because it turns out that the POSIX API the CRT exposes doesn't do O_APPEND
in a sane manner.
Attachment #8522973 - Flags: review?(nfroyd)
Comment on attachment 8522973 [details] [diff] [review]
Use native Win32 APIs to append to the LogAlloc output

Review of attachment 8522973 [details] [diff] [review]:
-----------------------------------------------------------------

I feel like comments in the code are warranted at every #ifdef, but it's no good repeating them everywhere.  Maybe the main comment in FdPrintf or at its declaration, and other comments pointing there?

::: memory/replace/logalloc/LogAlloc.cpp
@@ +24,5 @@
>  #include "base/lock.h"
>  
>  static const malloc_table_t* sFuncs = nullptr;
> +static intptr_t sFd = 0;
> +static bool sStdOutorErr = false;

I think sStdoutOrStderr reads better.  (sStdOutOrErr is, I think, the more consistent way to name this, but reads oddly IMHO.)
Attachment #8522973 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 8522973 [details] [diff] [review]
> Use native Win32 APIs to append to the LogAlloc output
> 
> Review of attachment 8522973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel like comments in the code are warranted at every #ifdef, but it's no
> good repeating them everywhere.  Maybe the main comment in FdPrintf or at
> its declaration, and other comments pointing there?

What kind of comments do you have in mind? Like the one added to FdPrintf.h?
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Nathan Froyd (:froydnj) from comment #2)
> > Comment on attachment 8522973 [details] [diff] [review]
> > Use native Win32 APIs to append to the LogAlloc output
> > 
> > Review of attachment 8522973 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I feel like comments in the code are warranted at every #ifdef, but it's no
> > good repeating them everywhere.  Maybe the main comment in FdPrintf or at
> > its declaration, and other comments pointing there?
> 
> What kind of comments do you have in mind? Like the one added to FdPrintf.h?

I was thinking something along the lines of why we aren't using the C stdio bits on Windows, a less snarky comment 0 + comment 2, perhaps.
https://hg.mozilla.org/mozilla-central/rev/4405339504c9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.