Bug 58471 – IndexedDB createObjectStore should throw if options arg is invalid

• Component: New Bugs
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

Two ways the options argument can be invalid: it's not an object, or it's an object with unknown attributes (besides keyPath and autoIncrement). Mozilla throws if the options argument is invalid in either of these ways; WebKit does not throw in either case.

[Mark Pilgrim (Google)]

Created attachment 89432 [details]
test case #1: options arg is not an object

[Mark Pilgrim (Google)]

Created attachment 89433 [details]
test case #2: options arg contains unknown attributes

[Joshua Bell]

Test case #2 is no longer valid - IndexedDB spec has changed to use the (new) WebIDL dictionary type for the options object, which ignores unknown attributes - http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary

Test case #1 is still valid; in Chrome 15 I can pass a string as the options argument and no exception is thrown. Per WebIDL: "If Type(V) is not Object, then throw a TypeError."

[Eugene Girard]

Created attachment 123959 [details]
Patch

[Eugene Girard]

Based on jsbell's comment, my patch uses (and fixes) only the first test case.

[Joshua Bell]

Comment on attachment 123959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123959&action=review

> Source/WebCore/storage/IDBDatabase.cpp:93
> +        ec = IDBDatabaseException::CONSTRAINT_ERR;

The IndexedDB spec doesn't indicate what exception should be thrown here; that's actually dictated by the WebIDL spec: http://www.w3.org/TR/WebIDL/#es-dictionary "If Type(V) is not Object, then throw a TypeError"

So... a more correct fix would be to put this check in the v8 binding code/generator and throw a TypeError there when an OptionsObject is expected if the v8 value fails the isObject test.

> LayoutTests/storage/indexeddb/createObjectStore-bad-options.html:10
> +<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">

This CSS file has been removed.

> LayoutTests/storage/indexeddb/createObjectStore-bad-options.html:11
> +<script src="../../fast/js/resources/js-test-post-function.js"></script>

The js-test-post-function.js script is no longer necessary to include.

> LayoutTests/storage/indexeddb/createObjectStore-bad-options.html:29
> +    shouldBeTrue("'webkitIndexedDB' in window");

Prefer doing: "indexedDB = window.indexedDB || window.webkitIndexedDB;" and using the non-prefixed version in the rest of the test. Ditto for any other webkit-prefixed types.

> LayoutTests/storage/indexeddb/createObjectStore-bad-options.html:34
> +    request = evalAndLog("webkitIndexedDB.open(name, description)");

Since the description parameter of open() is no longer in the spec we should drop it from new tests.

> LayoutTests/storage/indexeddb/createObjectStore-bad-options.html:52
> +    deleteAllObjectStores(db);

This is fine, but as an FYI we're trying to follow a new pattern in tests where the DB is deleted using deleteDatabase prior to open(), so we get to a clean state and don't rely on these helper functions. The pattern is:

request = indexedDB.deleteDatabase(dbname);
request.onerror = unexpectedErrorCallback;
request.onsuccess = function () {
    request = indexedDB.open(dbname);
    ...
};

> LayoutTests/storage/indexeddb/createObjectStore-bad-options.html:58
> +var successfullyParsed = true;

The "successfullyParsed = true;" statement is no longer necessary.

[Eugene Girard]

Created attachment 125412 [details]
Patch

[Joshua Bell]

Comment on attachment 125412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125412&action=review

The binding changes LGTM. abarth@ can you take an initial look?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1619
> +               $parameterCheckString .= "        ec = TYPE_MISMATCH_ERR; V8Proxy::setDOMException(ec); return throwError(\"Not an object.\", V8Proxy::TypeError);\n";

Nit: generate these three statements on their own lines, for readability of the generator and the generated code.

> LayoutTests/ChangeLog:17
> +        * storage/indexeddb/transaction-basics.html:

Add a short note e.g. "Corrected erroneous calls that would now throw." explaining the transaction-basics.html change.

> LayoutTests/storage/indexeddb/createObjectStore-bad-options.html:50
> +    evalAndExpectExceptionClass("db.createObjectStore('foo', 'bar');", "TypeError");

Since this is the only actual line in the test, can you just add it to objectstore-basics.html instead?

Also, in addition to a test with string, I'd suggest adding one with a boolean. Any other interesting types expected to throw? (null, host objects, ...?)

> LayoutTests/storage/indexeddb/index-basics.html:43
> +    window.indexObject3 = evalAndLog("store.createIndex('zIndex', 'z', {unique: true})");

While you're here:

* Add shouldBeFalse("indexObject2.unique") and shouldBeTrue("indexObject3.unique") - which would have caught these bogus calls
* Since this binding change fixes https://bugs.webkit.org/show_bug.cgi?id=58467 can you add a evalAndExpectExceptionClass() test here for bogus params to createIndex()? Then you can just dupe that bug to this one.

> LayoutTests/storage/indexeddb/resources/shared.js:51
> +function evalAndExpectExceptionClass(cmd, expected)

FYI, girard and I chatted about where this should live. We agree it should stay next to evalAndExpectException and remain a separate call rather than trying to handle both cases. As part of some planned IDB layout test cleanup dgrogan may move these evalAnd* calls to live with evalAndLog.

> LayoutTests/storage/indexeddb/transaction-basics.html:59
> +    var index = evalAndLog("index = store.createIndex('indexFail', 'x', {unique: false})");

Since false is the default here and it's not relevant to the test, just remove the options parameter entirely here and below.

[Eugene Girard]

(In reply to comment #8)
> (From update of attachment 125412 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125412&action=review
> 
> The binding changes LGTM. abarth@ can you take an initial look?
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1619
> > +               $parameterCheckString .= "        ec = TYPE_MISMATCH_ERR; V8Proxy::setDOMException(ec); return throwError(\"Not an object.\", V8Proxy::TypeError);\n";
> 
> Nit: generate these three statements on their own lines, for readability of the generator and the generated code.

Agreed.  Done.

> > LayoutTests/ChangeLog:17
> > +        * storage/indexeddb/transaction-basics.html:
> 
> Add a short note e.g. "Corrected erroneous calls that would now throw." explaining the transaction-basics.html change.

Done.

> > LayoutTests/storage/indexeddb/createObjectStore-bad-options.html:50
> > +    evalAndExpectExceptionClass("db.createObjectStore('foo', 'bar');", "TypeError");
> 
> Since this is the only actual line in the test, can you just add it to objectstore-basics.html instead?
> 
> Also, in addition to a test with string, I'd suggest adding one with a boolean. Any other interesting types expected to throw? (null, host objects, ...?)

Agree, there is only one real line in the unit test.  Moving the logic into create-object-store-options.html.
According to the spec, any "object-derived" item is valid, so calling with a host object should actually work... but have the effect of using default values for options unless the passed object happened to have a property called "unique".... I did add the boolean test though.

> > LayoutTests/storage/indexeddb/index-basics.html:43
> > +    window.indexObject3 = evalAndLog("store.createIndex('zIndex', 'z', {unique: true})");
> 
> While you're here:
> 
> * Add shouldBeFalse("indexObject2.unique") and shouldBeTrue("indexObject3.unique") - which would have caught these bogus calls
> * Since this binding change fixes https://bugs.webkit.org/show_bug.cgi?id=58467 can you add a evalAndExpectExceptionClass() test here for bogus params to createIndex()? Then you can just dupe that bug to this one.

I'll do that, but it will be part of 58467's CL.

> > LayoutTests/storage/indexeddb/resources/shared.js:51
> > +function evalAndExpectExceptionClass(cmd, expected)
> 
> FYI, girard and I chatted about where this should live. We agree it should stay next to evalAndExpectException and remain a separate call rather than trying to handle both cases. As part of some planned IDB layout test cleanup dgrogan may move these evalAnd* calls to live with evalAndLog.
> 
> > LayoutTests/storage/indexeddb/transaction-basics.html:59
> > +    var index = evalAndLog("index = store.createIndex('indexFail', 'x', {unique: false})");
> 
> Since false is the default here and it's not relevant to the test, just remove the options parameter entirely here and below.

Agreed. Done.

Thanks for the feedback, Joshua.

[Eugene Girard]

Created attachment 125632 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125632 [details]
Patch

Clearing flags on attachment: 125632

Committed r106827: <http://trac.webkit.org/changeset/106827>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

I rebaselined run-bindings-tests results.