Bug 110675 – IndexedDB: IO error when checking schema should destroy LevelDB directory

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

[David Grogan]

IndexedDB: IO error when checking schema should destroy LevelDB directory

[David Grogan]

Created attachment 189887 [details]
Patch

[David Grogan]

Josh, could you take a look at this?

[David Grogan]

Josh or Alec, could you take a look?

IO error on schema check and failed openInMemory were the last two situations caught by the fallthrough that is now histogrammed as UnknownErr.

[David Grogan]

Oops, comment 3 was supposed to go on bug 110677.

[Joshua Bell]

Comment on attachment 189887 [details]
Patch

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

lgtm

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:391
> +            if (ok && !known) {

Maybe more concise as:

} else if (!known) { 

(Not sure that's an improvement.)

[David Grogan]

Comment on attachment 189887 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:391
>> +            if (ok && !known) {
> 
> Maybe more concise as:
> 
> } else if (!known) { 
> 
> (Not sure that's an improvement.)

Done.

[David Grogan]

Created attachment 189900 [details]
Patch

[David Grogan]

Tony, could you review this?

[Tony Chang]

Comment on attachment 189900 [details]
Patch

I thought we had a unittest that truncates the level db data file.  Maybe something like that would work for this case?

[David Grogan]

Created attachment 190194 [details]
Patch

[David Grogan]

Comment on attachment 190194 [details]
Patch

Now with a unit test after some refactoring. Josh and/or Tony, could you review the new stuff?

[Joshua Bell]

Comment on attachment 190194 [details]
Patch

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

lgtm - nice work!

> Source/WebCore/ChangeLog:14
> +        (DefaultLevelDBFactory):

Add details or remove these lines in ().

> Source/WebKit/chromium/ChangeLog:13
> +        (WebCore):

Ditto.

[David Grogan]

Created attachment 190568 [details]
Patch

[David Grogan]

Tony, could you re-review this?

[David Grogan]

Ojan, with Tony out of town could you review this?

Tony r+'d a much smaller earlier version and Josh gave this an LGTM.

[David Grogan]

Adam, with Tony and Ojan out, could you review this patch?

Tony r+'d a much smaller earlier version and Josh gave this an LGTM.

[Adam Barth]

Comment on attachment 190568 [details]
Patch

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

I'm not sure about the GYP cargo culting, but the rest looks sane.

> Source/WebKit/chromium/ChangeLog:9
> +        This was cargo-culted. The component build wouldn't run otherwise.

:)

> Source/WebKit/chromium/WebKit.gyp:646
> +                                'tests/IDBCleanupOnIOErrorTest.cpp',

ugg

[WebKit Review Bot]

Comment on attachment 190568 [details]
Patch

Clearing flags on attachment: 190568

Committed r144323: <http://trac.webkit.org/changeset/144323>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Comment on attachment 190568 [details]
Patch

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

> Source/WebKit/chromium/tests/IDBCleanupOnIOErrorTest.cpp:31
> +#include <webkit/support/webkit_support.h>

To avoid the gyp cargo culting, you would need to not include this header.  I guess that means either porting CreateScopedTempDirectory to Source/WebKit/chromium/testing/ or moving this test into chromium's repo.  Anyway, not a big deal either way.