Closed Bug 973090 Opened 10 years ago Closed 10 years ago

Electrolyze GC/CC log writing.

Categories

(Toolkit :: about:memory, defect)

Product:
Toolkit
Component:
about:memory
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 12 obsolete files)

bug973090-take2-hg5.diff
69.72 KB, patch
jld
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #946407 +++

Let's have a separate bug for the GC/CC log remoting.

We'll probably want an IPC abstraction at the "write this string" level — either child->parent messages on the remote-CC protocol or two instanes of a separate "remote log file" actor that's passed parent->child as message parameters.  That means factoring out the relevant parts of nsMemoryInfoDumper.
Migrating the blocking nom from the dupe.
blocking-b2g: --- → 1.4?
Hi Jed,

Thanks for shedding some light on bug 986250. So the crash I'm seeing happens because I'm using DMD?
Flags: needinfo?(jld)
(In reply to Diego Wilson [:diego] from comment #3)
> Thanks for shedding some light on bug 986250. So the crash I'm seeing
> happens because I'm using DMD?

If DMD is enabled then sandboxing is disabled (see bug 956961), so it's not that.

Workarounds for this bug include:
* If the GC/CC logs aren't needed, use `./tools/get_about_memory.py --no-gc-cc-log` instead.
* If they are, then disable sandboxing by setting MOZ_DISABLE_CONTENT_SANDBOX in the environment (or building gecko with --disable-content-sandbox).
Flags: needinfo?(jld)
(In reply to Jed Davis [:jld] from comment #4)
> If DMD is enabled then sandboxing is disabled (see bug 956961), so it's not
> that.

You're right. It's off in my builds.

> Workarounds for this bug include:
> * If the GC/CC logs aren't needed, use `./tools/get_about_memory.py
> --no-gc-cc-log` instead.

Looks like this fixed the crash in bug 986250, thanks!
Can we use the work around per Comment 5?
The issue is not blocking anymore. We can just avoid using the feature as per comment 5.
blocking-b2g: 1.4? → ---
Taking this because it'd be nice if it were at least possible to get this into 1.4.

Vague plan: go into nsCycleCollector.cpp and jsfriendapi.cpp and abstract the fprintfs so they can be replaced with asprintf (or snprintf, if we can't assume that) into a buffer and send it to the parent over IPC, either with an actor per log file or by sending a filename with each string to write.

The parent could just log the (filename, string) pairs to a file (JSON, maybe?) and the collection script could demultiplex it, which would remove the other reason for the MOZ_IGNORE_NUWA_PROCESS hack, but that would break compatibility with anything else that expects the current format.  Not quite sure what the Right Thing to do here is.
Assignee: nobody → jld
Attachment #8406690 - Attachment filename: bug973090-cclog-e10s-p2-hg2.diff → CC logging.
Attachment #8406690 - Attachment description: bug973090-cclog-e10s-p2-hg2.diff → Step 3: IPC remoting for child GC/CC logging.
Attachment #8406690 - Attachment filename: CC logging. → bug973090-cclog-e10s-p2-hg2.diff
The reviewers are the usual vague guess based on blame/log; adjust as needed.

Lingering questions:

* Should nsCycleCollectorLogSinkToFile be an XPCOM service?  Relatedly, can/should the current inelegant situation with AllocPCycleCollectWithLogsParent be avoided? 

* How much of a problem is it that this makes B2G GC/CC log dumping 2x slower?  (In contrast, it was more like 14x slower without buffering in front of IPC.)  The next thing to optimize looks like nsCString::AppendPrintf, and it would probably need to be "optimized" by reinventing it with snprintf.

* Is it a problem that the GC log isn't closed/renamed until the CC log is also done?

As well as a few more minor things (e.g., naming) that I'm sure people will tell me about if I got them sufficiently wrong.
(In reply to Jed Davis [:jld] from comment #12)
> Lingering questions:

One more: The child processes will begin IPCing log text to the parent while the parent's main thread is busy with its own heap dump and CC; how can/should this be avoided?  (Previously they'd write to files on their own, so it made sense to run them in parallel with the parent.)
(In reply to Jed Davis [:jld] from comment #13)
> (In reply to Jed Davis [:jld] from comment #12)
> > Lingering questions:
> 
> One more: The child processes will begin IPCing log text to the parent while
> the parent's main thread is busy with its own heap dump and CC; how
> can/should this be avoided?  (Previously they'd write to files on their own,
> so it made sense to run them in parallel with the parent.)

This might actually be a good job for PBackground ...
Whiteboard: [memshrink]
Whiteboard: [memshrink] → [MemShrink:P2]
Comment on attachment 8406687 [details] [diff] [review]
Step 1: Allow JS heap logging to things that aren't files.

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

This looks reasonable to me, but Terrence has looked at this code more recently than me, and probably is more familiar with varargs stuff.

::: js/src/jsfriendapi.cpp
@@ +721,5 @@
>  }
>  
>  #endif
>  
> +class JSDumpHeapTracer : public JSTracer

I have a feeling that some compilers complain about a class inheriting from a struct?  Maybe Visual Studio?  Though Terrence is changing JSTracer to a class soon, anyways, in bug 807168...

@@ +729,5 @@
> +    void * const ctx_;
> +
> +    static void vfprintf_callback(void *ctx, const char *format, va_list ap)
> +    {
> +        vfprintf(reinterpret_cast<FILE *>(ctx), format, ap);

Can you not just static_cast here?
Attachment #8406687 - Flags: review?(continuation) → review?(terrence)
Comment on attachment 8406687 [details] [diff] [review]
Step 1: Allow JS heap logging to things that aren't files.

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

r=me  Nice! Just a few style nits.

::: js/src/jsfriendapi.cpp
@@ +726,3 @@
>  {
> +    typedef void (*Callback)(void *ctx, const char *format, va_list ap);
> +    const Callback cb_;

Please spell the class member out fully as |callback_|.

@@ +728,5 @@
> +    const Callback cb_;
> +    void * const ctx_;
> +
> +    static void vfprintf_callback(void *ctx, const char *format, va_list ap)
> +    {

{ on previous line for methods.

@@ +731,5 @@
> +    static void vfprintf_callback(void *ctx, const char *format, va_list ap)
> +    {
> +        vfprintf(reinterpret_cast<FILE *>(ctx), format, ap);
> +    }
> +public:

One newline and a two-space indent before public:.

@@ +742,4 @@
>      {}
> +
> +    void printf(const char *format, ...)
> +    {

{ on previous line.

@@ +824,5 @@
>              JS_GetTraceEdgeName(dtrc, buffer, sizeof(buffer)));
>  }
>  
> +static void
> +DumpHeapCompleteCommon(JSRuntime *rt, JSDumpHeapTracer *dtrc,

DumpHeapCommon.

::: js/src/jsfriendapi.h
@@ +429,5 @@
>    */
>  extern JS_FRIEND_API(void)
>  DumpHeapComplete(JSRuntime *rt, FILE *fp, DumpHeapNurseryBehaviour nurseryBehaviour);
> +extern JS_FRIEND_API(void)
> +DumpHeapCompleteExt(JSRuntime *rt, void (*callback)(void *, const char *, va_list), void *ctx,

(1) There is pretty much universal opprobrium around here for both the name DumpHeapComplete and using Ext prefixes to add new methods. Please rename these methods to |DumpHeapToFile| and |DumpHeapToCallback| respectively.

(2) Please templatize this on the type of |ctx|.
Attachment #8406687 - Flags: review?(terrence) → review+
Comment on attachment 8406689 [details] [diff] [review]
Step 2: Factor out the log writing from nsCycleCollectorLogger.

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

Thanks for fixing this, it is pretty gnarly.

::: xpcom/base/CycleCollectedJSRuntime.h
@@ +206,5 @@
>                          void* aThing);
>    void DeferredFinalize(nsISupports* aSupports);
>  
>    void DumpJSHeap(FILE* aFile);
> +  void DumpJSHeap(void (*callback)(void* ctx, const char* format, va_list), void* ctx);

Please define a new typedef for this callback type in jsfriendapi.h and use it there and here.  That seems to be the standard style used in JS api, and it is kind of nice to be able to search for it and find where it is used and defined.

::: xpcom/base/nsCycleCollector.cpp
@@ +1410,5 @@
> +        mProcessIdentifier = aIdentifier;
> +        return NS_OK;
> +    }
> +
> +    NS_IMETHOD GetGcLogPath(nsAString &aPath)

nit: please be consistent with where you put the & and *.  I guess on the left is better.  You have it on the right in a few places.  Please do a pass over the code you added in this file and fix that up.

@@ +1467,5 @@
> +        FILE *mStream;
> +        nsString mPath;
> +
> +        FileInfo(const char *aPrefix) : mPrefix(aPrefix), mStream(nullptr) { }
> +    } mGCLog, mCCLog;

nit: Please separate out mGCLog and mCCLog from the struct into two separate declarations.

@@ +1523,5 @@
> +
> +    nsresult OpenLog(FileInfo& log)
> +    {
> +        // Initially create the log in a file starting with
> +        // "incomplete-gc-edges".  We'll move the file and strip off the

Not just gc-edges now, right?  Maybe just change it to "incomplete-"?

@@ +1525,5 @@
> +    {
> +        // Initially create the log in a file starting with
> +        // "incomplete-gc-edges".  We'll move the file and strip off the
> +        // "incomplete-" once the dump completes.  (We do this because we don't
> +        // want scripts which poll the filesystem looking for gc/cc dumps to

While you are here, please upper case GC and CC in "gc/cc dumps".

@@ +1576,5 @@
> +            nsString msg = NS_LITERAL_STRING("Garbage Collector log dumped to ") +
> +                logPath;
> +            cs->LogStringMessage(msg.get());
> +
> +            log.mPath = logPath;

I actually have a separate bug on file for this already, bug 994196, but you might as well fix it while you are here: the problem with this code is that we only set .mPath when the console service exists.  So just hoist the assignment and the thing that makes logPath etc. outside of the if statement. (This shows up even on desktop when you try to look at the path property in shutdown.)

@@ +1858,5 @@
> +
> +    static void GCPrintCB(void* ctx, const char* format, va_list ap)
> +    {
> +        nsCycleCollectorLogger* that =
> +            reinterpret_cast<nsCycleCollectorLogger*>(ctx);

static_cast please.  I think it is nice to use the weakest cast possible, and reserve reinterpret_cast for when you are going full-cowboy.  But maybe that's just me.

@@ +1883,5 @@
>  {
>      if (NS_WARN_IF(aOuter))
>          return NS_ERROR_NO_AGGREGATION;
>  
> +    // FIXME is this right?

I think it should be okay, but I'm not a QI expert.  Anyways, you should find out and remove this comment before landing. ;)

::: xpcom/base/nsICycleCollectorListener.idl
@@ +52,5 @@
> +
> +/**
> + * This interface allows replacing the log-writing backend for an
> + * nsICycleCollectorListener.  As this interface is also called while
> + * the cycle collector works, it cannot be implemented in JS.

"works" --> "is running" please.
Attachment #8406689 - Flags: review?(continuation) → review+
Comment on attachment 8406690 [details] [diff] [review]
Step 3: IPC remoting for child GC/CC logging.

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

This looks good to me, but I'm not a peer for any of this code.

::: dom/ipc/ContentChild.cpp
@@ +274,5 @@
> +        if (!mCCBuffer.IsEmpty()) {
> +            SendWriteCCLog(mCCBuffer);
> +        }
> +        Send__delete__(this);
> +        mDestroyed = true;

Won't this line cause us to write to memory that has just been freed? I think Send__delete__ should call ActorDestroy anyway.

::: dom/ipc/ContentParent.cpp
@@ +350,5 @@
> +    }
> +
> +    virtual ~CycleCollectWithLogsParent()
> +    {
> +        MOZ_COUNT_CTOR(CycleCollectWithLogsParent);

DTOR.

@@ +2660,5 @@
> +ContentParent::CycleCollectWithLogs(nsICycleCollectorLogSink* aSink,
> +                                    bool aDumpAllTraces)
> +{
> +    CycleCollectWithLogsParent *actor = new CycleCollectWithLogsParent(aSink);
> +    aSink->Begin();

It would be cleaner to do this in the CycleCollectWithLogsParent constructor.

@@ +2663,5 @@
> +    CycleCollectWithLogsParent *actor = new CycleCollectWithLogsParent(aSink);
> +    aSink->Begin();
> +    return SendPCycleCollectWithLogsConstructor(actor, aDumpAllTraces);
> +}
> +

Extra blank line.

::: xpcom/base/nsCycleCollector.cpp
@@ +3911,5 @@
>  
>      return data->mCollector->FreeSnowWhite(false);
>  }
>  
> +nsICycleCollectorLogSink* nsCycleCollector_getLogSink()

Return type should go on a separate line.

::: xpcom/base/nsCycleCollector.h
@@ +40,5 @@
>  
>  void nsCycleCollector_dispatchDeferredDeletion(bool aContinuation = false);
>  bool nsCycleCollector_doDeferredDeletion();
>  
> +// FIXME: should this be an XPCOM service?

That seems unnecessary.

::: xpcom/base/nsIMemoryInfoDumper.idl
@@ +156,5 @@
>                               in bool aDumpChildProcesses,
>                               out AString aGCLogPath,
>                               out AString aCCLogPath);
> +
> +  // COMMENTME

Fix this somehow.

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ +240,5 @@
>    if (aDumpChildProcesses) {
>      nsTArray<ContentParent*> children;
>      ContentParent::GetAll(children);
>      for (uint32_t i = 0; i < children.Length(); i++) {
> +      ContentParent* child = children[i];

Can you call this |cp| instead of |child|? It's confusing to have a ContentParent called child.
Attachment #8406690 - Flags: review?(wmccloskey) → review+
(In reply to Terrence Cole [:terrence] from comment #16)
> (2) Please templatize this on the type of |ctx|.

Given that `export template` never caught on, I'd have to drag a bunch of the implementation into jsfriendapi.h, and I don't think we want that?
(In reply to Andrew McCreight [:mccr8] from comment #17)
> > +  void DumpJSHeap(void (*callback)(void* ctx, const char* format, va_list), void* ctx);
> 
> Please define a new typedef for this callback type in jsfriendapi.h and use
> it there and here.  That seems to be the standard style used in JS api, and
> it is kind of nice to be able to search for it and find where it is used and
> defined.

Using the jsfriendapi.h typedef here would mean #include'ing jsfriendapi.h into CycleCollectedJSRuntime.h instead of not; do we want that?  I've made a different typedef of the same type instead (and if for some reason one of them changes, the compiler should flag it, I think?).

> > +    NS_IMETHOD GetGcLogPath(nsAString &aPath)
> 
> nit: please be consistent with where you put the & and *.  I guess on the
> left is better.  You have it on the right in a few places.  Please do a pass
> over the code you added in this file and fix that up.

I was trying to keep them on the left in this part of the tree — I think that's more or less the existing style — but a few slipped through.

> @@ +1576,5 @@
> > +            nsString msg = NS_LITERAL_STRING("Garbage Collector log dumped to ") +
> > +                logPath;
> > +            cs->LogStringMessage(msg.get());
> > +
> > +            log.mPath = logPath;
> 
> I actually have a separate bug on file for this already, bug 994196, but you
> might as well fix it while you are here: the problem with this code is that
> we only set .mPath when the console service exists.  So just hoist the
> assignment and the thing that makes logPath etc. outside of the if
> statement. (This shows up even on desktop when you try to look at the path
> property in shutdown.)

I thought that looked a little odd.  Fixed.

> @@ +1858,5 @@
> > +
> > +    static void GCPrintCB(void* ctx, const char* format, va_list ap)
> > +    {
> > +        nsCycleCollectorLogger* that =
> > +            reinterpret_cast<nsCycleCollectorLogger*>(ctx);
> 
> static_cast please.  I think it is nice to use the weakest cast possible,
> and reserve reinterpret_cast for when you are going full-cowboy.  But maybe
> that's just me.

My thought was that casting back from void* feels like a reinterpret_cast in terms of danger level, but the goal is to make sense to not-me, so I'll change it.

> @@ +1883,5 @@
> >  {
> >      if (NS_WARN_IF(aOuter))
> >          return NS_ERROR_NO_AGGREGATION;
> >  
> > +    // FIXME is this right?
> 
> I think it should be okay, but I'm not a QI expert.  Anyways, you should
> find out and remove this comment before landing. ;)

I was _sure_ I'd grepped for all the FIXMEs before I posted these.  Oops.  But also, that can stay as nsISupports* — my first draft had a separate nsICycleCollectorLogSource role for nsCycleCollectorLogger, so the upcast to nsISupports* was ambiguous, but it just made things more complicated for no real benefit.  (And I thought I'd taken out all the leftovers from that, too, but no.)

> ::: xpcom/base/nsICycleCollectorListener.idl
> @@ +52,5 @@
> > +
> > +/**
> > + * This interface allows replacing the log-writing backend for an
> > + * nsICycleCollectorListener.  As this interface is also called while
> > + * the cycle collector works, it cannot be implemented in JS.
> 
> "works" --> "is running" please.

Copied from the corresponding warning on nsICycleCollectorListener… so I'll change both of them.
(In reply to Bill McCloskey (:billm) from comment #18)
> ::: dom/ipc/ContentChild.cpp
> @@ +274,5 @@
> > +        if (!mCCBuffer.IsEmpty()) {
> > +            SendWriteCCLog(mCCBuffer);
> > +        }
> > +        Send__delete__(this);
> > +        mDestroyed = true;
> 
> Won't this line cause us to write to memory that has just been freed? I
> think Send__delete__ should call ActorDestroy anyway.

That will release the reference that the IPC subsystem owns, but the XPCOM caller will also own a reference, I think?

> ::: dom/ipc/ContentParent.cpp
> @@ +350,5 @@
> > +    }
> > +
> > +    virtual ~CycleCollectWithLogsParent()
> > +    {
> > +        MOZ_COUNT_CTOR(CycleCollectWithLogsParent);
> 
> DTOR.

Oops.  Thanks.

(Everything I haven't specifically commented on is also in the "fixed; thanks" bucket.)
Sounds good to me.
(In reply to Jed Davis [:jld] from comment #21)
> (In reply to Bill McCloskey (:billm) from comment #18)
> > ::: dom/ipc/ContentChild.cpp
> > @@ +274,5 @@
> > > +        if (!mCCBuffer.IsEmpty()) {
> > > +            SendWriteCCLog(mCCBuffer);
> > > +        }
> > > +        Send__delete__(this);
> > > +        mDestroyed = true;
> > 
> > Won't this line cause us to write to memory that has just been freed? I
> > think Send__delete__ should call ActorDestroy anyway.
> 
> That will release the reference that the IPC subsystem owns, but the XPCOM
> caller will also own a reference, I think?

I guess that's true in this case, but it requires some pretty non-local reasoning. I think it would be better to fix it anyway.
Carrying over r=terrence.
Attachment #8406687 - Attachment is obsolete: true
Attachment #8409798 - Flags: review+
Carrying over r=mccr8.
Attachment #8406689 - Attachment is obsolete: true
Attachment #8409799 - Flags: review+
Carrying over r=billm (but more review is needed).
Attachment #8406690 - Attachment is obsolete: true
Attachment #8409800 - Flags: review+
:bent raises an interesting point: why am I passing strings child->parent instead of passing FileDescriptors parent->child?  Also, it's been confirmed with these patches as-is we'll buffer the IPC messages (containing, potentially, most or all of the children's GC/CC logs) in memory while the parent is busy, which is bad.
Redid... a lot of stuff.  Everything is in one patch now.  No more changes to jsfriendapi, because now everything uses FILE*.  And the XPCOM interface for requesting logs now reports the child process logs, and there's a test.

This may need more reviewers.
Attachment #8409798 - Attachment is obsolete: true
Attachment #8409799 - Attachment is obsolete: true
Attachment #8409800 - Attachment is obsolete: true
Attachment #8411440 - Flags: review?(continuation)
…and of course one typo gets in with the last-minute cleanups.  Let's try this again.
Attachment #8411440 - Attachment is obsolete: true
Attachment #8411440 - Flags: review?(continuation)
Attachment #8411462 - Flags: review?(continuation)
…and one more “slight” change that wasn't quite right, in the Windows-only part.  But it passes try now.
Attachment #8411462 - Attachment is obsolete: true
Attachment #8411462 - Flags: review?(continuation)
Attachment #8411524 - Flags: review?(continuation)
Comment on attachment 8411524 [details] [diff] [review]
IPC remoting for child GC/CC logging (using FileDescriptor passing).

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

nit: "an scraping" (in the patch description)

The dom/ipc/ changes look fine to me, but bent or somebody should look at them, too.  Somebody also needs to review the ipc/glue/ changes, which I have idea about.  I guess bent can review that, too?

::: dom/ipc/ContentParent.cpp
@@ +389,5 @@
> +    }
> +
> +    virtual bool Recv__delete__()
> +    {
> +        // Report completion to mCallback only only successful

nit: "only only" should be "only on"?

::: toolkit/components/aboutmemory/tests/test_dumpGCAndCCLogsToFile.xul
@@ +1,1 @@
> +<?xml version="1.0"?>

Somebody who understands child process stuff should look at this, too.  Maybe billm?

Nice test.

@@ +56,5 @@
> +      let numChildren = 0;
> +      // FIXME: should there be some kind of unique-ish identifier?
> +      dumper.dumpGCAndCCLogsToFile(
> +	  /* identifier: */ "",
> +	  /* allTraces: */ true,

Why are you passing in true for allTraces?

::: xpcom/base/nsCycleCollector.cpp
@@ +1375,5 @@
>    Type mType;
>  };
>  
> +class nsCycleCollectorLogSinkToFile MOZ_FINAL :
> +    public nsICycleCollectorLogSink

just put the "public nsICycleCollectorLogSink" on the same line as the class. We're not super strict about line width.

@@ +1377,5 @@
>  
> +class nsCycleCollectorLogSinkToFile MOZ_FINAL :
> +    public nsICycleCollectorLogSink
> +{
> + public:

micronit: no indent on public:

@@ +1466,5 @@
> +            fclose(mCCLog.mStream);
> +        }
> +    }
> +
> + private:

micronit: no indent on private:

@@ +1467,5 @@
> +        }
> +    }
> +
> + private:
> +    int32_t mProcessIdentifier;

Please put all the fields at the very end of the class definition.

@@ +1505,5 @@
> +            NS_NewNativeLocalFile(nsCString(env), /* followLinks = */ true,
> +                                  &logFile);
> +        }
> +
> +        // In Android case, this function will open a file named aFilename under

I know you are just moving existing code, but this should be something like "In the case of Android," and "a specific folder".

@@ +1667,5 @@
>          if (mDisableLog) {
>              return NS_OK;
>          }
>  
> +        rv = mLogSink->Open(&mGCLog, &mCCLog);

I think mGCLog can just be a local variable in Begin().  mCCLog is certainly a nicer than than "mStream".

::: xpcom/base/nsCycleCollector.h
@@ +40,5 @@
>  
>  void nsCycleCollector_dispatchDeferredDeletion(bool aContinuation = false);
>  bool nsCycleCollector_doDeferredDeletion();
>  
> +nsICycleCollectorLogSink* nsCycleCollector_getLogSink();

Please rename this to something like nsCycleCollector_getNewLogSink() to indicate that you are creating a new one, rather than returning some singleton.

::: xpcom/base/nsIMemoryInfoDumper.idl
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
> +#include "nsICycleCollectorListener.idl"

Maybe you can just forward declare rather than include?

@@ +37,5 @@
> +              in nsIFile aCCLog,
> +              in bool aIsParent);
> +
> +  /**
> +   * Called when GC/CC logging has finished.

Is the idea that this is called when all processes have called onDump()?

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ +242,5 @@
>  
> +// Use XPCOM refcounting to fire |onFinish| when all reference-holders
> +// (remote dump actors or the |DumpGCAndCCLogsToFile| activation itself)
> +// have gone away.
> +class nsDumpGCAndCCLogsCallbackHolder : public nsIDumpGCAndCCLogsCallback {

nit: { on new line

@@ +243,5 @@
> +// Use XPCOM refcounting to fire |onFinish| when all reference-holders
> +// (remote dump actors or the |DumpGCAndCCLogsToFile| activation itself)
> +// have gone away.
> +class nsDumpGCAndCCLogsCallbackHolder : public nsIDumpGCAndCCLogsCallback {
> +  nsCOMPtr<nsIDumpGCAndCCLogsCallback> mCallback;

nit: put this in a private block at the end of the class decl please.

@@ +250,5 @@
> +  NS_DECL_ISUPPORTS
> +
> +  nsDumpGCAndCCLogsCallbackHolder(nsIDumpGCAndCCLogsCallback* aCallback)
> +    : mCallback(aCallback)
> +  { }

nit: } on new line
Attachment #8411524 - Flags: review?(continuation)
Attachment #8411524 - Flags: review?(bent.mozilla)
Attachment #8411524 - Flags: review+
(In reply to Andrew McCreight [:mccr8] from comment #31)
> ::: toolkit/components/aboutmemory/tests/test_dumpGCAndCCLogsToFile.xul
> @@ +1,1 @@
> > +<?xml version="1.0"?>
> 
> Somebody who understands child process stuff should look at this, too. 
> Maybe billm?

The IPC stuff there is copied directly from existing tests (specifically test_memoryReporters2.xul, and it's also in test_aboutmemory5.xul), for what it's worth.  Someone might want to deduplicate it at some point....

> > +      // FIXME: should there be some kind of unique-ish identifier?

Sigh.  So, the files are CreateUnique()d anyway, and the default is seconds-since-epoch + pid… but it might be nice to have a "test" in the name somewhere.

> > +      dumper.dumpGCAndCCLogsToFile(
> > +	  /* identifier: */ "",
> > +	  /* allTraces: */ true,
> 
> Why are you passing in true for allTraces?

Is there anything that would get better test coverage if it were true (given that test_aboutmemory6.xul clicks both buttons, but without remote browsers running)?

> ::: xpcom/base/nsCycleCollector.h
> @@ +40,5 @@
> >  
> >  void nsCycleCollector_dispatchDeferredDeletion(bool aContinuation = false);
> >  bool nsCycleCollector_doDeferredDeletion();
> >  
> > +nsICycleCollectorLogSink* nsCycleCollector_getLogSink();
> 
> Please rename this to something like nsCycleCollector_getNewLogSink() to
> indicate that you are creating a new one, rather than returning some
> singleton.

nsCycleCollector_createLogSink should be obvious enough, I think?

> @@ +37,5 @@
> > +              in nsIFile aCCLog,
> > +              in bool aIsParent);
> > +
> > +  /**
> > +   * Called when GC/CC logging has finished.
> 
> Is the idea that this is called when all processes have called onDump()?

Yes; I'll make that explicit.

> @@ +250,5 @@
> > +  NS_DECL_ISUPPORTS
> > +
> > +  nsDumpGCAndCCLogsCallbackHolder(nsIDumpGCAndCCLogsCallback* aCallback)
> > +    : mCallback(aCallback)
> > +  { }
> 
> nit: } on new line

So many different C++ styles....
(In reply to Andrew McCreight [:mccr8] from comment #31)
> @@ +1467,5 @@
> > +        }
> > +    }
> > +
> > + private:
> > +    int32_t mProcessIdentifier;
> 
> Please put all the fields at the very end of the class definition.

The definition of FileInfo has to precede the definitions of OpenLog() and CloseLog(), apparently, so that part has to stay at the top.  Or else the definitions of those methods can move outside the class def'n.  Thoughts?

        Attachment #8411524 -
        Attachment is obsolete: true

        Attachment #8411524 -
        Flags: review?(bent.mozilla)

        Attachment #8413088 -
        Flags: review?(bent.mozilla)

    
    

    
  
Comment on attachment 8413088 [details] [diff] [review]
Remote GC/CC logging, revised after comment #31

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

This looks really great, sorry for the delay here.

Lots of comments below but many are not showstoppers. I'll happily stamp the next version of the patch, I just want to see it.

::: dom/ipc/ContentChild.cpp
@@ +205,5 @@
>      MOZ_COUNT_DTOR(MemoryReportRequestChild);
>  }
>  
> +// IPC sender for remote GC/CC logging.
> +class CycleCollectWithLogsChild : public PCycleCollectWithLogsChild,

This should be marked MOZ_FINAL, and then you don't need the virtual dtor.

@@ +206,5 @@
>  }
>  
> +// IPC sender for remote GC/CC logging.
> +class CycleCollectWithLogsChild : public PCycleCollectWithLogsChild,
> +                                  public nsICycleCollectorLogSink

Nit: Our style guide now apparently wants each base class on its own line if there are more than one.

@@ +214,5 @@
> +
> +    CycleCollectWithLogsChild(const FileDescriptor& aGCLog,
> +                              const FileDescriptor& aCCLog)
> +    {
> +        MOZ_COUNT_CTOR(CycleCollectWithLogsChild);

Any classes that use the isupports macros don't need to do manual ctor/dtor logging.

@@ +219,5 @@
> +        mGCLog = PlatformHandleToFILE(aGCLog.PlatformHandle(), "w");
> +        mCCLog = PlatformHandleToFILE(aCCLog.PlatformHandle(), "w");
> +    }
> +
> +    virtual ~CycleCollectWithLogsChild()

Since this class is refcounted please make this private.

@@ +221,5 @@
> +    }
> +
> +    virtual ~CycleCollectWithLogsChild()
> +    {
> +        if (mGCLog != nullptr) {

We don't normally test |!= nullptr|, just test |mGCLog|. Below too.

@@ +234,5 @@
> +        unused << Send__delete__(this);
> +        MOZ_COUNT_DTOR(CycleCollectWithLogsChild);
> +    }
> +
> +    NS_IMETHOD Open(FILE** aGCLog, FILE** aCCLog)

Please add MOZ_OVERRIDE to all your overrides.

@@ +246,5 @@
> +    }
> +
> +    NS_IMETHOD CloseGCLog()
> +    {
> +        fclose(mGCLog);

I'd first assert mGCLog is non-null. for CloseCCLog too

@@ +248,5 @@
> +    NS_IMETHOD CloseGCLog()
> +    {
> +        fclose(mGCLog);
> +        mGCLog = nullptr;
> +        return SendCloseGCLog() ? NS_OK : NS_ERROR_FAILURE;

One fun thing about child->parent messages is that they can never fail. If they do the child is aborted :) So no need to check here or below for success, you can always return NS_OK.

@@ +776,5 @@
>  bool
> +ContentChild::DeallocPCycleCollectWithLogsChild(PCycleCollectWithLogsChild* aActor)
> +{
> +    // ...so there's nothing to do here, because the actor is already being
> +    // destroyed.

This comment might need to be a little more scary... Say something like aActor is in its destructor at the moment and must not be touched? Oh, and comment out the |aActor| arg name so it's not possible to touch it :)

@@ +789,4 @@
>  {
>      nsCOMPtr<nsIMemoryInfoDumper> dumper = do_GetService("@mozilla.org/memory-info-dumper;1");
>  
> +    dumper->DumpGCAndCCLogsToSink(aDumpAllTraces, static_cast<CycleCollectWithLogsChild*>(aActor));

It's not safe to pass XPCOM objects around with a 0 refcnt (the first QI or nsCOMPtr that touches it could accidentally delete it).

Instead assign to a stack refptr in this method, then call DumpGCAndCCLogsToSink() when the refcount is 1. DumpGCAndCCLogsToSink will addref, your stack refptr will go away, and all will be safe.

::: dom/ipc/ContentParent.cpp
@@ +341,5 @@
>      MOZ_COUNT_DTOR(MemoryReportRequestParent);
>  }
>  
> +// IPC receiver for remote GC/CC logging.
> +class CycleCollectWithLogsParent : public PCycleCollectWithLogsParent

This should be marked MOZ_FINAL, and then you don't need the virtual dtor.

@@ +359,5 @@
> +
> +    bool DoSendConstructor(ContentParent *aManager, bool aDumpAllTraces)
> +    {
> +        FILE* gcLog = nullptr;
> +        FILE* ccLog = nullptr;

Pre-nulling these isn't necessary since you're checking Open() for failure.

@@ +364,5 @@
> +        nsresult rv;
> +
> +        rv = mSink->Open(&gcLog, &ccLog);
> +        if (NS_WARN_IF(NS_FAILED(rv))) {
> +            delete this;

This is kinda weird, see below in ContentParent::CycleCollectWithLogs.

@@ +375,5 @@
> +                                                 FILEToPlatformHandle(gcLog),
> +                                                 FILEToPlatformHandle(ccLog));
> +    }
> +
> +    virtual bool RecvCloseGCLog()

Please add MOZ_OVERRIDE to all your overrides.

@@ +399,5 @@
> +        return true;
> +    }
> +
> +    // Ignoring ActorDestroy; there are no parent->child messages in
> +    // this protocol, so no need to avoid sending them afterwards.

This isn't all that ActorDestroy helps with :) Recv__delete__ is only called when the child successfully sends a __delete__ message. If the child crashes you'll never call mSink->Close[G|C]CLog(). 

Also Josh just landed a patch that makes ActorDestroy required for all parent-side actors :)

@@ +2677,5 @@
> +                                                const FileDescriptor& aGCLog,
> +                                                const FileDescriptor& aCCLog)
> +{
> +    MOZ_CRASH("Don't call this; allocate a CycleCollectWithLogsParent directly.");
> +    return nullptr;

This return shouldn't be needed.

@@ +2693,5 @@
> +                                    nsICycleCollectorLogSink* aSink,
> +                                    nsIDumpGCAndCCLogsCallback* aCallback)
> +{
> +    CycleCollectWithLogsParent *actor = new CycleCollectWithLogsParent(aSink, aCallback);
> +    return actor->DoSendConstructor(this, aDumpAllTraces);

I'd kinda prefer a static CycleCollectWithLogsParent::SendConstructor() that dealt with actual instances under the hood and never exposed them.

The current code means that if DoSendConstructor fails the |actor| variable now points to garbage, and that could cause problems down the road.

::: dom/ipc/PCycleCollectWithLogs.ipdl
@@ +12,5 @@
> +    manager PContent;
> +
> +parent:
> +    CloseGCLog();
> +    CloseCCLog();

These might be unnecessary... Why not just handle both closes in the parent's ActorDestroy function?

::: ipc/glue/FileDescriptorUtils.cpp
@@ +84,5 @@
> +namespace mozilla {
> +namespace ipc {
> +
> +FILE*
> +PlatformHandleToFILE(FileDescriptor::PlatformHandleType aHandle,

We need to handle the case where aHandle.IsValid() is false (i.e. return null if so).

@@ +89,5 @@
> +                     const char* aOpenMode)
> +{
> +#ifdef XP_WIN
> +  int fd = _open_osfhandle(reinterpret_cast<intptr_t>(aHandle), 0);
> +  if (fd < 0) {

The docs say that if _open_osfhandle fails it will return -1 only, so let's test against that explicitly.

Also, let's add a warning like so: |if (NS_WARN_IF(fd == -1)) {|

@@ +92,5 @@
> +  int fd = _open_osfhandle(reinterpret_cast<intptr_t>(aHandle), 0);
> +  if (fd < 0) {
> +    return nullptr;
> +  }
> +  return _fdopen(fd, aOpenMode);

Hrm, the docs on the return value for this function (http://msdn.microsoft.com/en-us/library/dye30d82.aspx) say we might need to worry about the invalid parameter function (http://msdn.microsoft.com/en-us/library/ksazx244.aspx). We can handle failure gracefully here so it might be worth it. What do you think?

@@ +94,5 @@
> +    return nullptr;
> +  }
> +  return _fdopen(fd, aOpenMode);
> +#else
> +  return fdopen(aHandle, aOpenMode);

This does work with the sandbox? What happens if for some reason the parent handed out a readonly fd?

@@ +99,5 @@
> +#endif
> +}
> +
> +FileDescriptor::PlatformHandleType
> +FILEToPlatformHandle(FILE* aStream)

We should probably handle null here, return INVALID_HANDLE_VALUE on windows and -1 elsewhere.

@@ +102,5 @@
> +FileDescriptor::PlatformHandleType
> +FILEToPlatformHandle(FILE* aStream)
> +{
> +#ifdef XP_WIN
> +  int fd = _fileno(aStream);

This also has some funny behavior, see the return value section of http://msdn.microsoft.com/en-us/library/zs6wbdhx.aspx

@@ +103,5 @@
> +FILEToPlatformHandle(FILE* aStream)
> +{
> +#ifdef XP_WIN
> +  int fd = _fileno(aStream);
> +  if (fd < 0) {

NS_WARN_IF(fd == -1) like above.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ -989,5 @@
>  {
>    js::DumpHeapComplete(Runtime(), file, js::CollectNurseryBeforeDump);
>  }
>  
> -

Nit: This change isn't worth the blame, I would revert it.

::: xpcom/base/nsCycleCollector.cpp
@@ +1380,5 @@
> +public:
> +    NS_DECL_ISUPPORTS
> +
> +    nsCycleCollectorLogSinkToFile() :
> +        mProcessIdentifier(base::GetCurrentProcId()),

Nit: Our style guide says

  Foo()
    : mBar()
    , mBaz()
  {
  }

@@ +1385,5 @@
> +        mGCLog("gc-edges"), mCCLog("cc-edges")
> +    {
> +    }
> +
> +    NS_IMETHOD GetFilenameIdentifier(nsAString& aIdentifier)

Please add MOZ_OVERRIDE to your overrides.

@@ +1411,5 @@
> +    }
> +
> +    NS_IMETHOD GetGcLog(nsIFile** aPath)
> +    {
> +        NS_ADDREF(*aPath = mGCLog.mFile);

I think you want NS_IF_ADDREF here and in GetCcLog, this will crash if it is called when mFile is null (e.g. before someone calls Open()).

@@ +1421,5 @@
> +        NS_ADDREF(*aPath = mCCLog.mFile);
> +        return NS_OK;
> +    }
> +
> +    NS_IMETHOD Open(FILE** aGCLog, FILE** aCCLog)

I think you should return NS_ERROR_UNEXPECTED if open has been called already (like you do for the Close methods below).

@@ +1454,5 @@
> +        CloseLog(&mCCLog, NS_LITERAL_STRING("Cycle"));
> +        return NS_OK;
> +    }
> +
> +    ~nsCycleCollectorLogSinkToFile() {

Since this class is refcounted please make this private.

Also, { goes on its own line.

@@ +1592,4 @@
>  class nsCycleCollectorLogger MOZ_FINAL : public nsICycleCollectorListener
>  {
>  public:
>      nsCycleCollectorLogger() :

You're not initializing mCCLog to null

@@ +1592,5 @@
>  class nsCycleCollectorLogger MOZ_FINAL : public nsICycleCollectorListener
>  {
>  public:
>      nsCycleCollectorLogger() :
> +      mLogSink(new nsCycleCollectorLogSinkToFile),

Please add () to the new call... Or actually, couldn't you just make this call nsCycleCollector_createLogSink()?

@@ +1657,2 @@
>      {
> +        mLogSink = aLogSink;

Either you need to protect against null here or you need to change your GetLogSink method to use NS_IF_ADDREF.

@@ +1677,2 @@
>          if (data && data->mRuntime)
> +            data->mRuntime->DumpJSHeap(gcLog);

I know this isn't your fault but please add braces.

@@ +3907,5 @@
>  
> +nsICycleCollectorLogSink*
> +nsCycleCollector_createLogSink()
> +{
> +    return new nsCycleCollectorLogSinkToFile;

Please add ()

::: xpcom/base/nsCycleCollector.h
@@ +40,5 @@
>  
>  void nsCycleCollector_dispatchDeferredDeletion(bool aContinuation = false);
>  bool nsCycleCollector_doDeferredDeletion();
>  
> +nsICycleCollectorLogSink* nsCycleCollector_createLogSink();

This should be already_AddRefed<nsICycleCollectorLogSink> so that callers can't leak it.

::: xpcom/base/nsICycleCollectorListener.idl
@@ +4,5 @@
>  
>  #include "nsISupports.idl"
>  
> +%{C++
> +#include <stdio.h>

Just a heads up, sometimes including windows headers #define's a bunch of crap that causes later compilation problems.

::: xpcom/base/nsIMemoryInfoDumper.idl
@@ +6,5 @@
>  #include "nsISupports.idl"
>  
> +interface nsIFile;
> +interface nsICycleCollectorLogSink;
> +interface nsIDumpGCAndCCLogsCallback;

Nit: shouldn't need to forward-declare this one.

@@ +16,5 @@
>  };
>  
> +/**
> + * Callback interface for |dumpGCAndCCLogsToFile|, below.  Note that
> + * some of these method calls may occur synchronously.

Nit: Hm, this 'synchronously' part doesn't make much sense to me... These are simple methods so they are by definition 'synchronous'.

@@ +28,5 @@
> +   * exhaustion) are not reported.
> +   *
> +   * @param aGCLogPath The full path of the file that the GC log was written to.
> +   *
> +   * @param aCCLogPath The full path of the file that the CC log was written to.

Nit: These are nsIFiles, not paths.

::: xpcom/base/nsMemoryInfoDumper.cpp
@@ +60,5 @@
>    const nsString mIdentifier;
>    const bool mMinimizeMemoryUsage;
>  };
>  
> +class GCAndCCLogDumpRunnable : public nsRunnable, public nsIDumpGCAndCCLogsCallback

Since you're here you should make this MOZ_FINAL, and then each base goes on its own line

@@ +99,5 @@
>    const bool mDumpAllTraces;
>    const bool mDumpChildProcesses;
>  };
>  
> +NS_IMPL_ISUPPORTS_INHERITED1(GCAndCCLogDumpRunnable, nsRunnable, nsIDumpGCAndCCLogsCallback)

FYI the ISUPPORTS macros no longer have a number suffix

@@ +242,5 @@
>  
> +// Use XPCOM refcounting to fire |onFinish| when all reference-holders
> +// (remote dump actors or the |DumpGCAndCCLogsToFile| activation itself)
> +// have gone away.
> +class nsDumpGCAndCCLogsCallbackHolder : public nsIDumpGCAndCCLogsCallback

This should be marked MOZ_FINAL, and then you don't need the virtual dtor.

@@ +262,5 @@
> +  {
> +    return mCallback->OnDump(aGCLog, aCCLog, aIsParent);
> +  }
> +
> +  virtual ~nsDumpGCAndCCLogsCallbackHolder()

Since this is refcounted please make this private.

@@ +271,5 @@
> +private:
> +  nsCOMPtr<nsIDumpGCAndCCLogsCallback> mCallback;
> +};
> +
> +NS_IMPL_ISUPPORTS1(nsDumpGCAndCCLogsCallbackHolder, nsIDumpGCAndCCLogsCallback)

FYI the ISUPPORTS macros no longer have number suffixes

        Attachment #8413088 -
        Flags: review?(bent.mozilla) → review-

    
    

    
  
A few things that needed nontrivial comments or questions:

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #36)
> > +    // Ignoring ActorDestroy; there are no parent->child messages in
> > +    // this protocol, so no need to avoid sending them afterwards.
> 
> This isn't all that ActorDestroy helps with :) Recv__delete__ is only called
> when the child successfully sends a __delete__ message. If the child crashes
> you'll never call mSink->Close[G|C]CLog(). 

This is deliberate: if the log isn't finished, we should skip the Close* methods and just fclose the files, so the log keeps its "incomplete-" prefix.  And I *thought* I had a comment explaining that somewhere, but apparently not.

> ::: dom/ipc/PCycleCollectWithLogs.ipdl
> @@ +12,5 @@
> > +    manager PContent;
> > +
> > +parent:
> > +    CloseGCLog();
> > +    CloseCCLog();
> 
> These might be unnecessary... Why not just handle both closes in the
> parent's ActorDestroy function?

It's cosmetic-ish — the current code closes the GC log before starting the CC, so the script that watches the directory can see that it's done at that point.  The last time I tried to IPCify this I just closed them both at the end of the CC, and nobody had a problem with it then, but I figured I might as well try to keep the current behavior as much as reasonable.

I suppose I could also bake the GC-then-CC sequence into the protocol — have the child send a sync message that closes the GC log and returns the CC log FileDescriptor, or something like that, but I don't know that that's an improvement (or worth the effort, at this point).

> @@ +92,5 @@
> > +  int fd = _open_osfhandle(reinterpret_cast<intptr_t>(aHandle), 0);
> > +  if (fd < 0) {
> > +    return nullptr;
> > +  }
> > +  return _fdopen(fd, aOpenMode);
> 
> Hrm, the docs on the return value for this function
> (http://msdn.microsoft.com/en-us/library/dye30d82.aspx) say we might need to
> worry about the invalid parameter function
> (http://msdn.microsoft.com/en-us/library/ksazx244.aspx). We can handle
> failure gracefully here so it might be worth it. What do you think?

Do we generally do that for Windows API calls that could invoke error handlers like that?

> @@ +94,5 @@
> > +    return nullptr;
> > +  }
> > +  return _fdopen(fd, aOpenMode);
> > +#else
> > +  return fdopen(aHandle, aOpenMode);
> 
> This does work with the sandbox? What happens if for some reason the parent
> handed out a readonly fd?

fdopen is mostly userspace; it might use fcntl to read the file flags (or possibly change them; e.g., glibc sets O_APPEND if the mode is "a"), but that's allowed and there aren't plans to disallow it.

If it's a read-only fd and the mode is "w": it will return NULL and set errno to EINVAL.

…and Windows _fdopen could also do that, in which case we still need to clean up the osfhandle, don't we.

> ::: xpcom/base/CycleCollectedJSRuntime.cpp
> @@ -989,5 @@
> >  {
> >    js::DumpHeapComplete(Runtime(), file, js::CollectNurseryBeforeDump);
> >  }
> >  
> > -
> 
> Nit: This change isn't worth the blame, I would revert it.

Sigh.  There's always one of those that sneaks in around a change that gets undone.

> ::: xpcom/base/nsICycleCollectorListener.idl
> @@ +4,5 @@
> >  
> >  #include "nsISupports.idl"
> >  
> > +%{C++
> > +#include <stdio.h>
> 
> Just a heads up, sometimes including windows headers #define's a bunch of
> crap that causes later compilation problems.

nsIFile.idl already does that include, so it's not *too* hazardous.

Also, https://tbpl.mozilla.org/?tree=Try&rev=c0dfe122cb6d

> @@ +16,5 @@
> >  };
> >  
> > +/**
> > + * Callback interface for |dumpGCAndCCLogsToFile|, below.  Note that
> > + * some of these method calls may occur synchronously.
> 
> Nit: Hm, this 'synchronously' part doesn't make much sense to me... These
> are simple methods so they are by definition 'synchronous'.

Synchronously with respect to the call to dumpGCAndCCLogsToFile; i.e., the callback methods can be called before dumpGCAndCCLogsToFile returns, so don't write code as though that won't happen.  But what I actually wrote there is, on reread, incomprehensible; I'll reword it.

    
    

    
  
(In reply to Jed Davis [:jld] from comment #37)
> > > +  return _fdopen(fd, aOpenMode);
> > 
> > Hrm, the docs on the return value for this function
> > (http://msdn.microsoft.com/en-us/library/dye30d82.aspx) say we might need to
> > worry about the invalid parameter function
> > (http://msdn.microsoft.com/en-us/library/ksazx244.aspx). We can handle
> > failure gracefully here so it might be worth it. What do you think?
> 
> Do we generally do that for Windows API calls that could invoke error
> handlers like that?

It looks like we don't — _set_invalid_parameter_handler is used in Breakpad to get crash dumps for it, but it doesn't look like there's a way to exempt certain call sites.  (Assuming I understood the question correctly.)

    
    

    
  
(In reply to Jed Davis [:jld] from comment #37)
> …and Windows _fdopen could also [fail], in which case we still need to
> clean up the osfhandle, don't we.

Except that we kind of can't.  If we close() it, that will CloseHandle the original handle, which isn't what happens if Unix fdopen fails.  If we don't close() it, we leak the fd on error because the caller never sees it.

But also, I'm leaking the fd and/or handle if anything in PlatformHandleToFILE fails.

So I think it makes the most sense to have PlatformHandleToFILE close the handle on failure.

    
    

    
  
(In reply to Jed Davis [:jld] from comment #38)
> It looks like we don't — _set_invalid_parameter_handler is used in Breakpad
> to get crash dumps for it, but it doesn't look like there's a way to exempt
> certain call sites.  (Assuming I understood the question correctly.)

Well, I was thinking more along the lines of http://mxr.mozilla.org/mozilla-central/source/js/src/prmjtime.cpp#368 where the old handler is stashed temporarily to allow non-fatal failure.

I'm not convinced we need to do it though.

    
    

    
  
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #40)
> (In reply to Jed Davis [:jld] from comment #38)
> > It looks like we don't — _set_invalid_parameter_handler is used in Breakpad
> > to get crash dumps for it, but it doesn't look like there's a way to exempt
> > certain call sites.  (Assuming I understood the question correctly.)
> 
> Well, I was thinking more along the lines of
> http://mxr.mozilla.org/mozilla-central/source/js/src/prmjtime.cpp#368 where
> the old handler is stashed temporarily to allow non-fatal failure.

I see.  But isn't that thread-unsafe, since _set_invalid_parameter_handler applies to the entire process?

    
    

    
  


  

        Attachment #8413088 -
        Attachment is obsolete: true

        Attachment #8419832 -
        Flags: review?(bent.mozilla)

    
    

    
  
(In reply to Jed Davis [:jld] from comment #41)
> I see.  But isn't that thread-unsafe, since _set_invalid_parameter_handler
> applies to the entire process?

It certainly looks that way :-/

Let's punt on this for now. The JS engine will need to be fixed I guess.

    
    

    
  
Comment on attachment 8419832 [details] [diff] [review]
Remote GC/CC logging, revised after comment #36

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

I think this looks great!

::: dom/ipc/ContentChild.cpp
@@ +250,5 @@
> +    }
> +
> +    NS_IMETHOD GetFilenameIdentifier(nsAString& aIdentifier) MOZ_OVERRIDE
> +    {
> +        return NS_ERROR_UNEXPECTED;

These might all be better with a MOZ_ASSERT(false, "Don't call me!");

@@ +280,5 @@
> +    }
> +
> +private:
> +    FILE* mGCLog;
> +    FILE* mCCLog;

Nit: members after methods.

@@ +292,5 @@
> +        if (mCCLog) {
> +            fclose(mCCLog);
> +            mCCLog = nullptr;
> +        }
> +        // The XPCOM refcount drives the IPC lifecycle; see also 

Nit: trailing whitespace

::: dom/ipc/ContentParent.cpp
@@ +382,5 @@
> +                                                 FILEToFileDescriptor(gcLog),
> +                                                 FILEToFileDescriptor(ccLog));
> +    }
> +
> +    virtual bool RecvCloseGCLog() MOZ_OVERRIDE

Nit: All your IPDL methods can be private.

@@ +415,5 @@
> +    }
> +
> +private:
> +    nsCOMPtr<nsICycleCollectorLogSink> mSink;
> +    nsCOMPtr<nsIDumpGCAndCCLogsCallback> mCallback;

Nit: Normally members come after all methods

::: ipc/glue/FileDescriptorUtils.cpp
@@ +104,5 @@
> +  }
> +#else
> +  int fd = handle;
> +#endif
> +  FILE *file = fdopen(fd, aOpenMode);

Nit: * on the left

::: ipc/glue/FileDescriptorUtils.h
@@ +45,5 @@
>  
> +// On failure, FileDescriptorToFILE closes the given descriptor; on
> +// success, fclose()ing the returned FILE* will close the handle.
> +// This is meant for use with FileDescriptors received over IPC.
> +FILE* FileDescriptorToFILE(const FileDescriptor& aDesc,

This probably shouldn't take a const FileDescriptor, right? It's going to be modified somehow.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ -989,5 @@
>  {
>    js::DumpHeapComplete(Runtime(), file, js::CollectNurseryBeforeDump);
>  }
>  
> -

Nit: still here.

::: xpcom/base/nsCycleCollector.cpp
@@ +1376,5 @@
>      : mAddress("0x"), mCnt(0), mType(eUnknown) {}
>  
>    enum Type
> +    {
> +      eRefCountedObject,

Nit: This change looks unrelated?

@@ +1696,5 @@
>      if (mDisableLog) {
>        return NS_OK;
>      }
>  
> +    FILE *gcLog;

Nit: * on the left.

        Attachment #8419832 -
        Flags: review?(bent.mozilla) → review+

    
    

    
  
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #44)
> ::: ipc/glue/FileDescriptorUtils.h
> @@ +45,5 @@
> >  
> > +// On failure, FileDescriptorToFILE closes the given descriptor; on
> > +// success, fclose()ing the returned FILE* will close the handle.
> > +// This is meant for use with FileDescriptors received over IPC.
> > +FILE* FileDescriptorToFILE(const FileDescriptor& aDesc,
> 
> This probably shouldn't take a const FileDescriptor, right? It's going to be
> modified somehow.

Yes and no.  I wondered about that myself, but actually const is fine — as far as the language cares, at least.  It calls this method on the FileDescriptor:

  PlatformHandleType PlatformHandle() const

which assigns to this instance variable:

  mutable DebugOnly<bool> mHandleCreatedByOtherProcessWasUsed;

whose type pretty much speaks for itself. 

> ::: xpcom/base/nsCycleCollector.cpp
> @@ +1376,5 @@
> >      : mAddress("0x"), mCnt(0), mType(eUnknown) {}
> >  
> >    enum Type
> > +    {
> > +      eRefCountedObject,
> 
> Nit: This change looks unrelated?

I assume that got in when I was trying to merge across a mass whitespace/style change (bug 995730).  Fixed; thanks.
Depends on: 1009289

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug973090-take2-hg4.diff (obsolete)
        — 
          — Splinter Review
      

    


  
Carrying over r+.

Trying: https://tbpl.mozilla.org/?tree=Try&rev=5d589c782656

        Attachment #8419832 -
        Attachment is obsolete: true

        Attachment #8421449 -
        Flags: review+

    
    

    
  
This has conflicts with inbound. Please rebase.
Keywords: checkin-needed

    
    

    
  


  
Rebased; tested locally on B2G (keon); carrying over r+ again.

        Attachment #8421449 -
        Attachment is obsolete: true

        Attachment #8421950 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8a595b6c5750
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: