1098967 - LogAlloc has mangled output with e10s on Windows

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Mike Hommey [:glandium]]

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).

Comment 1

[Mike Hommey [:glandium]]

Attachment: Use native Win32 APIs to append to the LogAlloc output

Because it turns out that the POSIX API the CRT exposes doesn't do O_APPEND
in a sane manner.

Comment 2

[Nathan Froyd [:froydnj]]

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.)

Comment 3

[Mike Hommey [:glandium]]

(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?

Comment 4

[Nathan Froyd [:froydnj]]

(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.

Comment 5

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4405339504c9

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4405339504c9