Bug 1158558 - Part 2: Finish incremental GCs in progress in BeginCollection() and ShutdownCollect(). r=smaug, a=sledru
authorAndrew McCreight <continuation@gmail.com>
Fri, 15 May 2015 10:33:09 -0700
changeset 274766 feaee5986ce1665358b2ab96372c214e6f1d4b03
parent 274765 daf478785c62acb93c126d1fe111451b5d14ca1a
child 274767 c49d7de6e59aabab6b246a7a2bee0529b9d703c8
push id863
push userraliiev@mozilla.com
push dateMon, 03 Aug 2015 13:22:43 +0000
treeherdermozilla-release@f6321b14228d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, sledru
bugs1158558
milestone40.0a2
Bug 1158558 - Part 2: Finish incremental GCs in progress in BeginCollection() and ShutdownCollect(). r=smaug, a=sledru Various parts of the first half of BeginCollection() can start an incremental GC. This is bad because running the GC and CC at the same time can cause the CC to end up with pointers to dead JS objects. To avoid this, we finish any incremental GC in progress in BeginCollection. This is slow, but hopefully it is rare.
js/xpconnect/tests/mochitest/mochitest.ini
js/xpconnect/tests/mochitest/test_bug1158558.html
xpcom/base/nsCycleCollector.cpp
--- a/js/xpconnect/tests/mochitest/mochitest.ini
+++ b/js/xpconnect/tests/mochitest/mochitest.ini
@@ -91,16 +91,17 @@ skip-if= buildapp == 'mulet'
 [test_bug940783.html]
 [test_bug965082.html]
 [test_bug960820.html]
 [test_bug986542.html]
 [test_bug993423.html]
 [test_bug1005806.html]
 [test_bug1021258.html]
 [test_bug1094930.html]
+[test_bug1158558.html]
 [test_crossOriginObjects.html]
 [test_crosscompartment_weakmap.html]
 [test_frameWrapping.html]
 # The JS test component we use below is only available in debug builds.
 [test_getWebIDLCaller.html]
 skip-if = (debug == false || os == "android")
 [test_nac.xhtml]
 [test_sameOriginPolicy.html]
new file mode 100644
--- /dev/null
+++ b/js/xpconnect/tests/mochitest/test_bug1158558.html
@@ -0,0 +1,47 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1158558
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1158558</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1158558">Mozilla Bug 1158558</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <input id="ipt"></input>
+</div>
+<pre id="test">
+</pre>
+<script type="application/javascript">
+
+/** Test for Bug 1158558 **/
+
+// Observers of cycle-collector-begin can be implemented in JS, and
+// thus can end up starting an incremental GC while we're in the middle
+// of a CC slice.
+
+SimpleTest.waitForExplicitFinish();
+
+var observer = {
+  observe: function(subject, topic, data) {
+    SpecialPowers.removeObserver(observer, "cycle-collector-begin");
+    SpecialPowers.Cu.getJSTestingFunctions().startgc(1);
+
+    ok(true, "Do something so the test harness doesn't get angry");
+
+    SimpleTest.finish();
+  }
+};
+
+SpecialPowers.addObserver(observer, "cycle-collector-begin", false);
+
+SpecialPowers.Cu.forceCC();
+
+</script>
+</body>
+</html>
--- a/xpcom/base/nsCycleCollector.cpp
+++ b/xpcom/base/nsCycleCollector.cpp
@@ -1321,16 +1321,18 @@ public:
                            size_t* aPurpleBufferSize) const;
 
   JSPurpleBuffer* GetJSPurpleBuffer();
 private:
   void CheckThreadSafety();
   void ShutdownCollect();
 
   void FixGrayBits(bool aForceGC, TimeLog& aTimeLog);
+  bool IsIncrementalGCInProgress();
+  void FinishAnyIncrementalGCInProgress();
   bool ShouldMergeZones(ccType aCCType);
 
   void BeginCollection(ccType aCCType, nsICycleCollectorListener* aManualListener);
   void MarkRoots(SliceBudget& aBudget);
   void ScanRoots(bool aFullySynchGraphBuild);
   void ScanIncrementalRoots();
   void ScanWhiteNodes(bool aFullySynchGraphBuild);
   void ScanBlackNodes();
@@ -3505,16 +3507,32 @@ nsCycleCollector::FixGrayBits(bool aForc
     mResults.mForcedGC = true;
   }
 
   mJSRuntime->GarbageCollect(aForceGC ? JS::gcreason::SHUTDOWN_CC :
                                         JS::gcreason::CC_FORCED);
   aTimeLog.Checkpoint("FixGrayBits GC");
 }
 
+bool
+nsCycleCollector::IsIncrementalGCInProgress()
+{
+  return mJSRuntime && JS::IsIncrementalGCInProgress(mJSRuntime->Runtime());
+}
+
+void
+nsCycleCollector::FinishAnyIncrementalGCInProgress()
+{
+  if (IsIncrementalGCInProgress()) {
+    NS_WARNING("Finishing incremental GC in progress during CC");
+    JS::PrepareForIncrementalGC(mJSRuntime->Runtime());
+    JS::FinishIncrementalGC(mJSRuntime->Runtime(), JS::gcreason::CC_FORCED);
+  }
+}
+
 void
 nsCycleCollector::CleanupAfterCollection()
 {
   TimeLog timeLog;
   MOZ_ASSERT(mIncrementalPhase == CleanupPhase);
   mGraph.Clear();
   timeLog.Checkpoint("CleanupAfterCollection::mGraph.Clear()");
 
@@ -3545,16 +3563,18 @@ nsCycleCollector::CleanupAfterCollection
     timeLog.Checkpoint("CleanupAfterCollection::EndCycleCollectionCallback()");
   }
   mIncrementalPhase = IdlePhase;
 }
 
 void
 nsCycleCollector::ShutdownCollect()
 {
+  FinishAnyIncrementalGCInProgress();
+
   SliceBudget unlimitedBudget;
   uint32_t i;
   for (i = 0; i < DEFAULT_SHUTDOWN_COLLECTIONS; ++i) {
     if (!Collect(ShutdownCC, unlimitedBudget, nullptr)) {
       break;
     }
   }
   NS_WARN_IF_FALSE(i < NORMAL_SHUTDOWN_COLLECTIONS, "Extra shutdown CC");
@@ -3578,16 +3598,18 @@ nsCycleCollector::Collect(ccType aCCType
   CheckThreadSafety();
 
   // This can legitimately happen in a few cases. See bug 383651.
   if (mActivelyCollecting || mFreeingSnowWhite) {
     return false;
   }
   mActivelyCollecting = true;
 
+  MOZ_ASSERT(!IsIncrementalGCInProgress());
+
   bool startedIdle = (mIncrementalPhase == IdlePhase);
   bool collectedAny = false;
 
   // If the CC started idle, it will call BeginCollection, which
   // will do FreeSnowWhite, so it doesn't need to be done here.
   if (!startedIdle) {
     TimeLog timeLog;
     FreeSnowWhite(true);
@@ -3763,25 +3785,36 @@ nsCycleCollector::BeginCollection(ccType
   }
 
   bool forceGC = isShutdown;
   if (!forceGC && mListener) {
     // On a WantAllTraces CC, force a synchronous global GC to prevent
     // hijinks from ForgetSkippable and compartmental GCs.
     mListener->GetWantAllTraces(&forceGC);
   }
+
+  // BeginCycleCollectionCallback() might have started an IGC, and we need
+  // to finish it before we run FixGrayBits.
+  FinishAnyIncrementalGCInProgress();
+  timeLog.Checkpoint("Pre-FixGrayBits finish IGC");
+
   FixGrayBits(forceGC, timeLog);
 
   FreeSnowWhite(true);
   timeLog.Checkpoint("BeginCollection FreeSnowWhite");
 
   if (mListener && NS_FAILED(mListener->Begin())) {
     mListener = nullptr;
   }
 
+  // FreeSnowWhite could potentially have started an IGC, which we need
+  // to finish before we look at any JS roots.
+  FinishAnyIncrementalGCInProgress();
+  timeLog.Checkpoint("Post-FreeSnowWhite finish IGC");
+
   // Set up the data structures for building the graph.
   mGraph.Init();
   mResults.Init();
   bool mergeZones = ShouldMergeZones(aCCType);
   mResults.mMergedZones = mergeZones;
 
   MOZ_ASSERT(!mBuilder, "Forgot to clear mBuilder");
   mBuilder = new CCGraphBuilder(mGraph, mResults, mJSRuntime, mListener,