IndexedDB: Fire error when there are problems opening a DB
Created attachment 140156 [details] Patch
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 on attachment 140156 [details] Patch lgtm
Tony, sorry to send another test-less patch, but could you review this?
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.
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.
Created attachment 142654 [details] Patch
Created attachment 142655 [details] Patch
Josh, I added a unit test with the fake backing store from your other patch. Could you take a look?
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/ ?
Created attachment 143088 [details] Patch
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.
Tony, could you take a look at this?
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?
Created attachment 143113 [details] Patch for landing
(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 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.
Created attachment 143124 [details] Patch
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:.
(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.
Created attachment 143131 [details] Patch for landing
(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 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
Created attachment 143302 [details] Patch for landing
Comment on attachment 143302 [details] Patch for landing Clearing flags on attachment: 143302 Committed r117978: <http://trac.webkit.org/changeset/117978>
All reviewed patches have been landed. Closing bug.