Bug 153038 – Modern IDB: A few cursor tests are flaky because JS wrappers are GC'ed

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Modern IDB: A few cursor tests are flaky because JS wrappers are GC'ed

storage/indexeddb/cursor-update.html
storage/indexeddb/mutating-cursor.html

https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK1%20(Tests)/r194903%20(11144)/results.html
https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK1%20(Tests)/r194901%20(11142)/results.html

You can see on the flakiness dashboard that on debug bots they are crashes:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=storage%2Findexeddb%2Fmutating-cursor.html

But really ASSERTs.

I verified running these tests repeatedly with some logging that the IDBRequest answered "false" to hasPendingActivity(), leading to its JS wrapper to get GC'ed, but then an event was delivered.

The ASSERTs in debug builds are just that: 
inline JSC::JSObject* JSEventListener::jsFunction(ScriptExecutionContext* scriptExecutionContext) const
...
    ASSERT(!m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction);

IDBRequests for cursors operations can be reused and must maintain a "true" answer to this question while a cursor operation is outstanding.

[Brady Eidson]

Created attachment 268871 [details]
Patch v1

[Alex Christensen]

Comment on attachment 268871 [details]
Patch v1

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

r=me

> Source/WebCore/bindings/js/JSIDBCursorCustom.cpp:42
> +    auto& modernCursor = static_cast<IDBClient::IDBCursor&>(wrapped());

Is this static_cast necessary?  Other visitAdditionalChildren implementations don't have it.

[Alex Christensen]

Comment on attachment 268871 [details]
Patch v1

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

>> Source/WebCore/bindings/js/JSIDBCursorCustom.cpp:42
>> +    auto& modernCursor = static_cast<IDBClient::IDBCursor&>(wrapped());
> 
> Is this static_cast necessary?  Other visitAdditionalChildren implementations don't have it.

Ok, but this will be removed when we get rid of the legacy implementation.  Same with isModernCursor.

[Brady Eidson]

http://trac.webkit.org/changeset/194967