Bug 164685 – Fix exception handling in SQL database code, streamline and update code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

Fix exception handling in SQL database code, streamline and update code

[Darin Adler]

Created attachment 294630 [details]
Patch

[Darin Adler]

Created attachment 294631 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

Created attachment 294646 [details]
Patch

[Darin Adler]

Created attachment 294649 [details]
Patch

[Darin Adler]

Created attachment 294652 [details]
Patch

[Darin Adler]

Created attachment 294657 [details]
Patch

[Darin Adler]

Created attachment 294672 [details]
Patch

[Darin Adler]

Last few revisions have just been attempts to fix Windows build; otherwise this seems to be working and passing all the tests.

[Darin Adler]

Created attachment 294681 [details]
Patch

[Darin Adler]

OK, this one should now work on all platforms.

[Darin Adler]

Yes, all tests passing. Just need someone to review!

[Sam Weinig]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Committed r208672: <http://trac.webkit.org/changeset/208672>

[Darin Adler]

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.

[Ryan Haddad]

(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>