Bug 85579 - IndexedDB: Fire error when there are problems opening a DB
Summary: IndexedDB: Fire error when there are problems opening a DB
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-03 21:44 PDT by David Grogan
Modified: 2012-05-22 08:54 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.94 KB, patch)
2012-05-03 21:46 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (22.90 KB, patch)
2012-05-18 00:35 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (22.80 KB, patch)
2012-05-18 00:38 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (22.45 KB, patch)
2012-05-21 13:32 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch for landing (21.59 KB, patch)
2012-05-21 15:19 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (22.40 KB, patch)
2012-05-21 16:10 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch for landing (22.37 KB, patch)
2012-05-21 16:32 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch for landing (22.31 KB, patch)
2012-05-22 08:10 PDT, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-05-03 21:44:13 PDT
IndexedDB: Fire error when there are problems opening a DB
Comment 1 David Grogan 2012-05-03 21:46:54 PDT
Created attachment 140156 [details]
Patch
Comment 2 David Grogan 2012-05-03 22:26:01 PDT
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.
Comment 3 Joshua Bell 2012-05-04 14:27:56 PDT
Comment on attachment 140156 [details]
Patch

lgtm
Comment 4 David Grogan 2012-05-04 14:57:36 PDT
Tony, sorry to send another test-less patch, but could you review this?
Comment 5 Tony Chang 2012-05-04 15:25:15 PDT
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.
Comment 6 Joshua Bell 2012-05-04 15:28:24 PDT
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.
Comment 7 David Grogan 2012-05-18 00:35:44 PDT
Created attachment 142654 [details]
Patch
Comment 8 David Grogan 2012-05-18 00:38:39 PDT
Created attachment 142655 [details]
Patch
Comment 9 David Grogan 2012-05-18 00:40:53 PDT
Josh, I added a unit test with the fake backing store from your other patch.  Could you take a look?
Comment 10 Joshua Bell 2012-05-18 12:21:14 PDT
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/ ?
Comment 11 David Grogan 2012-05-21 13:32:08 PDT
Created attachment 143088 [details]
Patch
Comment 12 David Grogan 2012-05-21 13:33:13 PDT
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.
Comment 13 David Grogan 2012-05-21 14:19:15 PDT
Tony, could you take a look at this?
Comment 14 Tony Chang 2012-05-21 14:37:07 PDT
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?
Comment 15 David Grogan 2012-05-21 15:19:31 PDT
Created attachment 143113 [details]
Patch for landing
Comment 16 David Grogan 2012-05-21 15:21:27 PDT
(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.
Comment 17 Eric Seidel (no email) 2012-05-21 15:28:08 PDT
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.
Comment 18 David Grogan 2012-05-21 16:10:00 PDT
Created attachment 143124 [details]
Patch
Comment 19 Tony Chang 2012-05-21 16:14:39 PDT
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:.
Comment 20 David Grogan 2012-05-21 16:31:34 PDT
(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.
Comment 21 David Grogan 2012-05-21 16:32:16 PDT
Created attachment 143131 [details]
Patch for landing
Comment 22 David Grogan 2012-05-21 16:33:19 PDT
(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.
Comment 23 WebKit Review Bot 2012-05-21 22:59:08 PDT
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
Comment 24 David Grogan 2012-05-22 08:10:02 PDT
Created attachment 143302 [details]
Patch for landing
Comment 25 WebKit Review Bot 2012-05-22 08:54:32 PDT
Comment on attachment 143302 [details]
Patch for landing

Clearing flags on attachment: 143302

Committed r117978: <http://trac.webkit.org/changeset/117978>
Comment 26 WebKit Review Bot 2012-05-22 08:54:37 PDT
All reviewed patches have been landed.  Closing bug.