Bug 30919

Summary: [V8] Remove random crashes by removing retrieval of V8 context during garbage collection.
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: WebCore JavaScriptAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ager, eric, sgjesse
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Fixerate random crashes, v1.
none
Fixerate random crashes, v2.
none
Fixerate random crashes, v3. eric: review-, commit-queue: commit-queue-

Description Dimitri Glazkov (Google) 2009-10-29 11:35:45 PDT
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 :)
Comment 1 Dimitri Glazkov (Google) 2009-10-29 11:40:21 PDT
Created attachment 42117 [details]
Fixerate random crashes, v1.
Comment 2 Dimitri Glazkov (Google) 2009-10-29 11:46:30 PDT
Created attachment 42120 [details]
Fixerate random crashes, v2.
Comment 3 Adam Barth 2009-10-29 11:58:18 PDT
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.
Comment 4 Dimitri Glazkov (Google) 2009-10-29 12:25:56 PDT
Created attachment 42124 [details]
Fixerate random crashes, v3.
Comment 5 Adam Barth 2009-10-29 12:37:52 PDT
Comment on attachment 42124 [details]
Fixerate random crashes, v3.

Thanks.
Comment 6 Søren Gjesse 2009-10-30 00:17:21 PDT
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.
Comment 7 WebKit Commit Bot 2009-11-12 19:06:14 PST
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
Comment 8 Adam Barth 2009-11-20 07:15:40 PST
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.
Comment 9 Eric Seidel (no email) 2009-12-09 14:04:32 PST
Ping?  Looks like we need a new patch here.
Comment 10 Eric Seidel (no email) 2009-12-28 23:00:49 PST
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.
Comment 11 Eric Seidel (no email) 2009-12-28 23:01:48 PST
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?
Comment 12 Dimitri Glazkov (Google) 2009-12-29 08:34:42 PST
I stink. Landed as http://trac.webkit.org/changeset/50293.