Bug 110675 - IndexedDB: IO error when checking schema should destroy LevelDB directory
Summary: IndexedDB: IO error when checking schema should destroy LevelDB directory
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: David Grogan
URL:
Keywords:
Depends on:
Blocks: 110677 110820
  Show dependency tree / graph
 
Reported: 2013-02-22 18:10 PST by David Grogan
Modified: 2013-03-04 11:41 PST (History)
6 users (show)

See Also:

Attachments
Patch (2.14 KB, patch)
2013-02-22 18:12 PST, David Grogan 		no flags Details | Formatted Diff | Diff
Patch (2.14 KB, patch)
2013-02-22 19:45 PST, David Grogan 		no flags Details | Formatted Diff | Diff
Patch (14.07 KB, patch)
2013-02-25 20:33 PST, David Grogan 		no flags Details | Formatted Diff | Diff
Patch (13.56 KB, patch)
2013-02-27 11:51 PST, David Grogan 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2013-02-22 18:10:43 PST 
IndexedDB: IO error when checking schema should destroy LevelDB directory
Comment 1 David Grogan 2013-02-22 18:12:30 PST 
Created attachment 189887 [details]
Patch
Comment 2 David Grogan 2013-02-22 18:17:14 PST 
Josh, could you take a look at this?
Comment 3 David Grogan 2013-02-22 18:35:29 PST 
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.
Comment 4 David Grogan 2013-02-22 18:43:29 PST 
Oops, comment 3 was supposed to go on bug 110677.
Comment 5 Joshua Bell 2013-02-22 19:36:38 PST 
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.)
Comment 6 David Grogan 2013-02-22 19:45:10 PST 
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.
Comment 7 David Grogan 2013-02-22 19:45:32 PST 
Created attachment 189900 [details]
Patch
Comment 8 David Grogan 2013-02-22 19:46:18 PST 
Tony, could you review this?
Comment 9 Tony Chang 2013-02-25 09:46:43 PST 
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?
Comment 10 David Grogan 2013-02-25 20:33:16 PST 
Created attachment 190194 [details]
Patch
Comment 11 David Grogan 2013-02-25 20:36:33 PST 
Comment on attachment 190194 [details]
Patch

Now with a unit test after some refactoring. Josh and/or Tony, could you review the new stuff?
Comment 12 Joshua Bell 2013-02-27 11:33:47 PST 
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.
Comment 13 David Grogan 2013-02-27 11:51:24 PST 
Created attachment 190568 [details]
Patch
Comment 14 David Grogan 2013-02-27 11:52:24 PST 
Tony, could you re-review this?
Comment 15 David Grogan 2013-02-27 12:07:54 PST 
Ojan, with Tony out of town could you review this?

Tony r+'d a much smaller earlier version and Josh gave this an LGTM.
Comment 16 David Grogan 2013-02-27 13:58:56 PST 
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.
Comment 17 Adam Barth 2013-02-28 00:37:29 PST 
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
Comment 18 WebKit Review Bot 2013-02-28 10:13:18 PST 
Comment on attachment 190568 [details]
Patch

Clearing flags on attachment: 190568

Committed r144323: <http://trac.webkit.org/changeset/144323>
Comment 19 WebKit Review Bot 2013-02-28 10:13:23 PST 
All reviewed patches have been landed.  Closing bug.
Comment 20 Tony Chang 2013-03-04 11:41:28 PST 
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.