Bug 170000 - A null compound index value crashes the Databases process
Summary: A null compound index value crashes the Databases process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari Technology Preview
Hardware: All Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-23 03:12 PDT by Casey
Modified: 2017-03-24 14:13 PDT (History)
7 users (show)

See Also:


Attachments
com.apple.WebKit.Databases crash log (46.84 KB, text/plain)
2017-03-23 08:55 PDT, Casey
no flags Details
Fairly reduced test case (696 bytes, text/html)
2017-03-24 10:13 PDT, Brady Eidson
no flags Details
Patch (8.63 KB, patch)
2017-03-24 13:13 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (11.34 KB, patch)
2017-03-24 13:18 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Casey 2017-03-23 03:12:37 PDT
Posting this here at the recommendation of @beidson here: https://github.com/dfahlander/Dexie.js/issues/494

----

If you specify a compound index, and then subsequently attempt to store a record in IDB and the value for that index is null, Safari >= 10.1 blows up with the error message:

"UnknownError – "An unknown error occurred within Indexed Database."

After this point the database is unusable. It still reports it self as open, but any subsequent calls (e.g., adding an item, opening a transaction) result in further errors.

This affects the iOS 10.3 beta as well.

On Chrome and Firefox, and Safari < 10.1 there is no error, and it works as expected.

--

Sorry for not submitting a pure IDB test case, but I'm only familiar with the Dexie API. Hopefully this can help.

== Test Case ==

source: https://github.com/Ramblurr/dexie-null-compound-index/blob/master/test-case.html
run directly: https://rawgit.com/Ramblurr/dexie-null-compound-index/master/test-case.html
Comment 1 Casey 2017-03-23 04:10:32 PDT
I just found another possibly related bug that is exposed with this test case.

The first time you load the test case it will fail in the affected browsers, then if you reload it, the test hangs. If you open a new tab with the test case, the test case will run.

AFAICT, it seems that once this bug is triggered, after a refresh in the same tab, any calls to IDB (opening a DB, deleting a db) hangs completely.
Comment 2 Casey 2017-03-23 08:55:10 PDT
Created attachment 305198 [details]
com.apple.WebKit.Databases crash log

Here is a crash log!
Comment 3 Brady Eidson 2017-03-23 10:11:50 PDT
Confirmed, I get the same Databases crash when running your test case.

Won't be able to get to this ASAP, but will soon; please leave it at that URL to test when the time comes.
Comment 4 Radar WebKit Bug Importer 2017-03-23 10:29:47 PDT
<rdar://problem/31222351>
Comment 5 Brady Eidson 2017-03-23 13:07:49 PDT
Being asked to serialize an Invalid key, which should never happen.

1   0x12240e2fd WTFCrash
2   0x117151148 WebCore::serializedTypeForKeyType(WebCore::IndexedDB::KeyType)
3   0x11714ffa2 WebCore::encodeKey(WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>&, WebCore::IDBKeyData const&)
4   0x11715025e WebCore::encodeKey(WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul>&, WebCore::IDBKeyData const&)
5   0x11714ff40 WebCore::serializeIDBKeyData(WebCore::IDBKeyData const&)
….

    case IndexedDB::KeyType::Invalid:
        RELEASE_ASSERT_NOT_REACHED();    <----
Comment 6 Brady Eidson 2017-03-23 13:08:52 PDT
Specifically, the key is an Array that contains an Invalid key.
Comment 7 Brady Eidson 2017-03-23 13:18:19 PDT
The index is not multi-entry, so we collect all the index keys into an array of keys.

We never check to verify that each key in that array is valid.

I'll clarify with the spec what the right behavior is here, but I suspect it's "can't index this put", which might mean the put should fail.

If I get stuck making sense of the spec, I'll dump all the of raw IDB commands that get to this point and try to recreate a pure IDB test case, which might make it more clear.

(Note, I don't have time to take this exploration further right now, but if anybody else wants to do any of the above exploration it can get us closer to resolving this)
Comment 8 Brady Eidson 2017-03-24 10:13:50 PDT
Created attachment 305292 [details]
Fairly reduced test case
Comment 9 Brady Eidson 2017-03-24 10:21:26 PDT
And the right thing to do when any of the keys are invalid is obviously to not put anything in the index.
Comment 10 Brady Eidson 2017-03-24 13:13:42 PDT
Created attachment 305316 [details]
Patch
Comment 11 Brady Eidson 2017-03-24 13:15:05 PDT
Comment on attachment 305316 [details]
Patch

Whoops, left out the private browsing variant
Comment 12 Brady Eidson 2017-03-24 13:18:42 PDT
Created attachment 305318 [details]
Patch
Comment 13 WebKit Commit Bot 2017-03-24 14:13:43 PDT
Comment on attachment 305318 [details]
Patch

Clearing flags on attachment: 305318

Committed r214375: <http://trac.webkit.org/changeset/214375>
Comment 14 WebKit Commit Bot 2017-03-24 14:13:47 PDT
All reviewed patches have been landed.  Closing bug.