IndexedDB: Only log to histogram on fatal errors
Created attachment 179823 [details] Patch
Josh and Alec, Are these indicative of logic errors rather than IO errors? If so I will change them to emit ConsistencyError instead of InternalError.
Comment on attachment 179823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179823&action=review My intent was that InternalError would be used to flag (1) errors encountered at runtime due to bugs, bad disks, etc (2) errors generated during development by new bugs (e.g. developer forgets to write deletion code for a new field). Separating those concepts out into distinct macros is not a bad idea (I wanted to start off simple), but at first glance most of this patch is removing cases where #2 would be detected, which seems like a bad idea... > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:-620 > - InternalError(IDBLevelDBBackingStoreReadErrorGetObjectStores); This was to handle a case where we didn't correctly delete all object store metadata when a store was deleted, so we need skip leftover records. (Feel free to improve the comment.) > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:-633 > - InternalError(IDBLevelDBBackingStoreReadErrorGetObjectStores); The logic of this method could be improved significantly by doing a series of get() calls against the store rather than walking a cursor. It would simplify the method and make it more robust. Maybe it's time to do that?
(In reply to comment #2) > Are these indicative of logic errors rather than IO errors? If so I will change them to emit ConsistencyError instead of InternalError. In a word: Yes. If these trigger, it's because either a record has mysteriously vanished from the backing store or (more likely) we have a bug where we should have added/deleted a metadata entry but didn't.
Created attachment 179865 [details] Patch
Created attachment 179866 [details] Patch
This patch is the beginning of separating those concepts. The next will take advantage of the separation by making InternalReadError and maybe InternalWriteError also record successes. This is being driven because GetRecord is responsible for 95% of the errors but we don't know if that's just because GetRecord is called 20x more than the rest of the functions. Thoughts?
The latest patch LGTM. (In reply to comment #7) > This patch is the beginning of separating those concepts. The next will take advantage of the separation by making InternalReadError and maybe InternalWriteError also record successes. > > This is being driven because GetRecord is responsible for 95% of the errors but we don't know if that's just because GetRecord is called 20x more than the rest of the functions. > > Thoughts? Sounds good. Being able to correlate with filesystem-level errors would be nice, e.g. knowing that a single write attempt is resulting in tight loop generating a million I/O errors. I guess we can iterate towards that.
Tony, could you review this?
Comment on attachment 179866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179866&action=review > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:57 > + IDBLevelDBBackingStoreReadError_DEPRECATED, > + IDBLevelDBBackingStoreWriteError_DEPRECATED, > + IDBLevelDBBackingStoreConsistencyError_DEPRECATED, Nit: I probably wouldn't name these. You could set FindKeyInIndex = 4 and have a comment here saying that 1-3 are no longer used. > Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:96 > +#define InternalReadError(location) ReportError("Read", location) > +#define InternalConsistencyError(location) ReportError("Consistency", location) > +#define InternalWriteError(location) ReportError("Write", location) Nit: It looks like most macros in WebCore are in all caps, even if it's a function.
Comment on attachment 179866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179866&action=review >> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:57 >> + IDBLevelDBBackingStoreConsistencyError_DEPRECATED, > > Nit: I probably wouldn't name these. You could set FindKeyInIndex = 4 and have a comment here saying that 1-3 are no longer used. Done. >> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:96 >> +#define InternalWriteError(location) ReportError("Write", location) > > Nit: It looks like most macros in WebCore are in all caps, even if it's a function. Fixed.
Created attachment 180021 [details] Patch for landing
Comment on attachment 180021 [details] Patch for landing Clearing flags on attachment: 180021 Committed r138072: <http://trac.webkit.org/changeset/138072>
All reviewed patches have been landed. Closing bug.