Bug 165978 – IndexedDB: Refactor SQLiteIDBCursor to prepare for cursor prefetching

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

IndexedDB: Refactor SQLiteIDBCursor to prepare for cursor prefetching

This patch won't actually add prefetching, but it puts all of the necessary pieces in place for it (including managing a "fetched record" queue of size 1)

[Brady Eidson]

Created attachment 297376 [details]
Patch v1

[Alex Christensen]

Comment on attachment 297376 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:222
> +        m_currentLowerKey = m_fetchedRecords.first().record.key;

All the calls to Deque::first except these two have assertions that the deque is not empty.  Deque::first has an internal assumption that it's not empty.  Should you add more assertions here or remove the others?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:317
> +    for (size_t i = 0; i < count && !m_fetchedRecords.isEmpty(); ++i) {
> +        if (m_fetchedRecords.first().isTerminalRecord())

I don't see any bounds checking on the value of count other than making sure it's nonzero.  Are there tests that try to advance the cursor more than it can or put a negative/non-integer value into IDBCursor.advance?  I don't see any guarantee that this will continue to have a first value for the entire duration of this for loop.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:350
> +    ASSERT(m_fetchedRecords.isEmpty() || !m_fetchedRecords.last().isTerminalRecord());

If m_fetchedRecords is empty, m_fetchedRecords.last() will assert and read memory it shouldn't

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 297376 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297376&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:222
> > +        m_currentLowerKey = m_fetchedRecords.first().record.key;
> 
> All the calls to Deque::first except these two have assertions that the
> deque is not empty.  Deque::first has an internal assumption that it's not
> empty.  Should you add more assertions here or remove the others?

Some places that don't have ASSERTs *do* have "if (!deque.isEmpty())" checks beforehand.

Each site that calls to first() should have *either* an ASSERT or an appropriate isEmpty() check.

I will go through and make sure they all have at least one of those.

> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:317
> > +    for (size_t i = 0; i < count && !m_fetchedRecords.isEmpty(); ++i) {
> > +        if (m_fetchedRecords.first().isTerminalRecord())
> 
> I don't see any bounds checking on the value of count other than making sure
> it's nonzero.  Are there tests that try to advance the cursor more than it
> can or put a negative/non-integer value into IDBCursor.advance? 

Yes. Negative counts fail at the bindings level and "advance more than it can" is spec'ed behavior.

e.g., if you advance 50 and there's only 4 left, you simply reach the end.

> I don't see any guarantee that this will continue to have a first value for the entire
> duration of this for loop.

The guarantee is in the for clause: ` i < count && !m_fetchedRecords.isEmpty();`

By making non-emptiness of the deque an invariant of the loop.

> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBCursor.cpp:350
> > +    ASSERT(m_fetchedRecords.isEmpty() || !m_fetchedRecords.last().isTerminalRecord());
> 
> If m_fetchedRecords is empty, m_fetchedRecords.last() will assert and read
> memory it shouldn't

Agreed, but this assertion never does that; If the m_fetchedRecords.last() code runs, the deque is definitely not empty.

[Brady Eidson]

Created attachment 297391 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297391 [details]
Patch

Clearing flags on attachment: 297391

Committed r209960: <http://trac.webkit.org/changeset/209960>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.