Bug 105363 – [Chromium] [V8] IndexedDB: flaky crashes in storage/indexeddb/key-type-array.html

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore Misc.

[Joshua Bell]

Low frequency of crashes on Mac OS:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=storage%2Findexeddb%2Fkey-type-array.html

Sample crash:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7%20(dbg)/builds/1433/steps/webkit_tests/logs/stdio

Stack is useless:

23:52:18.696 1242 worker/2 storage/indexeddb/key-type-array.html crashed, (stderr lines):
23:52:18.696 1242   
23:52:18.696 1242   
23:52:18.696 1242   #
23:52:18.696 1242   # Fatal error in ../../src/heap-inl.h, line 292
23:52:18.696 1242   # CHECK(!result || gc_state_ != NOT_IN_GC || InToSpace(object)) failed
23:52:18.696 1242   #
23:52:18.696 1242

[Joshua Bell]

haraken@ - could you take a look in IDBBindingUtilities.cpp's createIDBKeyFromValue and V8IDBKeyCustom.cpp's toV8() and see if any lifetime issues stand out, e.g. wrong sort of handle being used? I'll dig in as well, of course.

[Joshua Bell]

Vaguely informative stack:

16:00:29.015 9152 worker/21 storage/indexeddb/key-type-array.html crashed, (stderr lines):
16:00:29.015 9152   Received signal 11
16:00:29.015 9152   	base::debug::StackTrace::StackTrace() [0x7f103dd3ffde]
16:00:29.015 9152   	base::debug::(anonymous namespace)::StackDumpSignalHandler() [0x7f103dd3fc93]
16:00:29.015 9152   	<unknown> [0x7f10337a5af0]
16:00:29.015 9152   	v8::internal::Map::instance_type() [0x7f103fb24bea]
16:00:29.015 9152   	v8::internal::Object::IsOddball() [0x7f103fb23a3a]
16:00:29.015 9152   	v8::internal::Object::IsTheHole() [0x7f103fb23cc0]
16:00:29.015 9152   	v8::internal::ObjectHashTable::Put() [0x7f103fd60fc4]
16:00:29.015 9152   	v8::internal::JSObject::SetHiddenProperty() [0x7f103fd428de]
16:00:29.015 9152   	v8::internal::JSObject::SetHiddenProperty() [0x7f103fd424bc]
16:00:29.015 9152   	v8::Object::SetHiddenValue() [0x7f103fb3a785]
16:00:29.015 9152   	WebCore::V8DOMWrapper::setNamedHiddenReference() [0x7f103ace323d]
16:00:29.015 9152   	WebCore::IDBRequestV8Internal::resultAttrGetter() [0x7f103b77d93a]
16:00:29.016 9152   	<unknown> [0x17ca2781a213]
16:00:29.018 8939 [22998/26975] storage/indexeddb/key-type-array.html failed unexpectedly (DumpRenderTree crashed [pid=9189])

[Joshua Bell]

One oddity in the generated code (although I don't think this is related):

static v8::Handle<v8::Value> resultAttrGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
{
    IDBRequest* imp = V8IDBRequest::toNative(info.Holder());
    ExceptionCode ec = 0;
    RefPtr<IDBAny> v = imp->result(ec);
    if (UNLIKELY(ec))
        return setDOMException(ec, info.GetIsolate());
    RefPtr<IDBAny> result = imp->result(ec);

...

Note how imp->result() is called twice. That seems... inefficient.

[Joshua Bell]

I wonder if this crash is due to walking the recursive array in toV8(). I wonder if the JS object gets accidentally released due to a refcount temporarily bouncing off of 0?

[Joshua Bell]

Note that the place in the test where IDBRequest.result is accessed is reading the string result out (expecting "value11" or some such). In instances where this fails with TEXT (not CRASH) the result is a corrupted string (non-ASCII data).

[Joshua Bell]

haraken@, abarth@ - I notice that in the generated V8IDBRequest::resultAttrGetter we're stashing the result of toV8() via:

        wrapper = toV8(result.get(), info.Holder(), info.GetIsolate());
        if (!wrapper.IsEmpty())
            V8DOMWrapper::setNamedHiddenReference(info.Holder(), "result", wrapper);

toV8() in this case is going to be the result of V8IDBAnyCustom.cpp's toV8(IDBAny*, ...) which in turn calls V8IDBKeyCustom.cpp's toV8(IDBKey*, ...) which is returning a v8::Local<v8::Array>. Is that safe?

(Grasping at straws here.)

[Kentaro Hara]

(In reply to comment #6)
> toV8() in this case is going to be the result of V8IDBAnyCustom.cpp's toV8(IDBAny*, ...) which in turn calls V8IDBKeyCustom.cpp's toV8(IDBKey*, ...) which is returning a v8::Local<v8::Array>. Is that safe?

I don't think it's safe. An object returned by toV8() should be a wrapper. A wrapper needs to hold a C++ pointer to the corresponding DOM object. For example, please look at V8ScriptProfileCustom.cpp::toV8():

Handle<Value> toV8() {
  ...;
  v8::Local<v8::Object> instance = ...;
  V8DOMWrapper::setNativeInfo(instance, &V8ScriptProfile::info, impl); // Setting native information on the wrapper object.
  return instance;
}

[Joshua Bell]

(In reply to comment #7)
> I don't think it's safe. An object returned by toV8() should be a wrapper. A wrapper needs to hold a C++ pointer to the corresponding DOM object. 

In some of the IDBAny cases a V8-only value is being returned (i.e. there is no DOM object). Some IDBAny types produce undefined, null, strings, or numbers, and the IDBKey case (one of the possible types of an IDBAny) yields null, string, number, date or array of the previous types.

Is the AttrGetter code gen incorrect for IDBAny? Should it only be creating a wrapper in cases where there's a backing DOM object?

[Kentaro Hara]

(In reply to comment #8)
> In some of the IDBAny cases a V8-only value is being returned (i.e. there is no DOM object). Some IDBAny types produce undefined, null, strings, or numbers, and the IDBKey case (one of the possible types of an IDBAny) yields null, string, number, date or array of the previous types.
> 
> Is the AttrGetter code gen incorrect for IDBAny? Should it only be creating a wrapper in cases where there's a backing DOM object?

Sorry for my previous comment. Returning strings, numbers, arrays etc from toV8() sounds a bit strange but is not a problem.

I thought that if an IDBAny getter is called for a wrapper object returned by toV8() without setting native information, the getter will crash when it tries to retrieve a native pointer from the wrapper object. However, this shouldn't be happen as long as toV8() returns strings, numbers, arrays etc from toV8(). (This kind of problem happens only when toV8() returns a wrapper object of IDBAny without setting native information.)

[Joshua Bell]

(In reply to comment #9)
> Sorry for my previous comment. Returning strings, numbers, arrays etc from toV8() sounds a bit strange but is not a problem.
> 
> I thought that if an IDBAny getter is called for a wrapper object returned by toV8() without setting native information, the getter will crash when it tries to retrieve a native pointer from the wrapper object. However, this shouldn't be happen as long as toV8() returns strings, numbers, arrays etc from toV8(). (This kind of problem happens only when toV8() returns a wrapper object of IDBAny without setting native information.)

Okay. And in the cases where toV8(IDBAny) is returning a DOM object, it's doing so by calling the auto-generated toV8() for those so they're handled correctly.

...

So, back to the bug: sometime late in 2012 there started to be infrequent flaky crashes or corrupted string data in Array-type IDBKey edge-case tests, with no corresponding changes to the IDBKey code or tests. I haven't been able to repro locally at all. :(

The stacks above may be a red herring, since they could be the resultAttrGetter wandering into already corrupted data. The creation of the V8 value in V8IDBKeyCustom.cpp::toV8 looks correct, but I'm still nervous about the v8::Local there - should it just be v8::Handle ?

[Kentaro Hara]

(In reply to comment #10)
> I'm still nervous about the v8::Local there - should it just be v8::Handle ?

At least, this is not a problem. In V8 there are Local handles and Persistent handles. Handle is just a super class of Local and Persistent. We're likely to use Handle instead of Local and Persistent to pass values around.

[Joshua Bell]

I managed to repro this once while working on http://wkbug.com/97375, same stack plus registers, and haven't been able to trigger it again.

[Joshua Bell]

Still poking, with no luck - haven't been able to trigger this again despite many attempts on Linux and MacOS and poking at the test in various ways.

Just to reiterate, the core of the test is:

var value = "simple string value";
var key = [ long, complex, array, of, dates, and, other, stuff, ... ];
var putreq = objectStore.put(value, key);
var getreq = objectStore.get(key);
getreq.onsuccess = function () {
  shouldBeEqualToString("getreq.result", value);
};

On Linux, occasionally the access to getreq.result generates a crash. On MacOS, the output of getreq.result is a garbage string.

Given that the trigger is a long array key, the suspect areas would be:
(1) conversion of JS value to IDBKey during the put() call
(2) handling of the putreq result (but that appears to not even touch v8)
(3) conversion of JS value to IDBKey during the get() call
(4) handling of the getreq result

Given the difficulty to reproduce and the distance between the trigger (array key) and crash (attribute fetch), it feels to me like an issue where either (1) or (3) has a bug, then GC runs corrupting the heap, then (4) exposes it.

[Joshua Bell]

... and the stacks w/ registers on the flakiness dashboard show pointers indicating ZapFromSpace has been called on heap objects, which only happens in GarbageCollectionEpilogue.

[Joshua Bell]

Created attachment 187973 [details]
Patch

[Joshua Bell]

Ugh, my reliable repro is no longer reliably reproducing. :P

(In reply to comment #15)
> Created an attachment (id=187973) [details]
> Patch

One thing that made the repro stop (but then again, so did nearly everything else I tried...) was not producing extraneous wrappers around value types coming out of the IDBAny variant type. I uploaded a patch that implements this in the code generator.

It's ugly - and we could add a IDBAny::isValueType() to simplify - but I think it's more correct than what we do now. That said, it's still unclear why the wrapper path would be causing strings to be released prematurely (which seems to be the root of the problem).

Thoughts?

[Kentaro Hara]

Comment on attachment 187973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187973&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1052
> +        if ($returnType eq "IDBAny") {
> +            push(@implContentDecls, "    if (!result.get() || result->type() == IDBAny::UndefinedType || result->type() == IDBAny::NullType || result->type() == IDBAny::ScriptValueType || result->type() == IDBAny::StringType || result->type() == IDBAny::IntegerType) {\n");
> +            push(@implContentDecls, "        return toV8(result.get(), info.Holder(), info.GetIsolate());\n");
> +            push(@implContentDecls, "    }\n");
> +        }

How about writing this in V8IDBCustom.cpp::toV8()?

I understand this is more correct than the current implementation but this patch will hide a potential bug, as you mentioned.

[Joshua Bell]

(In reply to comment #17)
> How about writing this in V8IDBCustom.cpp::toV8()?

To be clear, that would mean marking IDBAny as a non-wrapper type in the code generator, and somehow having the custom toV8 implementation handle creating the wrapper and hidden property on the container object for when the IDBAny is holding a wrappable object? Otherwise we're losing the benefit of what the code generator is doing.

> I understand this is more correct than the current implementation but this patch will hide a potential bug, as you mentioned.

Yes, so I don't think we should land it until we understand what's going on. I'll see if I can get it reproing again.

[Kentaro Hara]

(In reply to comment #18)
> (In reply to comment #17)
> > How about writing this in V8IDBCustom.cpp::toV8()?
> 
> To be clear, that would mean marking IDBAny as a non-wrapper type in the code generator, and somehow having the custom toV8 implementation handle creating the wrapper and hidden property on the container object for when the IDBAny is holding a wrappable object? Otherwise we're losing the benefit of what the code generator is doing.

Sorry I meant V8IDBAnyCustom.cpp. V8IDBAnyCustom.cpp already has custom toV8(). Why can't you do what you want to do by tweaking the custom toV8(). (Maybe I'm misunderstanding your problem.)

[Joshua Bell]

(In reply to comment #19)
> Sorry I meant V8IDBAnyCustom.cpp. V8IDBAnyCustom.cpp already has custom toV8(). Why can't you do what you want to do by tweaking the custom toV8(). (Maybe I'm misunderstanding your problem.)

IDBAny is a variant/union type which is not exposed to script. In a simplified form, pretend it has two types it can hold: either ScriptValue or an IDBIndex, distinguished by calling IDBAny::type() which gives ScriptValueType or IndexType. In the case where V8IDBRequest::resultAttrGetter is called, this leads to a call to IDBRequest::result() which gives a RefPtr<IDBAny>.

V8IDBAnyCustom's toV8 basically does:

switch(r->type()) {
  ScriptValueType: return r->scriptValue().v8Value(); // already v8 value
  IndexType: return toV8(r->index(), holder, isolate); // wrap
}

Now the autogenerated code for wrapper types kicks in to check the DOMDataStore::getWrapper and setNamedHiddenReference on the IDBRequest w/ "result" and the v8value, to try and prevent the wrapper from being GC'd too early. This makes sense for IDBAny's of IndexType (where IDBIndex would be a "wrapper-type" from codegen's perspective) but so far as I can understand things not for IDBAny's of ScriptValueType (which would be a "non-wrapper-type" from codegen's perspective).

[Kentaro Hara]

(In reply to comment #20)
> Now the autogenerated code for wrapper types kicks in to check the DOMDataStore::getWrapper and setNamedHiddenReference on the IDBRequest w/ "result" and the v8value, to try and prevent the wrapper from being GC'd too early. This makes sense for IDBAny's of IndexType (where IDBIndex would be a "wrapper-type" from codegen's perspective) but so far as I can understand things not for IDBAny's of ScriptValueType (which would be a "non-wrapper-type" from codegen's perspective).

Thanks for the detailed explanation. I understood.

But personally I feel that we might want to solve the original GC problem first. Then we might not need to add such tricky logic to code generators. I'll be in town from next week, so I can help.

[Joshua Bell]

From the investigation so far, this seems to be caused by https://bugs.webkit.org/show_bug.cgi?id=110206

*** This bug has been marked as a duplicate of bug 110206 ***