Bug 105222 – IndexedDB: Split BackingStore histogram

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: Only log to histogram on fatal errors

[David Grogan]

Created attachment 179823 [details]
Patch

[David Grogan]

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.

[Joshua Bell]

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?

[Joshua Bell]

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

[David Grogan]

Created attachment 179865 [details]
Patch

[David Grogan]

Created attachment 179866 [details]
Patch

[David Grogan]

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?

[Joshua Bell]

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.

[David Grogan]

Tony, could you review this?

[Tony Chang]

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.

[David Grogan]

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.

[David Grogan]

Created attachment 180021 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 180021 [details]
Patch for landing

Clearing flags on attachment: 180021

Committed r138072: <http://trac.webkit.org/changeset/138072>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.