Bug 153038 - Modern IDB: A few cursor tests are flaky because JS wrappers are GC'ed
Summary: Modern IDB: A few cursor tests are flaky because JS wrappers are GC'ed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117
  Show dependency treegraph
 
Reported: 2016-01-12 13:58 PST by Brady Eidson
Modified: 2016-01-13 10:42 PST (History)
1 user (show)

See Also:


Attachments
Patch v1 (11.76 KB, patch)
2016-01-13 09:11 PST, Brady Eidson
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-01-12 13:58:32 PST
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.
Comment 1 Brady Eidson 2016-01-13 09:11:50 PST
Created attachment 268871 [details]
Patch v1
Comment 2 Alex Christensen 2016-01-13 10:30:01 PST
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.
Comment 3 Alex Christensen 2016-01-13 10:37:14 PST
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.
Comment 4 Brady Eidson 2016-01-13 10:42:31 PST
http://trac.webkit.org/changeset/194967