Bug 85579 – IndexedDB: Fire error when there are problems opening a DB

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

[David Grogan]

IndexedDB: Fire error when there are problems opening a DB

[David Grogan]

Created attachment 140156 [details]
Patch

[David Grogan]

Josh and Alec, could you take a look at this?

There were a few places where we were firing success on open when we shouldn't have, this changes them to error or abort as appropriate. This won't help our proximate issues but it _might_ lessen some confusion.

I have a similar patch that surfaces errors when deleting object stores but that seems less relevant.

[Joshua Bell]

Comment on attachment 140156 [details]
Patch

lgtm

[David Grogan]

Tony, sorry to send another test-less patch, but could you review this?

[Tony Chang]

Comment on attachment 140156 [details]
Patch

Would it be possible to write a unittest that truncates the file?  I guess the location of the file is unknown.

A more involved solution would be to add a method to WebCore/testing/Internals.idl that corrupts the file, but a pyauto test is OK too.

[Joshua Bell]

Source/WebKit/chromium/tests/ might be another place to add tests - LevelDBTest.cpp there creates and corrupts a database. If IDBDatabaseBackendImpl's backing store could be pointed at an arbitrary directory, that might work.

[David Grogan]

Created attachment 142654 [details]
Patch

[David Grogan]

Created attachment 142655 [details]
Patch

[David Grogan]

Josh, I added a unit test with the fake backing store from your other patch.  Could you take a look?

[Joshua Bell]

Comment on attachment 142655 [details]
Patch

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

Overall LGTM

> Source/WebKit/chromium/ChangeLog:29
> +        * tests/IDBFakeBackingStore.h: Copied from Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.h.

Was that comment auto-generated?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:50
> +        return adoptRef(new IDBDatabaseBackendImpl(name, database, coordinator, factory, uniqueIdentifier, success));

Can IDBDatabaseBackendImpl::create() return null if the constructor sets success to false? Then the success parameter could be removed from create(). On the other hand, XXX::create() doesn't usually fail so perhaps an explicit flag is clearer, but having an unusable object returned also feels usafe.

> Source/WebKit/chromium/WebKit.gypi:109
> +            'tests/IDBAbortOnCorruptTest.cpp',

tests/IDBFakeBackingStore.h should be included as well.

> Source/WebKit/chromium/tests/IDBAbortOnCorruptTest.cpp:14
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

s/APPLE/GOOGLE/ ?

> Source/WebKit/chromium/tests/IDBFakeBackingStore.h:14
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

s/APPLE/GOOGLE/ ?

[David Grogan]

Created attachment 143088 [details]
Patch

[David Grogan]

Comment on attachment 142655 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:29
>> +        * tests/IDBFakeBackingStore.h: Copied from Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.h.
> 
> Was that comment auto-generated?

Yes

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:50
>> +        return adoptRef(new IDBDatabaseBackendImpl(name, database, coordinator, factory, uniqueIdentifier, success));
> 
> Can IDBDatabaseBackendImpl::create() return null if the constructor sets success to false? Then the success parameter could be removed from create(). On the other hand, XXX::create() doesn't usually fail so perhaps an explicit flag is clearer, but having an unusable object returned also feels usafe.

I gave this a shot, I think I like it more than the old way.

>> Source/WebKit/chromium/WebKit.gypi:109
>> +            'tests/IDBAbortOnCorruptTest.cpp',
> 
> tests/IDBFakeBackingStore.h should be included as well.

Done.

>> Source/WebKit/chromium/tests/IDBAbortOnCorruptTest.cpp:14
>> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> 
> s/APPLE/GOOGLE/ ?

All the other files have APPLE, so this is probably right.  Tony will know.

[David Grogan]

Tony, could you take a look at this?

[Tony Chang]

Comment on attachment 143088 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:55
> +        bool success = false;
> +        IDBDatabaseBackendImpl* backend = new IDBDatabaseBackendImpl(name, database, coordinator, factory, uniqueIdentifier, success);
> +        if (!success) {
> +          delete backend;
> +          return 0;
> +        }

Using an out param for success in a constructor is weird.  Can we just call openInternal here after the object is constructed?

[David Grogan]

Created attachment 143113 [details]
Patch for landing

[David Grogan]

(In reply to comment #14)
> Using an out param for success in a constructor is weird.  Can we just call openInternal here after the object is constructed?

Agreed. I think so, and done.

[Eric Seidel (no email)]

Comment on attachment 143088 [details]
Patch

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

Sorry, i'm not a great reviwer for this, but here are some drive-bys:

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:106
> -    openInternal();
> +    success = openInternal();

Why not just move the openInternal call out of the constructor?  Then you don't need the success callback, and the create() function is responsible for calling openInternal?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:117
>      bool success = m_backingStore->getIDBDatabaseMetaData(m_name, m_version, m_id);
>      ASSERT(success == (m_id != InvalidId));
>      if (success) {
>          loadObjectStores();
> -        return;
> +        return true;
>      }
> -    if (!m_backingStore->createIDBDatabaseMetaData(m_name, m_version, m_id))
> -        ASSERT_NOT_REACHED(); // FIXME: Need better error handling.
> +    return m_backingStore->createIDBDatabaseMetaData(m_name, m_version, m_id);

Alternatively, one could do this work up front, and then pass in the relevant structure to the DatabaseBackendImpl in the constructor (which could then never fail).

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:51
> +        IDBDatabaseBackendImpl* backend = new IDBDatabaseBackendImpl(name, database, coordinator, factory, uniqueIdentifier, success);

Better to put it in an OwnPtr right away, then you don't need a delete below.  WHy is this inline?  The malloc surely out-weighs the inline saving.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:86
> +    IDBDatabaseBackendImpl(const String& name, IDBBackingStore* database, IDBTransactionCoordinator*, IDBFactoryBackendImpl*, const String& uniqueIdentifier, bool& success);

A success parameter to a constructor is a kinda odd idium.  Should we be allocating some substructure first in a separate create method which could return 0 when it fails?  I'm not sure what you're using success for here.  It's not wrong in my eyes... just a bit odd.

[David Grogan]

Created attachment 143124 [details]
Patch

[Tony Chang]

Comment on attachment 143124 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:100
> +        backend.clear();

Nit: This shouldn't be necessary since backend is a RefPtr.

> Source/WebKit/chromium/tests/IDBAbortOnCorruptTest.cpp:40
> + public:

Nit: No space before public:.

[David Grogan]

(In reply to comment #17)
> (From update of attachment 143088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143088&action=review
> 
> Sorry, i'm not a great reviwer for this, but here are some drive-bys:
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:106
> > -    openInternal();
> > +    success = openInternal();
> 
> Why not just move the openInternal call out of the constructor?  Then you don't need the success callback, and the create() function is responsible for calling openInternal?

An idea so good it's already done.

> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:117
> >      bool success = m_backingStore->getIDBDatabaseMetaData(m_name, m_version, m_id);
> >      ASSERT(success == (m_id != InvalidId));
> >      if (success) {
> >          loadObjectStores();
> > -        return;
> > +        return true;
> >      }
> > -    if (!m_backingStore->createIDBDatabaseMetaData(m_name, m_version, m_id))
> > -        ASSERT_NOT_REACHED(); // FIXME: Need better error handling.
> > +    return m_backingStore->createIDBDatabaseMetaData(m_name, m_version, m_id);
> 
> Alternatively, one could do this work up front, and then pass in the relevant structure to the DatabaseBackendImpl in the constructor (which could then never fail).

Good point that I hadn't considered, but in this case splitting up openInternal or moving it outside the class would probably be more trouble than it's worth.

> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:51
> > +        IDBDatabaseBackendImpl* backend = new IDBDatabaseBackendImpl(name, database, coordinator, factory, uniqueIdentifier, success);
> 
> Better to put it in an OwnPtr right away, then you don't need a delete below.  WHy is this inline?

I don't know. Moved.

The malloc surely out-weighs the inline saving.
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:86
> > +    IDBDatabaseBackendImpl(const String& name, IDBBackingStore* database, IDBTransactionCoordinator*, IDBFactoryBackendImpl*, const String& uniqueIdentifier, bool& success);
> 
> A success parameter to a constructor is a kinda odd idium.  Should we be allocating some substructure first in a separate create method which could return 0 when it fails?  I'm not sure what you're using success for here.  It's not wrong in my eyes... just a bit odd.

Tony had the same feeling, this is all changed.

[David Grogan]

Created attachment 143131 [details]
Patch for landing

[David Grogan]

(In reply to comment #19)
> (From update of attachment 143124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143124&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:100
> > +        backend.clear();
> 
> Nit: This shouldn't be necessary since backend is a RefPtr.
> 
> > Source/WebKit/chromium/tests/IDBAbortOnCorruptTest.cpp:40
> > + public:
> 
> Nit: No space before public:.

Fixed and fixed.

[WebKit Review Bot]

Comment on attachment 143131 [details]
Patch for landing

Rejecting attachment 143131 [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:
l>&, WTF::Vector<WebCore::IDBKeyPath, 0ul>&, WTF::Vector<bool, 0ul>&, WTF::Vector<bool, 0ul>&)
Source/WebCore/Modules/indexeddb/IDBBackingStore.h:81: note: 	virtual bool WebCore::IDBBackingStore::createIndex(int64_t, int64_t, const WTF::String&, const WebCore::IDBKeyPath&, bool, bool, int64_t&)
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/IDBAbortOnCorruptTest.o] Error 1
make: *** Waiting for unfinished jobs....
  LINK(target) out/Debug/DumpRenderTree: Finished

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

[David Grogan]

Created attachment 143302 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 143302 [details]
Patch for landing

Clearing flags on attachment: 143302

Committed r117978: <http://trac.webkit.org/changeset/117978>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.