Bug 165978 - IndexedDB: Refactor SQLiteIDBCursor to prepare for cursor prefetching
Summary: IndexedDB: Refactor SQLiteIDBCursor to prepare for cursor prefetching
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 160306 165928
  Show dependency treegraph
 
Reported: 2016-12-16 16:52 PST by Brady Eidson
Modified: 2016-12-16 23:29 PST (History)
4 users (show)

See Also:


Attachments
Patch v1 (21.67 KB, patch)
2016-12-16 17:00 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (22.08 KB, patch)
2016-12-16 22:45 PST, Brady Eidson
no flags 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-12-16 16:52:26 PST
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)
Comment 1 Brady Eidson 2016-12-16 17:00:05 PST
Created attachment 297376 [details]
Patch v1
Comment 2 Alex Christensen 2016-12-16 18:22:29 PST
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
Comment 3 Brady Eidson 2016-12-16 18:39:44 PST
(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.
Comment 4 Brady Eidson 2016-12-16 22:45:24 PST
Created attachment 297391 [details]
Patch
Comment 5 WebKit Commit Bot 2016-12-16 23:29:55 PST
Comment on attachment 297391 [details]
Patch

Clearing flags on attachment: 297391

Committed r209960: <http://trac.webkit.org/changeset/209960>
Comment 6 WebKit Commit Bot 2016-12-16 23:29:58 PST
All reviewed patches have been landed.  Closing bug.