694436 - Crash when dumping CC heap dump if Firefox can't write to the cwd

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Joe Drew (not getting mail)]

Launch Firefox from the terminal from a place you can't write to; for example, /. Paste the CC heap dump JS into your error console (instructions at https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump). You'll crash immediately, likely because fopen() returned NULL.

Comment 1

[Andrew McCreight [:mccr8]]

This was the crash log: https://crash-stats.mozilla.com/report/index/bp-0a05630b-eea8-41cd-8bb5-71eea2111013

Oops, I see the problem.  This is fallout from my CC dumping refactoring patch, I think.  I added aListener as an argument to FinishCollection().  If fopen fails, aListener is set to null, but this change is local to BeginCollection().  We then return from BeginCollection, then call FinishCollection with the old non-null listener value, so FinishCollection tries to write to null.  Should be easy to fix.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: pass listener by reference so we don't forget that fopen failed

Thanks for finding this, Joe.  Sorry I haven't been able to figure out your actual problem yet.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 566986 [details] [diff] [review]
pass listener by reference so we don't forget that fopen failed

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

::: xpcom/base/nsCycleCollector.cpp
@@ +1101,5 @@
>      void GCIfNeeded(bool aForceGC);
>      void CleanupAfterCollection();
>  
>      // Start and finish an individual collection.
> +    bool BeginCollection(nsICycleCollectorListener *&aListener);

Hm... We rarely use pointer refs in mozilla code, and I don't particularly like it. Can we do something like add a bool* out-param here that indicates the same condition?

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: lift listener Begin() out of BeginCollection

Yeah, it is pretty gross, now that you mention it.  I was too focused on making a small change.

The problem with an out-param is that we have to check it after a call to BeginCollection, and it will probably have to be bubbled out further.  Another approach would be to hoist out the listener initialization.  This results in a tiny amount of code duplication, and looks weird because the listener Begin is not in BeginCollection, but avoids backwash.

I'll upload patches for both ways.  I haven't tested these yet, so I'm not requesting review yet.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: add outparam to tell if listener Begin() failed

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: lift listener Begin() out of BeginCollection

Slightly cleaner version of this.  Lift the listener begin out of BeginCollection, causing a tiny amount of code duplication, but greatly simplifying the dataflow.  I confirmed that Firefox doesn't crash when opening a file fails.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 573841 [details] [diff] [review]
lift listener Begin() out of BeginCollection

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

Sorry for the delay here. Looks good!

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to ben turner [:bent] from comment #7)
> Sorry for the delay here. Looks good!

No problem!

https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa115c24887

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 571715 [details] [diff] [review]
add outparam to tell if listener Begin() failed

># HG changeset patch
># User Andrew McCreight <[e-mail]>
># Date 1320344255 25200
># Node ID addac4e2a1e9bf0b58bb98e70279f0975cdaf1ae
># Parent  3c684b4b8f6805dcd81f345f0c219406503a0fc0
>[mq]: DumpCrash2
>
>diff --git a/xpcom/base/nsCycleCollector.cpp b/xpcom/base/nsCycleCollector.cpp
>--- a/xpcom/base/nsCycleCollector.cpp
>+++ b/xpcom/base/nsCycleCollector.cpp
>@@ -1101,17 +1101,18 @@ struct nsCycleCollector
>                      nsICycleCollectorListener *aListener);
> 
>     // Prepare for and cleanup after one or more collection(s).
>     bool PrepareForCollection(nsTArray<PtrInfo*> *aWhiteNodes);
>     void GCIfNeeded(bool aForceGC);
>     void CleanupAfterCollection();
> 
>     // Start and finish an individual collection.
>-    bool BeginCollection(nsICycleCollectorListener *aListener);
>+    bool BeginCollection(nsICycleCollectorListener *aListener,
>+                         bool& listenerFailed);
>     bool FinishCollection(nsICycleCollectorListener *aListener);
> 
>     PRUint32 SuspectedCount();
>     void Shutdown();
> 
>     void ClearGraph()
>     {
>         mGraph.mNodes.Clear();
>@@ -2694,36 +2695,43 @@ nsCycleCollector::Collect(PRUint32 aTryC
> 
>     if (!PrepareForCollection(&whiteNodes))
>         return 0;
> 
>     PRUint32 totalCollections = 0;
>     while (aTryCollections > totalCollections) {
>         // Synchronous cycle collection. Always force a JS GC beforehand.
>         GCIfNeeded(true);
>-        if (!(BeginCollection(aListener) &&
>-              FinishCollection(aListener)))
>+
>+        bool listenerFailed = false;
>+        if (!BeginCollection(aListener, listenerFailed))
>+            break;
>+        if (listenerFailed)
>+            aListener = nsnull;
>+        if (!FinishCollection(aListener))
>             break;
> 
>         ++totalCollections;
>     }
> 
>     CleanupAfterCollection();
> 
>     return mCollectedObjects;
> }
> 
> bool
>-nsCycleCollector::BeginCollection(nsICycleCollectorListener *aListener)
>+nsCycleCollector::BeginCollection(nsICycleCollectorListener *aListener,
>+                                  bool &aListenerFailed)
> {
>     if (mParams.mDoNothing)
>         return false;
> 
>     if (aListener && NS_FAILED(aListener->Begin())) {
>         aListener = nsnull;
>+        aListenerFailed = true;
>     }
> 
>     GCGraphBuilder builder(mGraph, mRuntimes, aListener);
>     if (!builder.Initialized())
>         return false;
> 
> #ifdef COLLECT_TIME_DEBUG
>     PRTime now = PR_Now();
>@@ -3489,17 +3497,20 @@ public:
>         while (1) {
>             mRequest.Wait();
> 
>             if (!mRunning) {
>                 mReply.Notify();
>                 return NS_OK;
>             }
> 
>-            mCollected = mCollector->BeginCollection(mListener);
>+            bool listenerFailed = false;
>+            mCollected = mCollector->BeginCollection(mListener, listenerFailed);
>+            if (listenerFailed)
>+                mListener = nsnull;
> 
>             mReply.Notify();
>         }
> 
>         return NS_OK;
>     }
> 
>     nsCycleCollectorRunner(nsCycleCollector *collector)
>@@ -3531,16 +3542,17 @@ public:
>             return 0;
> 
>         NS_ASSERTION(!mListener, "Should have cleared this already!");
>         mListener = aListener;
> 
>         mRequest.Notify();
>         mReply.Wait();
> 
>+        aListener = mListener;
>         mListener = nsnull;
> 
>         if (mCollected) {
>             mCollected = mCollector->FinishCollection(aListener);
> 
>             mCollector->CleanupAfterCollection();
> 
>             return mCollected ? mCollector->mCollectedObjects : 0;

Comment 10

[Andrew McCreight [:mccr8]]

Oops, I didn't mean to dump the patch into the bug.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1aa115c24887

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: test listener begin failure

I figured out how we can test this: create a dummy listener in JS that errors out in begin, and sets a flag to false if any of the other callbacks are hit.

Comment 13

[Andrew McCreight [:mccr8]]

I confirmed that this test passes on trunk, fails with the patch in this bug unapplied.  I'm tryservering it now to make sure it is okay across all platforms.