Reviewed by Maciej.
authordarin <darin@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 5 Aug 2007 10:16:41 +0000 (10:16 +0000)
committerdarin <darin@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 5 Aug 2007 10:16:41 +0000 (10:16 +0000)
        - fix <rdar://problem/5371862> crash in Dashcode due to Quartz Composer JavaScript garbage collector reentrancy

        * API/JSBase.cpp: (JSGarbageCollect): Don't call collector() if isBusy() returns true.

        * kjs/collector.h: Added isBusy(), removed the unused return value from collect()
        * kjs/collector.cpp: Added an "operation in progress" flag to the allocator.
        (KJS::Collector::allocate): Call abort() if an operation is already in progress. Set the new flag instead
        of using the debug-only GCLock.
        (KJS::Collector::collect): Ditto.
        (KJS::Collector::isBusy): Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@24874 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JavaScriptCore/API/JSBase.cpp
JavaScriptCore/ChangeLog
JavaScriptCore/kjs/collector.cpp
JavaScriptCore/kjs/collector.h

index e18498e463a6bf289e83f962f5077836e6e3e664..15b1952f9189cbdbe98e9f03c12c99f533eee0c7 100644 (file)
@@ -1,6 +1,6 @@
 // -*- mode: c++; c-basic-offset: 4 -*-
 /*
- * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
+ * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -79,5 +79,9 @@ bool JSCheckScriptSyntax(JSContextRef ctx, JSStringRef script, JSStringRef sourc
 void JSGarbageCollect(JSContextRef)
 {
     JSLock lock;
-    Collector::collect();
+    if (!Collector::isBusy())
+        Collector::collect();
+    // FIXME: Perhaps we should trigger a second mark and sweep
+    // once the garbage collector is done if this is called when
+    // the collector is busy.
 }
index 72ee3b88c2a4b284a59e48e32f44ff4cf311e180..5d7f07ca6acf4387dbf563eb0ce34532c18d939f 100644 (file)
@@ -1,3 +1,18 @@
+2007-08-04  Darin Adler  <darin@apple.com>
+
+        Reviewed by Maciej.
+
+        - fix <rdar://problem/5371862> crash in Dashcode due to Quartz Composer JavaScript garbage collector reentrancy
+
+        * API/JSBase.cpp: (JSGarbageCollect): Don't call collector() if isBusy() returns true.
+
+        * kjs/collector.h: Added isBusy(), removed the unused return value from collect()
+        * kjs/collector.cpp: Added an "operation in progress" flag to the allocator.
+        (KJS::Collector::allocate): Call abort() if an operation is already in progress. Set the new flag instead
+        of using the debug-only GCLock.
+        (KJS::Collector::collect): Ditto.
+        (KJS::Collector::isBusy): Added.
+
 2007-08-04  Maciej Stachowiak  <mjs@apple.com>
 
         Reviewed by Darin and Adam.
index aefc76594fb3ebe58e024c5eaf799f0e732f0fda..6ac45a8a719cafefe267eeae74ffdf5608ee912f 100644 (file)
@@ -1,7 +1,7 @@
 // -*- mode: c++; c-basic-offset: 4 -*-
 /*
  *  This file is part of the KDE libraries
- *  Copyright (C) 2003, 2004, 2005, 2006 Apple Computer, Inc.
+ *  Copyright (C) 2003, 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
 #include "config.h"
 #include "collector.h"
 
-#include <wtf/FastMalloc.h>
-#include <wtf/FastMallocInternal.h>
-#include <wtf/HashCountedSet.h>
-#include <wtf/UnusedParam.h>
 #include "internal.h"
 #include "list.h"
 #include "value.h"
-
-#include <setjmp.h>
 #include <algorithm>
+#include <setjmp.h>
+#include <stdlib.h>
+#include <wtf/FastMalloc.h>
+#include <wtf/FastMallocInternal.h>
+#include <wtf/HashCountedSet.h>
+#include <wtf/UnusedParam.h>
 
 #if USE(MULTIPLE_THREADS)
 #include <pthread.h>
@@ -68,7 +68,6 @@ using std::max;
 
 namespace KJS {
 
-
 // tunable parameters
 
 const size_t SPARE_EMPTY_BLOCKS = 2;
@@ -77,8 +76,10 @@ const size_t GROWTH_FACTOR = 2;
 const size_t LOW_WATER_FACTOR = 4;
 const size_t ALLOCATIONS_PER_COLLECTION = 4000;
 
+enum OperationInProgress { NoOperation, Allocation, Collection };
+
 struct CollectorHeap {
-  CollectorBlock **blocks;
+  CollectorBlock** blocks;
   size_t numBlocks;
   size_t usedBlocks;
   size_t firstBlockWithPossibleSpace;
@@ -86,34 +87,17 @@ struct CollectorHeap {
   size_t numLiveObjects;
   size_t numLiveObjectsAtLastCollect;
   size_t extraCost;
+
+  OperationInProgress operationInProgress;
 };
 
-static CollectorHeap heap = {NULL, 0, 0, 0, 0, 0, 0};
+static CollectorHeap heap = { 0, 0, 0, 0, 0, 0, 0, NoOperation };
 
+// FIXME: I don't think this needs to be a static data member of the Collector class.
+// Just a private global like "heap" above would be fine.
 size_t Collector::mainThreadOnlyObjectCount = 0;
-bool Collector::memoryFull = false;
-
-#ifndef NDEBUG
-
-class GCLock {
-    static bool isLocked;
-
-public:
-    GCLock()
-    {
-        ASSERT(!isLocked);
-        isLocked = true;
-    }
-    
-    ~GCLock()
-    {
-        ASSERT(isLocked);
-        isLocked = false;
-    }
-};
 
-bool GCLock::isLocked = false;
-#endif
+bool Collector::memoryFull = false;
 
 static CollectorBlock* allocateBlock()
 {
@@ -190,6 +174,12 @@ void* Collector::allocate(size_t s)
   ASSERT(s <= CELL_SIZE);
   UNUSED_PARAM(s); // s is now only used for the above assert
 
+  ASSERT(heap.operationInProgress == NoOperation);
+  // FIXME: If another global variable access here doesn't hurt performance
+  // too much, we could abort() in NDEBUG builds, which could help ensure we
+  // don't spend any time debugging cases where we allocate inside an object's
+  // deallocation code.
+
   // collect if needed
   size_t numLiveObjects = heap.numLiveObjects;
   size_t numLiveObjectsAtLastCollect = heap.numLiveObjectsAtLastCollect;
@@ -201,8 +191,10 @@ void* Collector::allocate(size_t s)
     numLiveObjects = heap.numLiveObjects;
   }
   
+  ASSERT(heap.operationInProgress == NoOperation);
 #ifndef NDEBUG
-  GCLock lock;
+  // FIXME: Consider doing this in NDEBUG builds too (see comment above).
+  heap.operationInProgress = Allocation;
 #endif
   
   // slab allocator
@@ -252,6 +244,11 @@ allocateNewBlock:
   targetBlock->usedCells = static_cast<uint32_t>(targetBlockUsedCells + 1);
   heap.numLiveObjects = numLiveObjects + 1;
 
+#ifndef NDEBUG
+  // FIXME: Consider doing this in NDEBUG builds too (see comment above).
+  heap.operationInProgress = NoOperation;
+#endif
+
   return newCell;
 }
 
@@ -759,17 +756,14 @@ bool Collector::collect()
   ASSERT(JSLock::lockCount() > 0);
   ASSERT(JSLock::currentThreadIsHoldingLock());
 
+  ASSERT(heap.operationInProgress == NoOperation);
+  if (heap.operationInProgress != NoOperation)
+    abort();
+
+  heap.operationInProgress = Collection;
+
+  bool currentThreadIsMainThread = onMainThread();
 
-#ifndef NDEBUG
-  GCLock lock;
-#endif
-  
-#if USE(MULTIPLE_THREADS)
-    bool currentThreadIsMainThread = onMainThread();
-#else
-    bool currentThreadIsMainThread = true;
-#endif
-    
   // MARK: first mark all referenced objects recursively starting out from the set of root objects
 
 #ifndef NDEBUG
@@ -900,6 +894,8 @@ bool Collector::collect()
   
   memoryFull = (numLiveObjects >= KJS_MEM_LIMIT);
 
+  heap.operationInProgress = NoOperation;
+
   return deleted;
 }
 
@@ -908,12 +904,6 @@ size_t Collector::size()
   return heap.numLiveObjects; 
 }
 
-#ifdef KJS_DEBUG_MEM
-void Collector::finalCheck()
-{
-}
-#endif
-
 size_t Collector::numInterpreters()
 {
   size_t count = 0;
@@ -977,4 +967,9 @@ HashCountedSet<const char*>* Collector::rootObjectTypeCounts()
     return counts;
 }
 
+bool Collector::isBusy()
+{
+    return heap.operationInProgress != NoOperation;
+}
+
 } // namespace KJS
index 59662fddd06360c79955968348596667b922c8db..7fa40a95a900083d14b70b6764238ac30ae6f660 100644 (file)
@@ -3,7 +3,7 @@
  *  This file is part of the KDE libraries
  *  Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
  *  Copyright (C) 2001 Peter Kelly (pmk@post.com)
- *  Copyright (C) 2003 Apple Computer, Inc.
+ *  Copyright (C) 2003, 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
@@ -38,6 +38,7 @@ namespace KJS {
   public:
     static void* allocate(size_t s);
     static bool collect();
+    static bool isBusy(); // true if an allocation or collection is in progress
 
     static const size_t minExtraCostSize = 256;
 
@@ -46,13 +47,6 @@ namespace KJS {
     static size_t size();
     static bool isOutOfMemory() { return memoryFull; }
 
-#ifdef KJS_DEBUG_MEM
-    /**
-     * Check that nothing is left when the last interpreter gets deleted
-     */
-    static void finalCheck();
-#endif
-
     static void protect(JSValue*);
     static void unprotect(JSValue*);