IndexedDB: Replace custom binding logic with [SetWrapperReferenceTo]

IndexedDB: Replace custom binding logic with [SetWrapperReferenceTo]

An IDBCursor's associated IDBRequest should be re-used each time the
cursor advances. To keep the request's wrapper alive a hidden
reference was set on the cursor's wrapper, with custom bindings code.
Replace that with the [SetWrapperReferenceTo] Blink IDL extended
attribute which does the same thing.

R=haraken@chromium.org,kouhei@chromium.org

[jsbell, 2015-02-13 19:28:01]

haraken@ - the patch was simple, but I'm seeing an intermittent crash here
running the storage/indexeddb layout tests with this CL.

Stack is... entertaining:

* Event is fired into script
* V8 triggers a GC
* The GC calls into V8IDBCursorWithValue::visitDOMWrapper()
* That attempts to wrap the IDBRequest...
* Which crashes in V8RecursionScope::MicrotaskSuppression::MicrotaskSuppression
?

Presumably it's the ASSERT(!ScriptForbiddenScope::isScriptForbidden()), since
the majorGCPrologue calls ScriptForbiddenScope::enter()?

This feels like an assumption in VisitPersistenHandle about visitDOMWrapper that
holds for Nodes but not other objects, and this is just the first use of
SetWrapperReferenceTo to tickle it. Any ideas?

 0 
libblink_web.so!blink::V8RecursionScope::MicrotaskSuppression::MicrotaskSuppression(v8::Isolate*)
[V8RecursionScope.h : 97 + 0x16]
 1  libblink_web.so!blink::ConstructorMode::ConstructorMode(v8::Isolate*)
[V8ObjectConstructor.h : 53 + 0x21]
 2  libblink_web.so!blink::V8ObjectConstructor::newInstance(v8::Isolate*,
v8::Handle<v8::Function>) [V8ObjectConstructor.cpp : 40 + 0x5]
 3  libblink_web.so!blink::V8DOMWrapper::createWrapper(v8::Isolate*,
v8::Handle<v8::Object>, blink::WrapperTypeInfo const*, blink::ScriptWrappable*)
[V8DOMWrapper.cpp : 76 + 0x10]
 4  libblink_web.so!blink::ScriptWrappable::wrap(v8::Handle<v8::Object>,
v8::Isolate*) [ScriptWrappable.cpp : 53 + 0x20]
 5  libblink_web.so!blink::V8IDBCursorWithValue::visitDOMWrapper(v8::Isolate*,
blink::ScriptWrappable*, v8::Persistent<v8::Object,
v8::NonCopyablePersistentTraits<v8::Object> > const&) [V8IDBCursorWithValue.cpp
: 69 + 0x2f]
 6  libblink_web.so!blink::WrapperTypeInfo::visitDOMWrapper(v8::Isolate*,
blink::ScriptWrappable*, v8::Persistent<v8::Object,
v8::NonCopyablePersistentTraits<v8::Object> > const&) const [WrapperTypeInfo.h :
171 + 0x16]
 7 
libblink_web.so!blink::MajorGCWrapperVisitor::VisitPersistentHandle(v8::Persistent<v8::Value,
v8::NonCopyablePersistentTraits<v8::Value> >*, unsigned short)
[V8GCController.cpp : 288 + 0x16]
 8  libv8.so!v8::VisitorAdapter::VisitEmbedderReference(v8::internal::Object**,
unsigned short) [api.cc : 6933 + 0xe]
 9 
libv8.so!v8::internal::GlobalHandles::IterateAllRootsWithClassIds(v8::internal::ObjectVisitor*)
[global-handles.cc : 909 + 0x12]
10  libv8.so!v8::Isolate::VisitHandlesWithClassIds(v8::PersistentHandleVisitor*)
[api.cc : 6946 + 0x5]
11  libblink_web.so!blink::V8GCController::majorGCPrologue(v8::Isolate*, bool)
[v8.h : 7523 + 0xd]
12  libblink_web.so!blink::V8GCController::gcPrologue(v8::GCType,
v8::GCCallbackFlags) [V8GCController.cpp : 348 + 0x21]
13 
libv8.so!v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector,
v8::GCCallbackFlags) [heap.cc : 1234 + 0x8]
14  libv8.so!v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector,
char const*, char const*, v8::GCCallbackFlags) [heap.cc : 894 + 0xe]
15  libv8.so!v8::internal::Heap::CollectAllGarbage(int, char const*,
v8::GCCallbackFlags) [heap-inl.h : 576 + 0x1a]
16  content_shell!content::GCController::CollectAll(gin::Arguments const&)
[gc_controller.cc : 62 + 0x13]
17  content_shell!base::internal::RunnableAdapter<void
(content::EventSenderBindings::*)(std::__debug::vector<std::string,
std::allocator<std::string> > const&)>::Run(content::EventSenderBindings*,
std::__debug::vector<std::string, std::allocator<std::string> > const&)
[bind_internal.h : 176 + 0xd]
18  content_shell!base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void
(content::EventSenderBindings::*)(std::__debug::vector<std::string,
std::allocator<std::string> > const&)>,
base::internal::TypeList<content::EventSenderBindings* const&,
std::__debug::vector<std::string, std::allocator<std::string> > const&>
>::MakeItSo(base::internal::RunnableAdapter<void
(content::EventSenderBindings::*)(std::__debug::vector<std::string,
std::allocator<std::string> > const&)>, content::EventSenderBindings* const&,
std::__debug::vector<std::string, std::allocator<std::string> > const&)
[bind_internal.h : 293 + 0xc]
19  content_shell!base::internal::Invoker<IndexSequence<>,
base::internal::BindState<base::internal::RunnableAdapter<void
(content::GCController::*)(gin::Arguments const&)>, void
(content::GCController*, gin::Arguments const&), base::internal::TypeList<> >,
base::internal::TypeList<>, base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void (content::GCController::*)(gin::Arguments
const&)>, base::internal::TypeList<content::GCController* const&, gin::Arguments
const&> >, void (content::GCController* const&, gin::Arguments
const&)>::Run(base::internal::BindStateBase*, content::GCController* const&,
gin::Arguments const&) [bind_internal.h : 343 + 0x14]
20  content_shell!base::Callback<int (content::WebAXObjectProxy*,
int)>::Run(content::WebAXObjectProxy* const&, int const&) const + 0x5b
21  content_shell!gin::internal::Invoker<gin::internal::IndicesHolder<0ul, 1ul>,
content::AccessibilityControllerBindings*, v8::Handle<v8::Function>
>::DispatchToCallback(base::Callback<void
(content::AccessibilityControllerBindings*, v8::Handle<v8::Function>)>)
[function_template.h : 178 + 0x1d]
22  content_shell!gin::internal::Dispatcher<void (content::GCController*,
gin::Arguments const&)>::DispatchToCallback(v8::FunctionCallbackInfo<v8::Value>
const&) [function_template.h : 212 + 0x28]
23  libv8.so!v8::internal::FunctionCallbackArguments::Call(void
(*)(v8::FunctionCallbackInfo<v8::Value> const&)) [arguments.cc : 33 + 0x3]
24  libv8.so!v8::internal::MaybeHandle<v8::internal::Object>
v8::internal::HandleApiCallHelper<false>(v8::internal::Isolate*,
v8::internal::(anonymous
namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>&)
[builtins.cc : 1077 + 0xd]
25  libv8.so!v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**,
v8::internal::Isolate*) [builtins.cc : 1100 + 0x8]
26  0x27fb807f9b
27  0x27fb882639
28  0x27fb882442
29  0x27fb806395
30  0x27fb82e13c
31  0x27fb816811
32  libv8.so!v8::internal::Invoke(bool,
v8::internal::Handle<v8::internal::JSFunction>,
v8::internal::Handle<v8::internal::Object>, int,
v8::internal::Handle<v8::internal::Object>*) [execution.cc : 128 + 0x14]
33  libv8.so!v8::Function::Call(v8::Handle<v8::Value>, int,
v8::Handle<v8::Value>*) [api.cc : 4251 + 0x13]
34 
libblink_web.so!blink::V8ScriptRunner::callFunction(v8::Handle<v8::Function>,
blink::ExecutionContext*, v8::Handle<v8::Value>, int, v8::Handle<v8::Value>*,
v8::Isolate*) [V8ScriptRunner.cpp : 449 + 0x1e]
35 
libblink_web.so!blink::ScriptController::callFunction(blink::ExecutionContext*,
v8::Handle<v8::Function>, v8::Handle<v8::Value>, int, v8::Handle<v8::Value>*,
v8::Isolate*) [ScriptController.cpp : 155 + 0x38]
36 
libblink_web.so!blink::ScriptController::callFunction(v8::Handle<v8::Function>,
v8::Handle<v8::Value>, int, v8::Handle<v8::Value>*) [ScriptController.cpp : 148
+ 0x1b]
37 
libblink_web.so!blink::V8EventListener::callListenerFunction(blink::ScriptState*,
v8::Handle<v8::Value>, blink::Event*) [V8EventListener.cpp : 88 + 0x16]
38 
libblink_web.so!blink::V8AbstractEventListener::invokeEventHandler(blink::ScriptState*,
blink::Event*, v8::Local<v8::Value>) [V8AbstractEventListener.cpp : 125 + 0x5d]
39 
libblink_web.so!blink::V8AbstractEventListener::handleEvent(blink::ScriptState*,
blink::Event*) [V8AbstractEventListener.cpp : 100 + 0x21]
40 
libblink_web.so!blink::V8AbstractEventListener::handleEvent(blink::ExecutionContext*,
blink::Event*) [V8AbstractEventListener.cpp : 85 + 0x18]
41  libblink_web.so!blink::EventTarget::fireEventListeners(blink::Event*,
blink::EventTargetData*, WTF::Vector<blink::RegisteredEventListener, 1ul,
WTF::DefaultAllocator>&) [EventTarget.cpp : 376 + 0x20]
42  libblink_web.so!blink::EventTarget::fireEventListeners(blink::Event*)
[EventTarget.cpp : 312 + 0x15]
43  libblink_web.so!blink::IDBEventDispatcher::dispatch(blink::Event*,
WTF::Vector<WTF::RefPtr<blink::EventTarget>, 0ul, WTF::DefaultAllocator>&)
[IDBEventDispatcher.cpp : 52 + 0x1d]
44 
libblink_web.so!blink::IDBTransaction::dispatchEvent(WTF::PassRefPtr<blink::Event>)
[IDBTransaction.cpp : 367 + 0x8]
45 
libblink_web.so!blink::DOMWindowEventQueue::dispatchEvent(WTF::PassRefPtr<blink::Event>)
[DOMWindowEventQueue.cpp : 154 + 0x2a]
46  libblink_web.so!blink::DOMWindowEventQueue::pendingEventTimerFired()
[DOMWindowEventQueue.cpp : 143 + 0x15]
47  libblink_web.so!blink::DOMWindowEventQueueTimer::fired()
[DOMWindowEventQueue.cpp : 51 + 0x14]
48  libblink_platform.so!blink::ThreadTimers::sharedTimerFiredInternal()
[ThreadTimers.cpp : 137 + 0xd]
49  libblink_platform.so!blink::ThreadTimers::sharedTimerFired()
[ThreadTimers.cpp : 107 + 0x15]

https://codereview.chromium.org/924863003/diff/1/LayoutTests/storage/indexedd...
File LayoutTests/storage/indexeddb/key-cursor-request-cycle.html (right):

https://codereview.chromium.org/924863003/diff/1/LayoutTests/storage/indexedd...
LayoutTests/storage/indexeddb/key-cursor-request-cycle.html:6:
description("Verify that that key cursors weakly hold request, and work if
request is GC'd");
Added to ensure we're exercising both IDBCursor and IDBCursorWithValue
interfaces.

https://codereview.chromium.org/924863003/diff/1/Source/bindings/modules/v8/I...
File Source/bindings/modules/v8/IDBBindingUtilities.cpp (left):

https://codereview.chromium.org/924863003/diff/1/Source/bindings/modules/v8/I...
Source/bindings/modules/v8/IDBBindingUtilities.cpp:125: // FIXME: Due to race at
worker shutdown, V8 may return empty handles.
Hopefully we're already handling this in the generated code...

[haraken, 2015-02-15 07:46:09]

haraken@chromium.org changed reviewers:
+ kouhei@chromium.org

[haraken, 2015-02-15 07:46:10]

+kouhei

[SetWrapperReferenceTo] is already used in a lot of places, so I'm wondering why
you hit the crash now... Actually I'm not sure what a right fix is.

- Allow a script execution during V8 GC (Note: Wrapper creation in
visitDOMWrapper can invoke arbitrary script execution). This means that we allow
a nested V8 GC, which doesn't seem a right thing to do.

- Disallow to create a V8 wrapper in visitDOMWrapper. Then I'm not sure how to
realize [SetWrapperReferenceTo].
 
kouhei-san: Do you have any idea on this?

[kouhei (in TOK), 2015-02-16 03:34:56]

On 2015/02/15 07:46:10, haraken wrote:
> +kouhei
> 
> [SetWrapperReferenceTo] is already used in a lot of places, so I'm wondering
why
> you hit the crash now... Actually I'm not sure what a right fix is.
> 
> - Allow a script execution during V8 GC (Note: Wrapper creation in
> visitDOMWrapper can invoke arbitrary script execution). This means that we
allow
> a nested V8 GC, which doesn't seem a right thing to do.
> 
> - Disallow to create a V8 wrapper in visitDOMWrapper. Then I'm not sure how to
> realize [SetWrapperReferenceTo].
>  
> kouhei-san: Do you have any idea on this?

We should disallow creating a new V8 wrapper in visitDOMWrapper.
SetWrapperReferenceTo generated code should never create new wrapper, but only
used to keep the target wrapper alive.

I'll create a CL to remove V8 wrapper creation code.

[jsbell, 2015-02-17 20:59:48]

On 2015/02/16 03:34:56, kouhei wrote:
> I'll create a CL to remove V8 wrapper creation code.

Thanks! But... giving it a whirl with this CL I'm able to hit the new
RELEASE_ASSERT.

Repro: this CL, running run-webkit-tests --iterations=100
--exit-after-n-crashes-or-timeouts=1 --no-new-test-results 'storage/indexeddb/'
- may take 500-1000 tests to hit it, though.

 0  libblink_web.so!blink::V8IDBCursorWithValue::visitDOMWrapper(v8::Isolate*,
blink::ScriptWrappable*, v8::Persistent<v8::Object,
v8::NonCopyablePersistentTraits<v8::Object> > const&) [V8IDBCursorWithValue.cpp
: 68 + 0x16]
 1  libblink_web.so!blink::WrapperTypeInfo::visitDOMWrapper(v8::Isolate*,
blink::ScriptWrappable*, v8::Persistent<v8::Object,
v8::NonCopyablePersistentTraits<v8::Object> > const&) const [WrapperTypeInfo.h :
171 + 0x16]
 2 
libblink_web.so!blink::MajorGCWrapperVisitor::VisitPersistentHandle(v8::Persistent<v8::Value,
v8::NonCopyablePersistentTraits<v8::Value> >*, unsigned short)
[V8GCController.cpp : 288 + 0x16]
 3  libv8.so!v8::VisitorAdapter::VisitEmbedderReference(v8::internal::Object**,
unsigned short) [api.cc : 6933 + 0xe]
 4 
libv8.so!v8::internal::GlobalHandles::IterateAllRootsWithClassIds(v8::internal::ObjectVisitor*)
[global-handles.cc : 909 + 0x12]
 5  libv8.so!v8::Isolate::VisitHandlesWithClassIds(v8::PersistentHandleVisitor*)
[api.cc : 6946 + 0x5]
 6  libblink_web.so!blink::V8GCController::majorGCPrologue(v8::Isolate*, bool)
[v8.h : 7523 + 0xd]
 7  libblink_web.so!blink::V8GCController::gcPrologue(v8::GCType,
v8::GCCallbackFlags) [V8GCController.cpp : 348 + 0x21]
 8 
libv8.so!v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector,
v8::GCCallbackFlags) [heap.cc : 1232 + 0x8]
 9  libv8.so!v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector,
char const*, char const*, v8::GCCallbackFlags) [heap.cc : 892 + 0xe]
10  libv8.so!v8::internal::Heap::CollectAllGarbage(int, char const*,
v8::GCCallbackFlags) [heap-inl.h : 576 + 0x1a]
11  content_shell!content::GCController::CollectAll(gin::Arguments const&)
[gc_controller.cc : 62 + 0x13]
12  content_shell!base::internal::RunnableAdapter<void
(content::EventSenderBindings::*)(std::__debug::vector<std::string,
std::allocator<std::string> > const&)>::Run(content::EventSenderBindings*,
std::__debug::vector<std::string, std::allocator<std::string> > const&)
[bind_internal.h : 176 + 0xd]
...

Since the only way to access an IDBCursor instance is via IDBRequest
(request.result) that seems to imply:

* script has a reference to a request - therefore, the request wrapper exists
* script accesses request.result - this creates the cursor's wrapper
* [[something happens]]
* gc prologue occurs, and when the cursor's wrapper is visited the request's
wrapper no longer exists

Perhaps a red herring, but the stack above is from a layout test that never
actually uses a cursor, so it's cleanup of a previous test's objects.

Another thought: we do use cursors "internally" for index population, but
reviewing the code (IndexPopulator::handleEvent) I don't see that we'd cause
cursor wrapper creation at all.

I'll keep pondering. If anyone has suggestions for additional assertions to
sprinkle in that'd be helpful.

[haraken, 2015-02-18 00:03:05]

> Since the only way to access an IDBCursor instance is via IDBRequest
> (request.result) that seems to imply:
> 
> * script has a reference to a request - therefore, the request wrapper exists
> * script accesses request.result - this creates the cursor's wrapper
> * [[something happens]]
> * gc prologue occurs, and when the cursor's wrapper is visited the request's
> wrapper no longer exists

I wonder why it happens, because the request's wrapper is not collected until we
hit a GC. When can the request's wrapper go away without hitting a GC?

[jsbell, 2015-02-18 01:12:23]

On 2015/02/18 00:03:05, haraken wrote:
> > Since the only way to access an IDBCursor instance is via IDBRequest
> > (request.result) that seems to imply:
> > 
> > * script has a reference to a request - therefore, the request wrapper
exists
> > * script accesses request.result - this creates the cursor's wrapper
> > * [[something happens]]
> > * gc prologue occurs, and when the cursor's wrapper is visited the request's
> > wrapper no longer exists
> 
> I wonder why it happens, because the request's wrapper is not collected until
we
> hit a GC. When can the request's wrapper go away without hitting a GC?

kouhei's change was reverted an hour or two ago - big spike in crashes on
Canary. So there's definitely something unexpected going on even w/o this
change.

https://codereview.chromium.org/931543002/

(The issue with this CL could still be IDB-specific, though.)

[jsbell, 2015-07-09 00:08:54]

haraken writes in https://codereview.chromium.org/1200613002/:

>>>>
[SetWrapperReferenceTo] is not a way to "create the target wrapper and keep it
alive". [SetWrapperReferenceTo] is a way to "keep the target wrapper alive if
the target wrapper exists". It is not a responsibility of
[SetWrapperReferenceFrom/To] to create the wrapper :)
<<<<

Ah, excellent point. I'd still like to eliminate the custom bindings here; it's
fragile to manually ensure that every place an IDBCursor is returned to script
the reference to an IDBRequest is established, and prevents us from replacing
chunks of IDBAny with a union. So maybe I can explore a
[EnsureWrapperReferenceTo] CL at some point.

[haraken, 2015-07-09 00:20:24]

On 2015/07/09 00:08:54, jsbell wrote:
> haraken writes in https://codereview.chromium.org/1200613002/:
> 
> >>>>
> [SetWrapperReferenceTo] is not a way to "create the target wrapper and keep it
> alive". [SetWrapperReferenceTo] is a way to "keep the target wrapper alive if
> the target wrapper exists". It is not a responsibility of
> [SetWrapperReferenceFrom/To] to create the wrapper :)
> <<<<
> 
> Ah, excellent point. I'd still like to eliminate the custom bindings here;
it's
> fragile to manually ensure that every place an IDBCursor is returned to script
> the reference to an IDBRequest is established, and prevents us from replacing
> chunks of IDBAny with a union. So maybe I can explore a
> [EnsureWrapperReferenceTo] CL at some point.

Yeah, that makes sense. And [EnsureWrapperReferenceTo] should create the target
wrapper when the original wrapper is created (not when a GC calls
visitDOMWrapper).