IndexedDB: createObjectStore() name argument is required

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Mark Pilgrim (Google)]

Created attachment 96662 [details]
test case

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBDatabase-createObjectStore-IDBObjectStore-DOMString-name-Object-optionalParameters states that the name argument is required and can not be null. This test calls createObjectStore with no name argument.

Expected behavior: throw IDBDatabaseException.NON_TRANSIENT_ERR
Actual behavior: does not throw, creates an object store with the 9-character name "undefined"

Test case attached.

[Kentaro Hara]

Created attachment 97696 [details]
Patch

[Kentaro Hara]

This patch includes the fix for bug 62415 also:
https://bugs.webkit.org/show_bug.cgi?id=62415

[Dominic Cooney]

Comment on attachment 97696 [details]
Patch

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

> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:18
> +function test()

Do these tests need to use the moz- prefixed names? This is a WebKit test. Or is this how other tests in this suite do it?

> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:20
> +    indexedDB = evalAndLog("indexedDB = window.indexedDB || window.webkitIndexedDB || window.mozIndexedDB;");

I think you need less evalAndLog here. Just log results that are pertinent to the test.

> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:58
> +    debug("objectStore.name.length is " + store.name.length);

This feels redundant. You could just use shouldBe and test that the string has the right value and length in one go.

> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:76
> +    evalAndExpectException("db.deleteObjectStore()", "IDBDatabaseException.NON_TRANSIENT_ERR");

How is this different to what you’re testing on the previous line?

> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:94
> +    store = evalAndLog("store = db.createObjectStore('undefined')");

I think this test would be more readable if you did something like:

var validNames = ['null', 'undefined', 'valid_name'];
validNames.forEach(function (name) {
  // same assertions in here
}, this);

to emphasize that these should behave the same. It will make the test shorter, too.

[David Grogan]

Comment on attachment 97696 [details]
Patch

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

>> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:18
>> +function test()
> 
> Do these tests need to use the moz- prefixed names? This is a WebKit test. Or is this how other tests in this suite do it?

This is how other indexeddb tests do it too.  I started asking for this after trying to use our layout tests to see how mozilla interpreted parts of the spec.  Though it's only really useful if all the type assignments do it, not just this first one.

[Kentaro Hara]

Created attachment 97922 [details]
Patch

[Kentaro Hara]

Comment on attachment 97696 [details]
Patch

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

>> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:20
>> +    indexedDB = evalAndLog("indexedDB = window.indexedDB || window.webkitIndexedDB || window.mozIndexedDB;");
> 
> I think you need less evalAndLog here. Just log results that are pertinent to the test.

This is how other indexeddb tests do it too. I would like to wait other reviewers' opinions.

>> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:58
>> +    debug("objectStore.name.length is " + store.name.length);
> 
> This feels redundant. You could just use shouldBe and test that the string has the right value and length in one go.

Done.

>> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:76
>> +    evalAndExpectException("db.deleteObjectStore()", "IDBDatabaseException.NON_TRANSIENT_ERR");
> 
> How is this different to what you’re testing on the previous line?

Done. Removed this line.

>> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:94
>> +    store = evalAndLog("store = db.createObjectStore('undefined')");
> 
> I think this test would be more readable if you did something like:
> 
> var validNames = ['null', 'undefined', 'valid_name'];
> validNames.forEach(function (name) {
>   // same assertions in here
> }, this);
> 
> to emphasize that these should behave the same. It will make the test shorter, too.

Done.

> Source/WebCore/storage/IDBDatabase.cpp:82
> +    if (name.isNull() || name.isEmpty()) {

I removed name.isEmpty() in the latest patch, since an empty string is a valid keypath according to the spec.

[Kentaro Hara]

This patch includes the fix of |name| argument check for createObjectStore() (bug 62414), deleteObjectStore() (bug 62415), createIndex(), deleteIndex() and index().

[David Grogan]

(In reply to comment #6)
> (From update of attachment 97696 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97696&action=review
> 
> >> LayoutTests/storage/indexeddb/create-and-delete-object-store.html:20
> >> +    indexedDB = evalAndLog("indexedDB = window.indexedDB || window.webkitIndexedDB || window.mozIndexedDB;");
> > 
> > I think you need less evalAndLog here. Just log results that are pertinent to the test.
> 
> This is how other indexeddb tests do it too. I would like to wait other reviewers' opinions.

Most of our recent tests have that stuff.  It doesn't really matter though.

[Mark Pilgrim (Google)]

Terribly sorry to throw a wrench in the works, but my original bug report was mistaken. As per WebIDL, the expected behavior here is to throw a TypeError, not a database exception. The proper place to fix it is the IDL file.

[Mark Pilgrim (Google)]

Created attachment 97974 [details]
Patch

[Kentaro Hara]

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

> LayoutTests/storage/indexeddb/setVersion-undefined-expected.txt:17
> +PASS db.setVersion(null); threw exception Error: NON_TRANSIENT_ERR: DOM IDBDatabase Exception 2.

Is it correct to throw NON_TRANSIENT_ERR instead of TypeError when the argument is null?

[Mark Pilgrim (Google)]

Aargh, no. Too many overlapping patches.

I'm going to make a consolidated patch that fixes everything in IDBDatabase.idl once and for all.

[Mark Pilgrim (Google)]

Consolidating several bugs.

*** This bug has been marked as a duplicate of bug 63140 ***

[Kentaro Hara]

> I'm going to make a consolidated patch that fixes everything in IDBDatabase.idl once and for all.

OK. I will stop to upload a patch for this bug. 

>> Is it correct to throw NON_TRANSIENT_ERR instead of TypeError when the argument is null?
>
> Aargh, no. 

[1] Do you have any idea about how to throw TypeError when 'Nullable' name is passed to createObjectStore()? As far as I know, there seems to be no convenient IDL option (like [ConvertUndefinedOrNullToNullString] or [StrictTypeChecking]...) that helps us throw TypeError for a 'Nullable' argument. Another possible way is to throw TypeError at the head of IDBDatabase::createObjectStore(), like this:

PassRefPtr<IDBObjectStore> IDBDatabase::createObjectStore(String& name, ...) {
    ...;
    if (name.isNull()) {
        ec = TypeErrorExceptionCode;
        return 0;
    }
}

However, I could not find the exception code for TypeError anywhere in xxxxxxException.h. Maybe do we have to write a code for throwing TypeError for each JavaScript engine???

[2] According to the spec (http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBDatabase-createObjectStore-IDBObjectStore-DOMString-name-Object-optionalParameters), it just says name must not be 'Nullable', and seems not to say which exception should be thrown. Why did you think we should throw TypeError in this case? (TypeError makes sense to me, but I would like to confirm the spec that supports our implementation.)

I am asking this since I am encountering the same problem at the patch for bug 62288.

[Hajime Morrita]

Comment on attachment 97922 [details]
Patch

Marking obsolete as the bug closed.

[Hajime Morrita]

Comment on attachment 97974 [details]
Patch

Marking obsolete as the bug closed.