Fix exception handling in SQL database code, streamline and update code
Created attachment 294630 [details] Patch
Created attachment 294631 [details] Patch
Comment on attachment 294631 [details] Patch Attachment 294631 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2505571 Number of test failures exceeded the failure limit.
Created attachment 294638 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 294631 [details] Patch Attachment 294631 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2505601 Number of test failures exceeded the failure limit.
Created attachment 294639 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 294631 [details] Patch Attachment 294631 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2505653 Number of test failures exceeded the failure limit.
Created attachment 294641 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 294646 [details] Patch
Created attachment 294649 [details] Patch
Created attachment 294652 [details] Patch
Created attachment 294657 [details] Patch
Created attachment 294672 [details] Patch
Last few revisions have just been attempts to fix Windows build; otherwise this seems to be working and passing all the tests.
Created attachment 294681 [details] Patch
OK, this one should now work on all platforms.
Yes, all tests passing. Just need someone to review!
Comment on attachment 294681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294681&action=review > Source/WebCore/Modules/webdatabase/Database.cpp:431 > - errorMessage = "unable to open database, version mismatch, '" + m_expectedVersion + "' does not match the currentVersion of '" + currentVersion + "'"; > m_sqliteDatabase.close(); > - return false; > + return Exception { INVALID_STATE_ERR, "unable to open database, version mismatch, '" + m_expectedVersion + "' does not match the currentVersion of '" + currentVersion + "'" }; I'm unclear about why in some places you are constructing the error message with the Exception construction, and in some places above, you use a temporary. Is it when you need to access m_sqliteDatabase.lastError()? Does that need to be called before calling m_sqliteDatabase.close()? > Source/WebCore/Modules/webdatabase/DatabaseTask.cpp:48 > +static Exception isolatedCopy(Exception&& value) > +{ > + return Exception { value.code(), value.releaseMessage().isolatedCopy() }; > +} > + > +static ExceptionOr<void> isolatedCopy(ExceptionOr<void>&& value) > +{ > + if (value.hasException()) > + return isolatedCopy(value.releaseException()); > + return { }; > +} Seems like these might be useful elsewhere at somepoint, and I doubt we will remember to look here for them. Perhaps they should go in ExceptionOr now. > Source/WebCore/Modules/webdatabase/DatabaseThread.cpp:159 > +void DatabaseThread::scheduleTask(std::unique_ptr<DatabaseTask>&& task) Does adding the r-value reference for a move-only type add anything? (Answering my own question, http://scottmeyers.blogspot.com/2014/07/should-move-only-types-ever-be-passed.html says...maybe? But that was a few years ago). I'm curious where Anders stands on this. > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:552 > + auto* nameMap = m_openDatabaseMap->get(&origin); > + if (!nameMap) { > + nameMap = new DatabaseNameMap; > + m_openDatabaseMap->add(origin.isolatedCopy(), nameMap); > + } This is an interesting double hash lookup idiom. Maybe we need a form of ensure that let's you allocate a new key on lookup failure as well.
Comment on attachment 294681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294681&action=review >> Source/WebCore/Modules/webdatabase/Database.cpp:431 >> + return Exception { INVALID_STATE_ERR, "unable to open database, version mismatch, '" + m_expectedVersion + "' does not match the currentVersion of '" + currentVersion + "'" }; > > I'm unclear about why in some places you are constructing the error message with the Exception construction, and in some places above, you use a temporary. Is it when you need to access m_sqliteDatabase.lastError()? Does that need to be called before calling m_sqliteDatabase.close()? Yes, that was my assumption, though I did not check that assumption. >> Source/WebCore/Modules/webdatabase/DatabaseTask.cpp:48 >> +} > > Seems like these might be useful elsewhere at somepoint, and I doubt we will remember to look here for them. Perhaps they should go in ExceptionOr now. OK, I will move them. I was thinking that what we really want is to implement isolatedCopy using templates and visit. >> Source/WebCore/Modules/webdatabase/DatabaseThread.cpp:159 >> +void DatabaseThread::scheduleTask(std::unique_ptr<DatabaseTask>&& task) > > Does adding the r-value reference for a move-only type add anything? (Answering my own question, http://scottmeyers.blogspot.com/2014/07/should-move-only-types-ever-be-passed.html says...maybe? But that was a few years ago). I'm curious where Anders stands on this. I asked this a long time ago, and someone told me we should use rvalue references. I think we were talking about std::function at the time: I mistakenly thought that was a move-only type. >> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:552 >> + } > > This is an interesting double hash lookup idiom. Maybe we need a form of ensure that let's you allocate a new key on lookup failure as well. Yes, I found this irritating. But also, isolatedCopy seems so fragile anyway. So hard to not forget to do it. There must be bugs about isolatedCopy and empty strings some places.
Comment on attachment 294681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294681&action=review >>> Source/WebCore/Modules/webdatabase/DatabaseTask.cpp:48 >>> +} >> >> Seems like these might be useful elsewhere at somepoint, and I doubt we will remember to look here for them. Perhaps they should go in ExceptionOr now. > > OK, I will move them. I was thinking that what we really want is to implement isolatedCopy using templates and visit. Annoying, if I want to move them now then I have to either make them inline functions, or create .cpp files. Made them inlines.
Committed r208672: <http://trac.webkit.org/changeset/208672>
Comment on attachment 294681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294681&action=review >>> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:552 >>> + } >> >> This is an interesting double hash lookup idiom. Maybe we need a form of ensure that let's you allocate a new key on lookup failure as well. > > Yes, I found this irritating. But also, isolatedCopy seems so fragile anyway. So hard to not forget to do it. There must be bugs about isolatedCopy and empty strings some places. We do have a way to do this kind of thing, the version of add that takes a HashTranslator, although we don’t have a version of ensure that supports it.
(In reply to comment #21) > Committed r208672: <http://trac.webkit.org/changeset/208672> It appears that this change has caused a reproducible LayoutTest crash under GuardMalloc and ASan. See <rdar://problem/29274545>