Bug 100692 – IndexedDB: Crash on checking version of corrupt backing store

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

IndexedDB: Crash on checking version of corrupt backing store

[Joshua Bell]

Created attachment 171288 [details]
Patch

[Joshua Bell]

alecflett@ - please take a look

[Joshua Bell]

See comments in webkit.org/b/99636 re: testing

The in-progress tests in crrev.com/11196029 should be expanded to include this case.

[Joshua Bell]

Created attachment 171292 [details]
Patch

[Alec Flett]

Comment on attachment 171292 [details]
Patch

LGTM

[Joshua Bell]

tony@ - r? cq?

[Tony Chang]

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?

[Joshua Bell]

Created attachment 171308 [details]
Patch for landing

[Joshua Bell]

(In reply to comment #7)
> Nit: Can we merge these into a single if?

Done, thanks.

[WebKit Review Bot]

Comment on attachment 171308 [details]
Patch for landing

Clearing flags on attachment: 171308

Committed r132848: <http://trac.webkit.org/changeset/132848>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Grogan]

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].

[Joshua Bell]

(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.)