Bug 103960 – IndexedDB: Abort transactions because of leveldb errors part 3

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

[David Grogan]

IndexedDB: Abort transactions because of leveldb errors part 3

[David Grogan]

Created attachment 177395 [details]
Patch

[David Grogan]

Created attachment 177606 [details]
Patch

[David Grogan]

Josh or Alec, could you take a look?

[Joshua Bell]

Comment on attachment 177606 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:252
> +        int64_t databaseId, int64_t objectStoreId, int64_t indexId, bool& canAddKeys,

Just remove wrapping?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:253
> +        const IDBKey* primaryKey = 0, String* errorMessage = 0)

Add WARN_UNUSED_RETURN

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:282
> +        int64_t databaseId, int64_t objectStoreId, int64_t indexId,

Just remove wrapping?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:283
> +        const IDBKey* indexKey, const IDBKey* primaryKey, bool& allowed) const

Add WARN_UNUSED_RETURN

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:306
> +static bool makeIndexWriters(PassRefPtr<IDBTransactionBackendImpl> transaction, IDBObjectStoreBackendImpl* objectStore, PassRefPtr<IDBKey> primaryKey, bool keyWasGenerated, const Vector<int64_t>& indexIds, const Vector<IDBObjectStoreBackendInterface::IndexKeys>& indexKeys, Vector<OwnPtr<IndexWriter> >* indexWriters, String* errorMessage, bool& completed)

Add WARN_UNUSED_RETURN

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:329
> +        bool noLevelDBErrors = indexWriter->verifyIndexKeys(*objectStore->backingStore(),

I'd avoid using the "leveldb" name here. um... backingStoreSuccess ?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:330
> +            transaction->backingStoreTransaction(), objectStore->databaseId(),

Just remove wrapping?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:368
> +    bool completed = false;

Maybe rename to consistencySuccess ? Or something that's more clearly the inverse of constraintError ?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:371
> +        transaction->abort(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error in makeIndexWriters"));

I wouldn't name the internal method explicitly here, just as it's one more thing to keep in sync. Maybe "Backing store error updating index keys." ?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:448
> +    bool completed = false;

Rename to constraintError?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:449
> +    bool noLevelDBErrors = makeIndexWriters(transaction, objectStore.get(), key, keyWasGenerated, *indexIds, *indexKeys, &indexWriters, &errorMessage, completed);

Ditto re: "leveldb"

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:451
> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error in makeIndexWriters."));

Same as above.

[David Grogan]

Comment on attachment 177606 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:252
>> +        int64_t databaseId, int64_t objectStoreId, int64_t indexId, bool& canAddKeys,
> 
> Just remove wrapping?

check-webkit-style complained about the number of spaces. Do you know how to appease it?

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:253
>> +        const IDBKey* primaryKey = 0, String* errorMessage = 0)
> 
> Add WARN_UNUSED_RETURN

Done

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:283
>> +        const IDBKey* indexKey, const IDBKey* primaryKey, bool& allowed) const
> 
> Add WARN_UNUSED_RETURN

Done.

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:306
>> +static bool makeIndexWriters(PassRefPtr<IDBTransactionBackendImpl> transaction, IDBObjectStoreBackendImpl* objectStore, PassRefPtr<IDBKey> primaryKey, bool keyWasGenerated, const Vector<int64_t>& indexIds, const Vector<IDBObjectStoreBackendInterface::IndexKeys>& indexKeys, Vector<OwnPtr<IndexWriter> >* indexWriters, String* errorMessage, bool& completed)
> 
> Add WARN_UNUSED_RETURN

Done. Apparently it needs to go before the function here, maybe because of the static?

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:329
>> +        bool noLevelDBErrors = indexWriter->verifyIndexKeys(*objectStore->backingStore(),
> 
> I'd avoid using the "leveldb" name here. um... backingStoreSuccess ?

Better, changed.

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:368
>> +    bool completed = false;
> 
> Maybe rename to consistencySuccess ? Or something that's more clearly the inverse of constraintError ?

I went with obeysConstraints

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:371
>> +        transaction->abort(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error in makeIndexWriters"));
> 
> I wouldn't name the internal method explicitly here, just as it's one more thing to keep in sync. Maybe "Backing store error updating index keys." ?

Done.

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:448
>> +    bool completed = false;
> 
> Rename to constraintError?

went with obeysConstraints

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:449
>> +    bool noLevelDBErrors = makeIndexWriters(transaction, objectStore.get(), key, keyWasGenerated, *indexIds, *indexKeys, &indexWriters, &errorMessage, completed);
> 
> Ditto re: "leveldb"

done.

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:451
>> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error in makeIndexWriters."));
> 
> Same as above.

Done.

[David Grogan]

Created attachment 177651 [details]
Patch

[David Grogan]

Josh, I'm assuming the lack of lgtm means you want to review this again before I send it off to Tony. (Is that a generally accurate interpretation?)

[Joshua Bell]

(In reply to comment #7)
> Josh, I'm assuming the lack of lgtm means you want to review this again before I send it off to Tony. (Is that a generally accurate interpretation?)

Generally yeah but I just forgot. LGTM!

[David Grogan]

Tony, could you review this?

[WebKit Review Bot]

Comment on attachment 177651 [details]
Patch

Rejecting attachment 177651 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
set 108 lines).
Hunk #2 succeeded at 387 (offset 108 lines).
Hunk #3 succeeded at 411 (offset 108 lines).
Hunk #4 succeeded at 433 (offset 108 lines).
Hunk #5 succeeded at 473 (offset 108 lines).
Hunk #6 FAILED at 445.
1 out of 6 hunks FAILED -- saving rejects to file Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tony Chang']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/15173025

[David Grogan]

Created attachment 177849 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177849 [details]
Patch for landing

Clearing flags on attachment: 177849

Committed r136775: <http://trac.webkit.org/changeset/136775>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.