105363 2012-12-18 16:24:48 -0800 [Chromium] [V8] IndexedDB: flaky crashes in storage/indexeddb/key-type-array.html 2013-02-27 11:57:08 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED DUPLICATE 110206 P2 Normal --- 110916 1 jsbell jsbell abarth alecflett dgrogan haraken oldest_to_newest 794150 0 jsbell 2012-12-18 16:24:48 -0800 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 794168 1 jsbell 2012-12-18 16:33:09 -0800 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. 799844 2 jsbell 2013-01-03 16:47:03 -0800 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]) 799845 3 jsbell 2013-01-03 16:47:08 -0800 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. 799846 4 jsbell 2013-01-03 16:48:49 -0800 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? 804813 5 jsbell 2013-01-10 16:27:42 -0800 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). 804824 6 jsbell 2013-01-10 16:58:14 -0800 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.) 805016 7 haraken 2013-01-11 00:07:22 -0800 (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; } 805291 8 jsbell 2013-01-11 08:51:59 -0800 (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? 805303 9 haraken 2013-01-11 09:05:59 -0800 (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.) 805310 10 jsbell 2013-01-11 09:15:09 -0800 (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 ? 805313 11 haraken 2013-01-11 09:19:35 -0800 (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. 806629 12 jsbell 2013-01-14 14:25:56 -0800 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. 809777 13 jsbell 2013-01-17 14:20:20 -0800 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. 809861 14 jsbell 2013-01-17 15:14:36 -0800 ... and the stacks w/ registers on the flakiness dashboard show pointers indicating ZapFromSpace has been called on heap objects, which only happens in GarbageCollectionEpilogue. 831460 15 187973 jsbell 2013-02-12 17:25:58 -0800 Created attachment 187973 Patch 831463 16 jsbell 2013-02-12 17:29:24 -0800 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? 831492 17 187973 haraken 2013-02-12 18:16:47 -0800 Comment on attachment 187973 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. 831656 18 jsbell 2013-02-12 22:56:56 -0800 (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. 831658 19 haraken 2013-02-12 23:00:05 -0800 (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.) 831987 20 jsbell 2013-02-13 09:43:47 -0800 (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). 832518 21 haraken 2013-02-13 16:41:52 -0800 (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. 843335 22 jsbell 2013-02-27 11:57:08 -0800 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 *** 187973 2013-02-12 17:25:58 -0800 2013-02-12 18:16:47 -0800 Patch bug-105363-20130212172228.patch text/plain 2363 jsbell U3VidmVyc2lvbiBSZXZpc2lvbjogMTQyNjQ2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYjUwODgzMjRkMzU1ZThh ZDRiMmMzNTcwNTRiZjMzNzUzMjJjY2E0MS4uZTQ4NDZhMzQ4NWFhNDkyZTJlZTE0N2Q2ZTIxOTMy YTQ2YjMwNmNiOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEzLTAyLTEyICBKb3No dWEgQmVsbCAgPGpzYmVsbEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW0Nocm9taXVtXSBbVjhd IEluZGV4ZWREQjogZmxha3kgY3Jhc2hlcyBpbiBzdG9yYWdlL2luZGV4ZWRkYi9rZXktdHlwZS1h cnJheS5odG1sCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMDUzNjMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBObyBuZXcgdGVzdHMgKE9PUFMhKS4KKworICAgICAgICAqIGJpbmRpbmdzL3NjcmlwdHMvQ29k ZUdlbmVyYXRvclY4LnBtOgorICAgICAgICAoR2VuZXJhdGVOb3JtYWxBdHRyR2V0dGVyKToKKwog MjAxMy0wMi0xMiAgVG9tYXMgUG9wZWxhICA8dHBvcGVsYUByZWRoYXQuY29tPgogCiAgICAgICAg IFtHVEtdW0ludHJvc3BlY3Rpb25dIEdPYmplY3QgYmluZGluZ3MgZm9yIERhdGFUcmFuc2Zlckl0 ZW1MaXN0IC0gb25lIGFkZCgpIG1ldGhvZCBtdXN0IGJlIHJlbW92ZWQgZnJvbSAuaWRsCmRpZmYg LS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0NvZGVHZW5lcmF0b3JWOC5w bSBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtCmlu ZGV4IDg2ZjhlYWJmZjFhZjFhMWE0YzZmNTY4NmJjMjhmNWU3YTZmNTI3NDYuLmI2YWRiMGJmMmU2 YzhhNzUxMGFkNzM4OTdmNTNlYjZkNDA5MDdkMmUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3Jl L2JpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtCisrKyBiL1NvdXJjZS9XZWJDb3Jl L2JpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtCkBAIC0xMDQ1LDYgKzEwNDUsMTEg QEAgRU5ECiAgICAgICAgICMgQ2hlY2sgZm9yIGEgd3JhcHBlciBpbiB0aGUgd3JhcHBlciBjYWNo ZS4gSWYgdGhlcmUgaXMgb25lLCB3ZSBrbm93IHRoYXQgYSBoaWRkZW4gcmVmZXJlbmNlIGhhcyBh bHJlYWR5CiAgICAgICAgICMgYmVlbiBjcmVhdGVkLiBJZiB3ZSBkb24ndCBmaW5kIGEgd3JhcHBl ciwgd2UgY3JlYXRlIGJvdGggYSB3cmFwcGVyIGFuZCBhIGhpZGRlbiByZWZlcmVuY2UuCiAgICAg ICAgIHB1c2goQGltcGxDb250ZW50RGVjbHMsICIgICAgUmVmUHRyPCRyZXR1cm5UeXBlPiByZXN1 bHQgPSAke2dldHRlclN0cmluZ307XG4iKTsKKyAgICAgICAgaWYgKCRyZXR1cm5UeXBlIGVxICJJ REJBbnkiKSB7CisgICAgICAgICAgICBwdXNoKEBpbXBsQ29udGVudERlY2xzLCAiICAgIGlmICgh cmVzdWx0LmdldCgpIHx8IHJlc3VsdC0+dHlwZSgpID09IElEQkFueTo6VW5kZWZpbmVkVHlwZSB8 fCByZXN1bHQtPnR5cGUoKSA9PSBJREJBbnk6Ok51bGxUeXBlIHx8IHJlc3VsdC0+dHlwZSgpID09 IElEQkFueTo6U2NyaXB0VmFsdWVUeXBlIHx8IHJlc3VsdC0+dHlwZSgpID09IElEQkFueTo6U3Ry aW5nVHlwZSB8fCByZXN1bHQtPnR5cGUoKSA9PSBJREJBbnk6OkludGVnZXJUeXBlKSB7XG4iKTsK KyAgICAgICAgICAgIHB1c2goQGltcGxDb250ZW50RGVjbHMsICIgICAgICAgIHJldHVybiB0b1Y4 KHJlc3VsdC5nZXQoKSwgaW5mby5Ib2xkZXIoKSwgaW5mby5HZXRJc29sYXRlKCkpO1xuIik7Cisg ICAgICAgICAgICBwdXNoKEBpbXBsQ29udGVudERlY2xzLCAiICAgIH1cbiIpOworICAgICAgICB9 CiAgICAgICAgIHB1c2goQGltcGxDb250ZW50RGVjbHMsICIgICAgdjg6OkhhbmRsZTx2ODo6VmFs dWU+IHdyYXBwZXIgPSByZXN1bHQuZ2V0KCkgPyB2ODo6SGFuZGxlPHY4OjpWYWx1ZT4oRE9NRGF0 YVN0b3JlOjpnZXRXcmFwcGVyKHJlc3VsdC5nZXQoKSwgaW5mby5HZXRJc29sYXRlKCkpKSA6IHY4 VW5kZWZpbmVkKCk7XG4iKTsKICAgICAgICAgcHVzaChAaW1wbENvbnRlbnREZWNscywgIiAgICBp ZiAod3JhcHBlci5Jc0VtcHR5KCkpIHtcbiIpOwogICAgICAgICBwdXNoKEBpbXBsQ29udGVudERl Y2xzLCAiICAgICAgICB3cmFwcGVyID0gdG9WOChyZXN1bHQuZ2V0KCksIGluZm8uSG9sZGVyKCks IGluZm8uR2V0SXNvbGF0ZSgpKTtcbiIpOyAjIEZJWE1FOiBDb3VsZCB1c2Ugd3JhcCBoZXJlIHNp bmNlIHRoZSB3cmFwcGVyIGlzIGVtcHR5Lgo=