Bug 58471 - IndexedDB createObjectStore should throw if options arg is invalid
Summary: IndexedDB createObjectStore should throw if options arg is invalid
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Eugene Girard
URL:
Keywords:
Depends on:
Blocks: 76198 58467
  Show dependency treegraph
 
Reported: 2011-04-13 12:21 PDT by Mark Pilgrim (Google)
Modified: 2012-02-06 17:39 PST (History)
10 users (show)

See Also:


Attachments
test case #1: options arg is not an object (1.62 KB, text/html)
2011-04-13 12:22 PDT, Mark Pilgrim (Google)
no flags Details
test case #2: options arg contains unknown attributes (1.63 KB, text/html)
2011-04-13 12:23 PDT, Mark Pilgrim (Google)
no flags Details
Patch (6.82 KB, patch)
2012-01-25 09:29 PST, Eugene Girard
no flags Details | Formatted Diff | Diff
Patch (14.92 KB, patch)
2012-02-03 14:18 PST, Eugene Girard
no flags Details | Formatted Diff | Diff
Patch (13.98 KB, patch)
2012-02-06 06:33 PST, Eugene Girard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 2011-04-13 12:21:16 PDT
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.
Comment 1 Mark Pilgrim (Google) 2011-04-13 12:22:57 PDT
Created attachment 89432 [details]
test case #1: options arg is not an object
Comment 2 Mark Pilgrim (Google) 2011-04-13 12:23:13 PDT
Created attachment 89433 [details]
test case #2: options arg contains unknown attributes
Comment 3 Joshua Bell 2011-10-14 13:46:02 PDT
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."
Comment 4 Eugene Girard 2012-01-25 09:29:33 PST
Created attachment 123959 [details]
Patch
Comment 5 Eugene Girard 2012-01-25 09:32:14 PST
Based on jsbell's comment, my patch uses (and fixes) only the first test case.
Comment 6 Joshua Bell 2012-01-25 09:50:33 PST
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.
Comment 7 Eugene Girard 2012-02-03 14:18:03 PST
Created attachment 125412 [details]
Patch
Comment 8 Joshua Bell 2012-02-03 14:52:29 PST
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.
Comment 9 Eugene Girard 2012-02-06 06:31:56 PST
(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.
Comment 10 Eugene Girard 2012-02-06 06:33:07 PST
Created attachment 125632 [details]
Patch
Comment 11 WebKit Review Bot 2012-02-06 11:50:27 PST
Comment on attachment 125632 [details]
Patch

Clearing flags on attachment: 125632

Committed r106827: <http://trac.webkit.org/changeset/106827>
Comment 12 WebKit Review Bot 2012-02-06 11:50:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Kentaro Hara 2012-02-06 17:39:00 PST
I rebaselined run-bindings-tests results.