Changeset 53455 in webkit

Timestamp:

Jan 18, 2010 11:00:38 PM (14 years ago)

Author:

ggaren@apple.com

Message:

2010-01-18 Geoffrey Garen <​ggaren@apple.com>

Reviewed by Oliver Hunt.

REGRESSION (52082): Crash on worker thread when reloading ​http://radnan.public.iastate.edu/procedural/
​https://bugs.webkit.org/show_bug.cgi?id=33826

This bug was caused by a GC-protected object being destroyed early by
Heap::destroy. Clients of the GC protect APIs (reasonably) expect pointers
to GC-protected memory to be valid.

The solution is to do two passes of tear-down in Heap::destroy. The first
pass tears down all unprotected objects. The second pass ASSERTs that all
previously protected objects are now unprotected, and then tears down
all perviously protected objects. These two passes simulate the two passes
that would have been required to free a protected object during normal GC.

• API/JSContextRef.cpp: Removed some ASSERTs that have moved into Heap.

• runtime/Collector.cpp: (JSC::Heap::destroy): Moved ASSERTs to here. (JSC::Heap::freeBlock): Tidied up the use of didShrink by moving its setter to the function that does the shrinking. (JSC::Heap::freeBlocks): Implemented above algorithm. (JSC::Heap::shrinkBlocks): Tidied up the use of didShrink.

2010-01-18 Geoffrey Garen <​ggaren@apple.com>

Reviewed by Oliver Hunt.

REGRESSION (52082): Crash on worker thread when reloading ​http://radnan.public.iastate.edu/procedural/
​https://bugs.webkit.org/show_bug.cgi?id=33826

Test: fast/workers/worker-gc2.html

• bindings/js/WorkerScriptController.cpp: (WebCore::WorkerScriptController::~WorkerScriptController): Removed some ASSERTs that have moved to JavaScriptCore.

2010-01-18 Geoffrey Garen <​ggaren@apple.com>

Reviewed by Oliver Hunt.

REGRESSION (52082): Crash on worker thread when reloading ​http://radnan.public.iastate.edu/procedural/
​https://bugs.webkit.org/show_bug.cgi?id=33826

Added a test for this edge case.

• fast/workers/resources/worker-gc2.js: Added. (Dummy):
• fast/workers/worker-gc2.html: Added.

Location:

trunk

Files:

• JavaScriptCore/API/JSContextRef.cpp (modified) (1 diff)
• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/runtime/Collector.cpp (modified) (4 diffs)
• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/workers/resources/worker-gc2.js (added)
• LayoutTests/fast/workers/worker-gc2.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/bindings/js/WorkerScriptController.cpp (modified) (1 diff)

trunk/JavaScriptCore/API/JSContextRef.cpp

@@ -128 +128 @@
     if (globalData.refCount() == 2) { // One reference is held by JSGlobalObject, another added by JSGlobalContextRetain().
         // The last reference was released, this is our last chance to collect.
-        ASSERT(!globalData.heap.protectedObjectCount());
-        ASSERT(!globalData.heap.isBusy());
         globalData.heap.destroy();
     } else

trunk/JavaScriptCore/ChangeLog

@@ +1 @@
+2010-01-18  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Oliver Hunt.
+
+        REGRESSION (52082): Crash on worker thread when reloading http://radnan.public.iastate.edu/procedural/
+        https://bugs.webkit.org/show_bug.cgi?id=33826
+
+        This bug was caused by a GC-protected object being destroyed early by
+        Heap::destroy. Clients of the GC protect APIs (reasonably) expect pointers
+        to GC-protected memory to be valid.
+
+        The solution is to do two passes of tear-down in Heap::destroy. The first
+        pass tears down all unprotected objects. The second pass ASSERTs that all
+        previously protected objects are now unprotected, and then tears down
+        all perviously protected objects. These two passes simulate the two passes
+        that would have been required to free a protected object during normal GC.
+        
+        * API/JSContextRef.cpp: Removed some ASSERTs that have moved into Heap.
+
+        * runtime/Collector.cpp:
+        (JSC::Heap::destroy): Moved ASSERTs to here.
+        (JSC::Heap::freeBlock): Tidied up the use of didShrink by moving its
+        setter to the function that does the shrinking.
+        (JSC::Heap::freeBlocks): Implemented above algorithm.
+        (JSC::Heap::shrinkBlocks): Tidied up the use of didShrink.
+
 2010-01-18  Gavin Barraclough  <barraclough@apple.com>
 

trunk/JavaScriptCore/runtime/Collector.cpp

@@ -187 +187 @@
         return;
 
+    ASSERT(!m_globalData->dynamicGlobalObject);
+    ASSERT(!isBusy());
+    
     // The global object is not GC protected at this point, so sweeping may delete it
     // (and thus the global data) before other objects that may use the global data.
@@ -291 +294 @@
 NEVER_INLINE void Heap::freeBlock(size_t block)
 {
+    m_heap.didShrink = true;
+
     ObjectIterator it(m_heap, block);
     ObjectIterator end(m_heap, block + 1);
@@ -330 +335 @@
 void Heap::freeBlocks()
 {
-    while (m_heap.usedBlocks)
-        freeBlock(0);
+    ProtectCountSet protectedValuesCopy = m_protectedValues;
+
+    clearMarkBits();
+    markProtectedObjects(m_globalData->markStack);
+
+    m_heap.nextCell = 0;
+    m_heap.nextBlock = 0;
+    DeadObjectIterator it(m_heap, m_heap.nextBlock, m_heap.nextCell);
+    DeadObjectIterator end(m_heap, m_heap.usedBlocks);
+    for ( ; it != end; ++it)
+        (*it)->~JSCell();
+
+    ASSERT(!protectedObjectCount());
+
+    ProtectCountSet::iterator protectedValuesEnd = protectedValuesCopy.end();
+    for (ProtectCountSet::iterator protectedValuesIt = protectedValuesCopy.begin(); protectedValuesIt != protectedValuesEnd; ++protectedValuesIt)
+        protectedValuesIt->first->~JSCell();
+
+    for (size_t block = 0; block < m_heap.usedBlocks; ++block)
+        freeBlockPtr(m_heap.blocks[block]);
+
     fastFree(m_heap.blocks);
+
     memset(&m_heap, 0, sizeof(CollectorHeap));
 }
@@ -441 +466 @@
         if (m_heap.blocks[i]->marked.isEmpty()) {
             freeBlock(i);
-            m_heap.didShrink = true;
         } else
             ++i;

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2010-01-18  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Oliver Hunt.
+
+        REGRESSION (52082): Crash on worker thread when reloading http://radnan.public.iastate.edu/procedural/
+        https://bugs.webkit.org/show_bug.cgi?id=33826
+        
+        Added a test for this edge case.
+
+        * fast/workers/resources/worker-gc2.js: Added.
+        (Dummy):
+        * fast/workers/worker-gc2.html: Added.
+
 2010-01-18  Daniel Bates  <dbates@webkit.org>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+2010-01-18  Geoffrey Garen  <ggaren@apple.com>
+
+        Reviewed by Oliver Hunt.
+
+        REGRESSION (52082): Crash on worker thread when reloading http://radnan.public.iastate.edu/procedural/
+        https://bugs.webkit.org/show_bug.cgi?id=33826
+
+        Test: fast/workers/worker-gc2.html
+
+        * bindings/js/WorkerScriptController.cpp:
+        (WebCore::WorkerScriptController::~WorkerScriptController): Removed some
+        ASSERTs that have moved to JavaScriptCore.
+
 2010-01-18  Daniel Bates  <dbates@webkit.org>
 

trunk/WebCore/bindings/js/WorkerScriptController.cpp

@@ -59 +59 @@
 {
     m_workerContextWrapper = 0; // Unprotect the global object.
-
-    ASSERT(!m_globalData->heap.protectedObjectCount());
-    ASSERT(!m_globalData->heap.isBusy());
     m_globalData->heap.destroy();
 }