Bug 85224 - IndexedDB: stale index entries may not be removed in some cases
Summary: IndexedDB: stale index entries may not be removed in some cases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-30 13:01 PDT by dstockwell
Modified: 2012-05-02 08:52 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.32 KB, patch)
2012-04-30 13:05 PDT, dstockwell
no flags Details | Formatted Diff | Diff
Patch (2.33 KB, patch)
2012-04-30 13:08 PDT, dstockwell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dstockwell 2012-04-30 13:01:26 PDT
Stale entries in indexes are removed on iteration when a version mismatch is detected. However, this currently only happens when versions are different (when an entry is overwitten). If the key is removed from the object store, the index entry will not be removed on iteration. Over time this causes a significant degradation in index performance.
Comment 1 dstockwell 2012-04-30 13:05:52 PDT
Created attachment 139503 [details]
Patch
Comment 2 dstockwell 2012-04-30 13:08:45 PDT
Created attachment 139504 [details]
Patch
Comment 3 Alec Flett 2012-05-01 09:33:36 PDT
lgtm - emphasizes how much duplicated code there is between IndexCursorImpl::loadCurrentRow() and IndexKeyCursorImpl::loadCurrentRow() - ripe for a refactor.

I wonder, while you're there, if any other place where we return false, if we should be removing as well - I mean is there any way that once we find bad data, that the data will ever not be bad, and will just slow down the index as it already has for you? 

On the other hand, if there is data that isn't really corrupt but is actually just buggy and readable by future versions of chrome, we should leave it in case we can recover it later.

Though on the other other hand, that means that a software upgrade could make data suddenly appear that wasn't there before. Huh. something to think about for a refactor.
Comment 4 dstockwell 2012-05-01 10:27:06 PDT
(In reply to comment #3)

> I wonder, while you're there, if any other place where we return false, if we should be removing as well - I mean is there any way that once we find bad data, that the data will ever not be bad, and will just slow down the index as it already has for you? 
> 
> On the other hand, if there is data that isn't really corrupt but is actually just buggy and readable by future versions of chrome, we should leave it in case we can recover it later.
> 
> Though on the other other hand, that means that a software upgrade could make data suddenly appear that wasn't there before. Huh. something to think about for a refactor.

Yes, definitely needs some thought before we make such a change.
Comment 5 dstockwell 2012-05-01 10:44:28 PDT
Ojan, r?
Comment 6 WebKit Review Bot 2012-05-01 12:04:40 PDT
Comment on attachment 139504 [details]
Patch

Clearing flags on attachment: 139504

Committed r115743: <http://trac.webkit.org/changeset/115743>
Comment 7 WebKit Review Bot 2012-05-01 12:04:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Tony Chang 2012-05-01 13:07:55 PDT
(In reply to comment #5)
> Ojan, r?

Hah, I've transferred indexeddb reviews to Ojan!
Comment 9 Alec Flett 2012-05-02 08:52:15 PDT
filed bug 85293 about the refactoring