Finish completeSweep before shutting down V8

Finish completeSweep before shutting down V8

If Oilpan runs completeSweep after shutting down V8, some destructor
may access V8 and cause crash. Oilpan should finish completeSweep before
shutting down V8.

BUG=593092
Committed: https://crrev.com/29a57aad9a5cd9388811408f1a457493d6f9671d
Cr-Commit-Position: refs/heads/master@{#380415}

[haraken, 2016-03-10 05:42:22]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2016-03-10 05:42:23]

PTAL

https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:219: completeSweep();

Another option is just to remove completeSweep(). There is no reason we have to
complete sweeping before shutting down the main thread. However, I prefer
completing sweeping just in case.

In production, blink::shutdown would rarely be called because the renderer is
killed immediately in common cases. So having completeSweep() in the shutdown
sequence won't matter for real-world performance.

https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:219: completeSweep();

Maybe we should add enterGCForbidenScope() before finishing stopMainThread() to
make sure that no GC happens before detaching the main thread. (I don't have a
strong opinion.)

https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.h (left):

https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.h:433: void
unregisterTraceDOMWrappers()

Previously I mentioned that we need to clear m_traceDOMWrappers before detaching
the main thread, but I was wrong. I don't think we need it because Oilpan's GC
should not happen after stopMainThread is called.

[haraken, 2016-03-10 06:13:13]

On 2016/03/10 05:42:23, haraken wrote:
> PTAL
> 
>
https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/heap/ThreadState.cpp:219: completeSweep();
> 
> Another option is just to remove completeSweep(). There is no reason we have
to
> complete sweeping before shutting down the main thread. However, I prefer
> completing sweeping just in case.
> 
> In production, blink::shutdown would rarely be called because the renderer is
> killed immediately in common cases. So having completeSweep() in the shutdown
> sequence won't matter for real-world performance.
> 
>
https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/heap/ThreadState.cpp:219: completeSweep();
> 
> Maybe we should add enterGCForbidenScope() before finishing stopMainThread()
to
> make sure that no GC happens before detaching the main thread. (I don't have a
> strong opinion.)
> 
>
https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/heap/ThreadState.h (left):
> 
>
https://codereview.chromium.org/1771353010/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/heap/ThreadState.h:433: void
> unregisterTraceDOMWrappers()
> 
> Previously I mentioned that we need to clear m_traceDOMWrappers before
detaching
> the main thread, but I was wrong. I don't think we need it because Oilpan's GC
> should not happen after stopMainThread is called.

(This fix is rather urgent.)

[sof, 2016-03-10 06:33:21]

bots are red; will take a close look at the CL at the other side of the morning
commute.

[haraken, 2016-03-10 06:38:41]

On 2016/03/10 06:33:21, sof wrote:
> bots are red; will take a close look at the CL at the other side of the
morning
> commute.

ah, I need to remove ASSERT(!state->m_isolate).

[sof, 2016-03-10 07:00:51]

lgtm.

I suspect this will take care of issue 592880 also.

[haraken, 2016-03-10 07:01:13]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-03-10 07:01:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1771353010/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1771353010/40001

[commit-bot: I haz the power, 2016-03-10 07:19:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-10 07:19:44]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[haraken, 2016-03-10 07:28:53]

https://codereview.chromium.org/1771353010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1771353010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:233:
SafePointAwareMutexLocker locker(threadAttachMutex(),
BlinkGC::NoHeapPointersOnStack);
Ah, now I remember why I had to clear state->m_isolate before detaching the main
thread.

1) V8 is shut down.
2) ThreadState::detachMainThread() is called.
3) SafePointAwareMutexLocker triggers a GC.
4) The GC tries to iterate V8 wrappers. V8 is already gone.

#0 0x000004714f2b base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7fcaeee9dcb0 <unknown>
#2 0x00000293f208 v8::internal::GlobalHandles::IterateAllRootsWithClassIds()
#3 0x0000026f6746 v8::Isolate::VisitHandlesWithClassIds()
#4 0x000002cdda0f blink::V8GCController::traceDOMWrappers()
#5 0x000002c70b73 blink::ThreadState::visitPersistents()
#6 0x000002c70941 blink::ThreadState::visitPersistentRoots()
#7 0x000002c663cf blink::Heap::collectGarbage()
#8 0x000002c672e1 blink::Heap::collectAllGarbage()
#9 0x000002c765b6 blink::ThreadState::enterSafePoint()
#10 0x000002c7014a blink::SafePointAwareMutexLocker::SafePointAwareMutexLocker()
#11 0x000002c6ffd0 blink::ThreadState::detachMainThread()
#12 0x000002c8f1f7 blink::shutdownWithoutV8()
#13 0x00000144ca68
content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport()
#14 0x00000144cb49
content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport()
#15 0x000001448b6b content::UnitTestTestSuite::~UnitTestTestSuite()

Clearing m_isolate is helpful to prevent the Oilpan's GC from iterating V8
wrappers, but it's not a right solution because it's already problematic that a
GC is triggered after V8 is shut down. As described in this bug, some destructor
invoked in the GC may access V8 and cause crashes.

My proposal is 1) change the SafePointMutexLocker to MutexLocker or 2) just
remove the SafePointMutexLocker. It is already guaranteed that the main thread
is the last thread that gets detached. So we don't need to grab the mutex here.

What do you think?

[sof, 2016-03-10 07:35:41]

https://codereview.chromium.org/1771353010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1771353010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:233:
SafePointAwareMutexLocker locker(threadAttachMutex(),
BlinkGC::NoHeapPointersOnStack);
On 2016/03/10 07:28:53, haraken wrote:
> Ah, now I remember why I had to clear state->m_isolate before detaching the
main
> thread.
> 
> 1) V8 is shut down.
> 2) ThreadState::detachMainThread() is called.
> 3) SafePointAwareMutexLocker triggers a GC.
> 4) The GC tries to iterate V8 wrappers. V8 is already gone.
> 
> #0 0x000004714f2b base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #1 0x7fcaeee9dcb0 <unknown>
> #2 0x00000293f208 v8::internal::GlobalHandles::IterateAllRootsWithClassIds()
> #3 0x0000026f6746 v8::Isolate::VisitHandlesWithClassIds()
> #4 0x000002cdda0f blink::V8GCController::traceDOMWrappers()
> #5 0x000002c70b73 blink::ThreadState::visitPersistents()
> #6 0x000002c70941 blink::ThreadState::visitPersistentRoots()
> #7 0x000002c663cf blink::Heap::collectGarbage()
> #8 0x000002c672e1 blink::Heap::collectAllGarbage()
> #9 0x000002c765b6 blink::ThreadState::enterSafePoint()
> #10 0x000002c7014a
blink::SafePointAwareMutexLocker::SafePointAwareMutexLocker()
> #11 0x000002c6ffd0 blink::ThreadState::detachMainThread()
> #12 0x000002c8f1f7 blink::shutdownWithoutV8()
> #13 0x00000144ca68
> content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport()
> #14 0x00000144cb49
> content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport()
> #15 0x000001448b6b content::UnitTestTestSuite::~UnitTestTestSuite()
> 
> Clearing m_isolate is helpful to prevent the Oilpan's GC from iterating V8
> wrappers, but it's not a right solution because it's already problematic that
a
> GC is triggered after V8 is shut down. As described in this bug, some
destructor
> invoked in the GC may access V8 and cause crashes.
> 
> My proposal is 1) change the SafePointMutexLocker to MutexLocker or 2) just
> remove the SafePointMutexLocker. It is already guaranteed that the main thread
> is the last thread that gets detached. So we don't need to grab the mutex
here.
> 
> What do you think?

If you add an assert or two to capture those assumptions in detachMainThread(),
that approach sounds fine to me.

[haraken, 2016-03-10 07:48:29]

On 2016/03/10 07:35:41, sof wrote:
>
https://codereview.chromium.org/1771353010/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/1771353010/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/heap/ThreadState.cpp:233:
> SafePointAwareMutexLocker locker(threadAttachMutex(),
> BlinkGC::NoHeapPointersOnStack);
> On 2016/03/10 07:28:53, haraken wrote:
> > Ah, now I remember why I had to clear state->m_isolate before detaching the
> main
> > thread.
> > 
> > 1) V8 is shut down.
> > 2) ThreadState::detachMainThread() is called.
> > 3) SafePointAwareMutexLocker triggers a GC.
> > 4) The GC tries to iterate V8 wrappers. V8 is already gone.
> > 
> > #0 0x000004714f2b base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> > #1 0x7fcaeee9dcb0 <unknown>
> > #2 0x00000293f208 v8::internal::GlobalHandles::IterateAllRootsWithClassIds()
> > #3 0x0000026f6746 v8::Isolate::VisitHandlesWithClassIds()
> > #4 0x000002cdda0f blink::V8GCController::traceDOMWrappers()
> > #5 0x000002c70b73 blink::ThreadState::visitPersistents()
> > #6 0x000002c70941 blink::ThreadState::visitPersistentRoots()
> > #7 0x000002c663cf blink::Heap::collectGarbage()
> > #8 0x000002c672e1 blink::Heap::collectAllGarbage()
> > #9 0x000002c765b6 blink::ThreadState::enterSafePoint()
> > #10 0x000002c7014a
> blink::SafePointAwareMutexLocker::SafePointAwareMutexLocker()
> > #11 0x000002c6ffd0 blink::ThreadState::detachMainThread()
> > #12 0x000002c8f1f7 blink::shutdownWithoutV8()
> > #13 0x00000144ca68
> > content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport()
> > #14 0x00000144cb49
> > content::TestBlinkWebUnitTestSupport::~TestBlinkWebUnitTestSupport()
> > #15 0x000001448b6b content::UnitTestTestSuite::~UnitTestTestSuite()
> > 
> > Clearing m_isolate is helpful to prevent the Oilpan's GC from iterating V8
> > wrappers, but it's not a right solution because it's already problematic
that
> a
> > GC is triggered after V8 is shut down. As described in this bug, some
> destructor
> > invoked in the GC may access V8 and cause crashes.
> > 
> > My proposal is 1) change the SafePointMutexLocker to MutexLocker or 2) just
> > remove the SafePointMutexLocker. It is already guaranteed that the main
thread
> > is the last thread that gets detached. So we don't need to grab the mutex
> here.
> > 
> > What do you think?
> 
> If you add an assert or two to capture those assumptions in
detachMainThread(),
> that approach sounds fine to me.

PTAL

[sof, 2016-03-10 07:52:16]

lgtm

https://codereview.chromium.org/1771353010/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1771353010/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/ThreadState.cpp:237:
ASSERT(!state->isSweepingInProgress());
nit: add ASSERT(state->isGCForbidden());

[haraken, 2016-03-10 07:53:08]

>
https://codereview.chromium.org/1771353010/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/1771353010/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/heap/ThreadState.cpp:237:
> ASSERT(!state->isSweepingInProgress());
> nit: add ASSERT(state->isGCForbidden());

Done.

[haraken, 2016-03-10 07:53:20]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-03-10 07:53:20]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1771353010/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-03-10 07:53:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1771353010/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1771353010/80001

[commit-bot: I haz the power, 2016-03-10 08:18:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-10 08:18:10]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[sof, 2016-03-10 10:20:08]

shutdown untidiness in a HeapTest unit test.

[haraken, 2016-03-10 10:45:18]

https://codereview.chromium.org/1771353010/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/HeapTest.cpp (left):

https://codereview.chromium.org/1771353010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapTest.cpp:5754: class
CrossThreadPersistentOnMainThreadTester {

Would it make sense to remove this test?

https://codereview.chromium.org/1771353010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapTest.cpp:5766:
ThreadState::detachMainThread();

It's not allowed to detach the main thread before detaching other threads.

https://codereview.chromium.org/1771353010/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/HeapTest.cpp:5780: // Do this to verify
that CrossThreadPersistent<>s referring to objects

The CrossThreadPersistent is not pointing to an object on the main thread (it's
pointing to the object on the worker thread)...

[haraken, 2016-03-10 14:02:01]

Looks like the tests pass. PTAL.

[sof, 2016-03-10 14:16:35]

lgtm

sorry, forgot to follow up on HeapTest question, but agreeing that the test in
question was no longer testing a valid condition.

[haraken, 2016-03-10 14:20:36]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-03-10 14:20:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1771353010/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1771353010/140001

[commit-bot: I haz the power, 2016-03-10 15:50:01]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-10 15:51:13]

Description was changed from

==========
Finish completeSweep before shutting down V8

If Oilpan runs completeSweep after shutting down V8, some destructor
may access V8 and cause crash. Oilpan should finish completeSweep before
shutting down V8.

BUG=593092
==========

to

==========
Finish completeSweep before shutting down V8

If Oilpan runs completeSweep after shutting down V8, some destructor
may access V8 and cause crash. Oilpan should finish completeSweep before
shutting down V8.

BUG=593092

Committed: https://crrev.com/29a57aad9a5cd9388811408f1a457493d6f9671d
Cr-Commit-Position: refs/heads/master@{#380415}
==========

[commit-bot: I haz the power, 2016-03-10 15:51:14]

Patchset 8 (id:??) landed as
https://crrev.com/29a57aad9a5cd9388811408f1a457493d6f9671d
Cr-Commit-Position: refs/heads/master@{#380415}