Bug 30919 – [V8] Remove random crashes by removing retrieval of V8 context during garbage collection.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dimitri Glazkov (Google)]

These exhibit themselves as random crashes on innocent-looking tests.

The timing has to be just right. I wasn't able to write a test for this.

The issue occurs when all of the following is true:

1) new V8 context is being created.
2) creating the context (actual allocation of descriptor) triggers garbage collection
3) there's an existing left-over DOM object in map that ask to  needs to retrieve current V8 context (MessagePort, for 
example).

Like so:

*boom* <-- global_context ain't yet cooked.
test_shell!v8::internal::Context::global_context+0x42
test_shell!v8::internal::Context::global_proxy+0x5
test_shell!v8::Context::Global+0x52
test_shell!WebCore::WorkerContextExecutionProxy::retrieve+0x2e
test_shell!WebCore::V8DOMWrapper::convertToV8Object+0x31
test_shell!WebCore::GCPrologueVisitor::visitDOMWrapper+0x93
test_shell!WebCore::visitActiveDOMObjectsInCurrentThread+0xa2
test_shell!WebCore::V8GCController::gcPrologue+0x1f
test_shell!v8::internal::Heap::PerformGarbageCollection+0x15
test_shell!v8::internal::Heap::CollectGarbage+0x8a
test_shell!v8::internal::Factory::NewGlobalObject+0x77
test_shell!v8::internal::Genesis::CreateRoots+0x5aa
test_shell!v8::internal::Genesis::Genesis+0x7d
test_shell!v8::internal::Bootstrapper::CreateEnvironment+0x1b
test_shell!v8::Context::New+0x14a
test_shell!WebCore::V8Proxy::createNewContext+0x241
test_shell!WebCore::V8Proxy::initContextIfNeeded+0x79
test_shell!WebCore::V8Proxy::updateDocument+0x21
test_shell!WebCore::Frame::setDocument+0x9e
test_shell!WebCore::FrameLoader::begin+0x273

Luckily, we don't need to do V8DOMWrapper::convertToV8Object. We can just reach into the map and grab the object, since we know it's there.

I wasn't able to write a layout test that consistently triggers this condition. I can trigger garbage collection and I can trigger navigation, but not at the same time :)

[Dimitri Glazkov (Google)]

Created attachment 42117 [details]
Fixerate random crashes, v1.

[Dimitri Glazkov (Google)]

Created attachment 42120 [details]
Fixerate random crashes, v2.

[Adam Barth]

Comment on attachment 42120 [details]
Fixerate random crashes, v2.

+ V8DOMWrapper::jsWrapperForDOMObject

We should have two of these: one for DOMObjects and one for ActiveDOMObjects, to mirror the setters.

[Dimitri Glazkov (Google)]

Created attachment 42124 [details]
Fixerate random crashes, v3.

[Adam Barth]

Comment on attachment 42124 [details]
Fixerate random crashes, v3.

Thanks.

[Søren Gjesse]

Nit:

I Don't think it it's required retrieve port1Wrapper, as it is already in wrapper, that is

  v8::Handle<v8::Value> port1Wrapper = V8DOMWrapper::jsWrapperForActiveDOMObject(port1);

could be

  v8::Handle<v8::Value> port1Wrapper = wrapper

or the variable port1Wrapper could be removed.

[WebKit Commit Bot]

Comment on attachment 42124 [details]
Fixerate random crashes, v3.

Rejecting patch 42124 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adam Barth', '--force']" exit_code: 1
Last 500 characters of output:
geLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/bindings/v8/V8DOMWrapper.cpp
Hunk #1 succeeded at 208 with fuzz 1 (offset 54 lines).
patching file WebCore/bindings/v8/V8DOMWrapper.h
Hunk #1 FAILED at 279.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/bindings/v8/V8DOMWrapper.h.rej
patching file WebCore/bindings/v8/V8GCController.cpp
Hunk #1 FAILED at 218.
Hunk #2 FAILED at 228.
2 out of 2 hunks FAILED -- saving rejects to file WebCore/bindings/v8/V8GCController.cpp.rej

[Adam Barth]

Comment on attachment 42124 [details]
Fixerate random crashes, v3.

Rejecting patch 42124 from commit-queue.

Failed to run "['/Users/abarth/git/webkit-kr/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adam Barth']" exit_code: 1
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/bindings/v8/V8DOMWrapper.cpp
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file WebCore/bindings/v8/V8DOMWrapper.cpp.rej
patch -p0 "WebCore/bindings/v8/V8DOMWrapper.cpp" returned 1.  Pass --force to ignore patch failures.

[Eric Seidel (no email)]

Ping?  Looks like we need a new patch here.

[Eric Seidel (no email)]

Comment on attachment 42124 [details]
Fixerate random crashes, v3.

Changing Adam Barth's r+ to an r- since this patch no longer applies.  We need a new patch.

[Eric Seidel (no email)]

I mean, I guess if Dimitri plans to land this himself, that's totally fine and he should consider it still r+'d by Adam, but not having been touched in nearly 2 months, it's silly to have this still sitting in the pending-commit list.  That's assuming this bug should even still be open?

[Dimitri Glazkov (Google)]

I stink. Landed as http://trac.webkit.org/changeset/50293.