Bug 70251 – IndexedDB: Passing empty array to IDBDatabase.transaction should raise exception

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Passing empty array to IDBDatabase.transaction should raise exception

[Joshua Bell]

Created attachment 111290 [details]
Patch

[WebKit Review Bot]

Attachment 111290 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joshua Bell]

Since this is likely to break code, we may want to consider having this show a warning on the console rather than failing, at least for a milestone.

(Personally, I'd rather just throw the switch and get the pain over with.)

[David Grogan]

Comment on attachment 111290 [details]
Patch

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

LGTM

I didn't look at every single test, just spot-checked a few.

> LayoutTests/storage/indexeddb/transaction-basics.html:209
> +    transaction = evalAndLog("db.transaction(['storeName'], webkitIDBTransaction.READ_WRITE)");

This could be READ_ONLY I think, but don't upload a new patch solely for that.

> LayoutTests/storage/indexeddb/tutorial.html:213
> +    // First of all is the parameter "objectStoreNames", which must be an array of names of the 

I'm guessing the "if you pass in a string" part is not in the spec?

[Joshua Bell]

(In reply to comment #4)
> 
> > LayoutTests/storage/indexeddb/transaction-basics.html:209
> > +    transaction = evalAndLog("db.transaction(['storeName'], webkitIDBTransaction.READ_WRITE)");
> 
> This could be READ_ONLY I think, but don't upload a new patch solely for that.

Yeah, there is a lot of cruft in the unit tests. Rather than doing extensive cleanup I only changed transaction creation and tests directly related to this change that would become invalid.
 
> > LayoutTests/storage/indexeddb/tutorial.html:213
> > +    // First of all is the parameter "objectStoreNames", which must be an array of names of the 
> 
> I'm guessing the "if you pass in a string" part is not in the spec?

huh.... looking more deeply, that *is* supported in the spec: 

"If storeNames is of type DOMStringList or Array leave it as is. Otherwise, interpret it as an Array with one value, and that value is the stringified version of storeNames. If any of the strings in storeNames identifies an object store which doesn't exist, throw a NOT_FOUND_ERR exception."

However, in our IDL it's a DOMStringList. Passing in a string "just worked" in WebKit because a anything other than a string array (e.g. an object, a single string, etc) was being coerced into an empty DOMStringList, and hitting the previous case.

I'll follow up on the list to ensure that single string behavior is still desired. That would require more extensive code changes and revert many of the layouttest changes in the patch.

[Joshua Bell]

Created attachment 111502 [details]
Patch

[Adam Barth]

Comment on attachment 111502 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8IDBDatabaseCustom.cpp:48
> +v8::Handle<v8::Value> V8IDBDatabase::transactionCallback(const v8::Arguments& args)

Why does this need to be custom?

> Source/WebCore/storage/IDBDatabase.idl:51
> +        // FIXME: Improve code generator so this doesn't need to be Custom.

Can we just do that now instead of adding the custom implementation?

[Joshua Bell]

(In reply to comment #7)
> (From update of attachment 111502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111502&action=review
> 
> > Source/WebCore/bindings/v8/custom/V8IDBDatabaseCustom.cpp:48
> > +v8::Handle<v8::Value> V8IDBDatabase::transactionCallback(const v8::Arguments& args)
> 
> Why does this need to be custom?

The proposed IDL has three versions: one that takes a DOMString, one that takes a DOMStringList, and one that takes a DOMString[] - which we don't appear to support yet.  Without the third, if you pass ["a", "b"] it is coerced into a DOMString "ab" and the first is invoked.

> 
> > Source/WebCore/storage/IDBDatabase.idl:51
> > +        // FIXME: Improve code generator so this doesn't need to be Custom.
> 
> Can we just do that now instead of adding the custom implementation?

That would be preferable, if the above seems tractable.

[Adam Barth]

Autogeneration is preferable to custom bindings code because custom bindings are always wrong.  :)

If there's a problem parsing DOMString[], we can use something like DOMStringArray instead with a FIXME to change this once our IDL parser is smarter.

[Joshua Bell]

Created attachment 111823 [details]
Patch

[David Grogan]

The IDB stuff all LGTM, but let abarth look at the bindings part.  I'm assuming you ran the code generator's tests.  (I'm also assuming that such things exist.)

[Joshua Bell]

Created attachment 111860 [details]
Patch

[Joshua Bell]

Comment on attachment 111860 [details]
Patch

Latest patch adds support in the JS code generator and code generator test cases for DOMString[]. I didn't touch the other generators.

Any further suggestions, abarth?

[Adam Barth]

Comment on attachment 111860 [details]
Patch

Looks fantastic.  Thanks.

[WebKit Review Bot]

Comment on attachment 111860 [details]
Patch

Rejecting attachment 111860 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp
Hunk #1 succeeded at 1160 (offset -10 lines).
Hunk #2 succeeded at 1191 (offset -10 lines).
patching file Source/WebCore/storage/IDBDatabase.cpp
patching file Source/WebCore/storage/IDBDatabase.h
patching file Source/WebCore/storage/IDBDatabase.idl
patching file Source/WebCore/storage/IDBTransactionBackendImpl.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10210424

[Joshua Bell]

Created attachment 112517 [details]
Patch

[Joshua Bell]

Comment on attachment 112517 [details]
Patch

Not quite sure what happened with previous patch, but I needed to rebase anyway. (Looks like this one's cr-linux failed since bugs.webkit.org was flaky right after I uploaded and the bots couldn't fetch the patch.)

[Adam Barth]

Comment on attachment 112517 [details]
Patch

Ok.  I think the SVN server might be down, but we'll give it a try.

[WebKit Review Bot]

Comment on attachment 112517 [details]
Patch

Clearing flags on attachment: 112517

Committed r98563: <http://trac.webkit.org/changeset/98563>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.