Bug 100692

Summary: IndexedDB: Crash on checking version of corrupt backing store
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: New BugsAssignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dgrogan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 99636    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Joshua Bell 2012-10-29 12:09:42 PDT 
IndexedDB: Crash on checking version of corrupt backing store
Comment 1 Joshua Bell 2012-10-29 12:11:28 PDT 
Created attachment 171288 [details]
Patch
Comment 2 Joshua Bell 2012-10-29 12:12:10 PDT 
alecflett@ - please take a look
Comment 3 Joshua Bell 2012-10-29 12:19:53 PDT 
See comments in webkit.org/b/99636 re: testing

The in-progress tests in crrev.com/11196029 should be expanded to include this case.
Comment 4 Joshua Bell 2012-10-29 12:21:58 PDT 
Created attachment 171292 [details]
Patch
Comment 5 Alec Flett 2012-10-29 13:41:13 PDT 
Comment on attachment 171292 [details]
Patch

LGTM
Comment 6 Joshua Bell 2012-10-29 13:41:40 PDT 
tony@ - r? cq?
Comment 7 Tony Chang 2012-10-29 13:51:40 PDT 
Comment on attachment 171292 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:299
> +        if (db) {
> +            if (!isSchemaKnown(db.get())) {

Nit: Can we merge these into a single if?
Comment 8 Joshua Bell 2012-10-29 14:00:09 PDT 
Created attachment 171308 [details]
Patch for landing
Comment 9 Joshua Bell 2012-10-29 14:00:29 PDT 
(In reply to comment #7)
> Nit: Can we merge these into a single if?

Done, thanks.
Comment 10 WebKit Review Bot 2012-10-29 14:20:32 PDT 
Comment on attachment 171308 [details]
Patch for landing

Clearing flags on attachment: 171308

Committed r132848: <http://trac.webkit.org/changeset/132848>
Comment 11 WebKit Review Bot 2012-10-29 14:20:35 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 12 David Grogan 2012-11-06 15:07:06 PST 
Comment on attachment 171308 [details]
Patch for landing

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

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:301
> +            db.release();

Is there a reason that this is db.release() instead of db.clear()?  AFAICT they would act identically here, just wondering if [release() was arbitrary] or [a conscious choice and I'm missing something].
Comment 13 Joshua Bell 2012-11-06 15:14:48 PST 
(In reply to comment #12)
> (From update of attachment 171308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171308&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:301
> > +            db.release();
> 
> Is there a reason that this is db.release() instead of db.clear()?  AFAICT they would act identically here, just wondering if [release() was arbitrary] or [a conscious choice and I'm missing something].

Not a conscious choice. I agree it should be clear(). (I noticed that later and have that change in a branch somewhere, apparently not uploaded anywhere.)