Bug 153724

Summary: Modern IDB: Cursors (still) do not keep their opening request alive
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebCore JavaScriptAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, commit-queue, ggaren, ryanhaddad
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149117    
Attachments:
Description Flags
Patch v1 none

Alexey Proskuryakov
Reported 2016-01-30 16:48:03 PST
storage/indexeddb/cursor-update-private.html hist an assertion somewhat frequently: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=storage%2Findexeddb%2Fcursor-update-private.html ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction /Volumes/Data/slave/elcapitan-debug/build/Source/WebCore/bindings/js/JSEventListener.h(118) : JSC::JSObject *WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext *) const 1 0x104dd5670 WTFCrash 2 0x10a995273 WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const 3 0x10ac1f98b WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) 4 0x10a2f1857 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow, 16ul>&) 5 0x10a2f1083 WebCore::EventTarget::fireEventListeners(WebCore::Event&) 6 0x10a7beceb WebCore::IDBEventDispatcher::dispatch(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::EventTarget>, 0ul, WTF::CrashOnOverflow, 16ul>&) This assertion means that a function was called after having been garbage collected.
Attachments
Patch v1 (26.92 KB, patch)
2016-02-01 16:37 PST, Brady Eidson 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Brady Eidson
Comment 1 2016-01-30 20:15:50 PST
This is the same thing previously seen for other cursor tests, and means that the relationship between the IDBCursor and IDBRequest is not doing enough to keep the request wrapper alive. We should skip for now (I'm not anywhere near a checkout, in case somebody else wants to get to it tonight).
Alexey Proskuryakov
Comment 2 2016-01-30 22:44:13 PST
Same on storage/indexeddb/exceptions-private.html. I will not be landing expectations today FWIW.
Brady Eidson
Comment 3 2016-02-01 09:17:59 PST
> ... hits an assertion somewhat frequently: Looking at the dashboard, it doesn't appear to be that frequent. Of course it shouldn't happen *at all*, but this is infrequent enough that I dread trying to reproduce locally to explore a cause and/or fix.
Brady Eidson
Comment 4 2016-02-01 09:18:15 PST
(In reply to comment #3) > > ... hits an assertion somewhat frequently: > > Looking at the dashboard, it doesn't appear to be that frequent. > > Of course it shouldn't happen *at all*, but this is infrequent enough that I > dread trying to reproduce locally to explore a cause and/or fix. (Dread it, but am starting on it right now)
Brady Eidson
Comment 5 2016-02-01 09:35:45 PST
Great! With a DRT input file alternating these two tests back and forth 1500 times, I can eventually hit the ASSERT. Now time to dig in and find out wtf it happens.
Brady Eidson
Comment 6 2016-02-01 09:36:10 PST
(In reply to comment #5) > Great! With a DRT input file alternating these two tests back and forth 1500 > times, I can eventually hit the ASSERT. > > Now time to dig in and find out wtf it happens. (We know what the assert means, I meant find out wtf the GC collects the wrapper)
Brady Eidson
Comment 7 2016-02-01 10:13:17 PST
In https://bugs.webkit.org/show_bug.cgi?id=153038 I made IDBCursor.idl additional opaque roots to keep the wrapper for the request alive. My current working theory is that IDBCursorWithValue.idl does *not* do the same, even though it "derives from" IDBCursor.idl. Building and testing now.
Brady Eidson
Comment 8 2016-02-01 10:27:12 PST
I've confirmed that the flaky cursors are, in fact, of the type IDBCursorWithValue. That said, adding the custom visitAdditionalChildren(SlotVisitor& visitor) did not immediately fix it.
Brady Eidson
Comment 9 2016-02-01 14:10:46 PST
I know what has to be done to fix this, but implementing that is problematic. Trying to get continued assistance from a GC expert.
Brady Eidson
Comment 10 2016-02-01 15:38:44 PST
Retitling: Modern IDB: Cursors (still) do not keep their opening request alive Patch coming soon.
Brady Eidson
Comment 11 2016-02-01 16:37:40 PST
Created attachment 270450 [details] Patch v1
WebKit Commit Bot
Comment 12 2016-02-01 16:39:37 PST
Attachment 270450 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.h:106: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.h:106: The parameter name "keyRange" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/Modules/indexeddb/client/IDBObjectStoreImpl.h:106: The parameter name "ec" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 13 2016-02-01 17:43:00 PST
Comment on attachment 270450 [details] Patch v1 Clearing flags on attachment: 270450 Committed r195997: <http://trac.webkit.org/changeset/195997>
WebKit Commit Bot
Comment 14 2016-02-01 17:43:03 PST
All reviewed patches have been landed. Closing bug.
Geoffrey Garen
Comment 15 2016-02-02 10:08:22 PST
Comment on attachment 270450 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=270450&action=review > Source/WebCore/Modules/indexeddb/client/IDBRequestImpl.cpp:158 > + m_cursorRequestNotifier = std::make_unique<ScopeGuard>([this]() { There shouldn't be any need to malloc the ScopeGuard. You can just make m_cursorRequestNotifier a data member and m_cursorRequestNotifier.enable() instead.
Brady Eidson
Comment 16 2016-02-02 11:34:42 PST
(In reply to comment #15) > Comment on attachment 270450 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270450&action=review > > > Source/WebCore/Modules/indexeddb/client/IDBRequestImpl.cpp:158 > > + m_cursorRequestNotifier = std::make_unique<ScopeGuard>([this]() { > > There shouldn't be any need to malloc the ScopeGuard. You can just make > m_cursorRequestNotifier a data member and m_cursorRequestNotifier.enable() > instead. You are, of course, totally right. Might followup today if my tree is ever clean
Note You need to log in before you can comment on or make changes to this bug.