Modern IDB: Cursors (still) do not keep their opening request alive

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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.

[Brady Eidson]

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]

Same on storage/indexeddb/exceptions-private.html. I will not be landing expectations today FWIW.

[Brady Eidson]

> ... 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]

(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]

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]

(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]

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]

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]

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]

Retitling:
Modern IDB: Cursors (still) do not keep their opening request alive

Patch coming soon.

[Brady Eidson]

Created attachment 270450 [details]
Patch v1

[WebKit Commit Bot]

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 on attachment 270450 [details]
Patch v1

Clearing flags on attachment: 270450

Committed r195997: <http://trac.webkit.org/changeset/195997>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

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]

(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