| Summary: | IndexedDB: IO error when checking schema should destroy LevelDB directory | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Grogan <dgrogan> | ||||||||||
| Component: | New Bugs | Assignee: | David Grogan <dgrogan> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, alecflett, jsbell, ojan, tony, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 110677, 110820 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
David Grogan
2013-02-22 18:10:43 PST
Created attachment 189887 [details]
Patch
Josh, could you take a look at this? 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. Oops, comment 3 was supposed to go on bug 110677. 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 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. Created attachment 189900 [details]
Patch
Tony, could you review this? 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?
Created attachment 190194 [details]
Patch
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 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. Created attachment 190568 [details]
Patch
Tony, could you re-review this? Ojan, with Tony out of town could you review this? Tony r+'d a much smaller earlier version and Josh gave this an LGTM. 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 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 on attachment 190568 [details] Patch Clearing flags on attachment: 190568 Committed r144323: <http://trac.webkit.org/changeset/144323> All reviewed patches have been landed. Closing bug. 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. |