Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla11

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: mccr8)

Tracking

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → continuation
Blocks: 649532
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

6 years ago
Created attachment 566986 [details] [diff] [review]
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.
Attachment #566986 - Flags: review?(bent.mozilla)
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?
(Assignee)

Comment 4

6 years ago
Created attachment 571714 [details] [diff] [review]
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.
Attachment #566986 - Attachment is obsolete: true
Attachment #566986 - Flags: review?(bent.mozilla)
(Assignee)

Comment 5

6 years ago
Created attachment 571715 [details] [diff] [review]
add outparam to tell if listener Begin() failed
(Assignee)

Comment 6

6 years ago
Created attachment 573841 [details] [diff] [review]
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.
Attachment #571714 - Attachment is obsolete: true
Attachment #573841 - Flags: review?(bent.mozilla)
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!
Attachment #573841 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 8

6 years ago
(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
Target Milestone: --- → mozilla11
(Assignee)

Comment 9

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

># HG changeset patch
># User Andrew McCreight <amccreight@mozilla.com>
># 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;
Attachment #571715 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Oops, I didn't mean to dump the patch into the bug.
https://hg.mozilla.org/mozilla-central/rev/1aa115c24887
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

6 years ago
Created attachment 580956 [details] [diff] [review]
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.
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 710761
You need to log in before you can comment on or make changes to this bug.