Oilpan: We need to collect all garbage before shutting down the worker thread

Source/heap/ThreadState.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 278 matching lines @@
     // From here on ignore all conservatively discovered
     // pointers into the heap owned by this thread.
     m_isCleaningUp = true;
 
     for (size_t i = 0; i < m_cleanupTasks.size(); i++)
         m_cleanupTasks[i]->preCleanup();
 
     // After this GC we expect heap to be empty because
     // preCleanup tasks should have cleared all persistent
     // handles that were externally owned.
-    // FIXME: oilpan: we should perform a single GC and everything
-    // should die. Unfortunately it is not the case for all objects
-    // because the hierarchy was not completely moved to the heap and
-    // some heap allocated objects own objects that contain persistents
-    // pointing to other heap allocated objects.
-    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
-    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+    Heap::collectAllGarbage(ThreadState::NoHeapPointersOnStack);
 
     // Verify that all heaps are empty now.
     for (int i = 0; i < NumberOfHeaps; i++)
         m_heaps[i]->assertEmpty();
 
     for (size_t i = 0; i < m_cleanupTasks.size(); i++)
         m_cleanupTasks[i]->postCleanup();
 
     m_cleanupTasks.clear();
 }
 
 void ThreadState::detach()
 {
     ThreadState* state = current();
+
+    // When ThreadState is detaching from non-main thread its

[Mads Ager (chromium), 2014/03/05 07:40:34]

Ah, you moved it here. Have the comment on what cleanup does in the header file
with the other comments?

[haraken, 2014/03/05 08:50:26]

Done.

+    // heap is expected to be empty (because it is going away).
+    // Perform registered cleanup tasks and garbage collection
+    // to sweep away any objects that are left on this heap.
+    // We assert that nothing must remain after this cleanup.
+    // If assertion does not hold we crash as we are potentially
+    // in the dangling pointer situation.
+    state->cleanup();
+
     // Enter safe point before trying to acquire threadAttachMutex
     // to avoid dead lock if another thread is preparing for GC, has acquired
     // threadAttachMutex and waiting for other threads to pause or reach a
     // safepoint.
     if (!state->isAtSafePoint())
         state->enterSafePointWithoutPointers();
     MutexLocker locker(threadAttachMutex());
     state->leaveSafePoint();
     attachedThreads().remove(state);
     delete state;

[kouhei (in TOK), 2014/03/05 05:58:15]

Doesn't this mean we fail if we couldn't collect all objects within 5 gc cycles?

[haraken, 2014/03/05 06:00:57]

Yes, it means. However, there is no way to know the number of GCs we really need
to trigger. I guess 5 would be enough. If we have a persistent chain longer than
5, it will be something we should fix.

 }
 
 void ThreadState::visitRoots(Visitor* visitor)
 {
     {
         // All threads are at safepoints so this is not strictly necessary.
         // However we acquire the mutex to make mutation and traversal of this
         // list symmetrical.
         MutexLocker locker(globalRootsMutex());
         globalRoots()->trace(visitor);
@@ skipping 360 matching lines @@
     state->safePoint(HeapPointersOnStack);
 }
 
 ThreadState::AttachedThreadStateSet& ThreadState::attachedThreads()
 {
     DEFINE_STATIC_LOCAL(AttachedThreadStateSet, threads, ());
     return threads;
 }
 
 }