Bug 70251 - IndexedDB: Passing empty array to IDBDatabase.transaction should raise exception
Summary: IndexedDB: Passing empty array to IDBDatabase.transaction should raise exception
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-17 11:26 PDT by Joshua Bell
Modified: 2011-10-27 06:36 PDT (History)
6 users (show)

See Also:


Attachments
Patch (85.14 KB, patch)
2011-10-17 11:44 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (66.94 KB, patch)
2011-10-18 14:32 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (68.83 KB, patch)
2011-10-20 12:17 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (77.45 KB, patch)
2011-10-20 15:43 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (77.67 KB, patch)
2011-10-26 08:58 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2011-10-17 11:26:25 PDT
IndexedDB: Passing empty array to IDBDatabase.transaction should raise exception
Comment 1 Joshua Bell 2011-10-17 11:44:41 PDT
Created attachment 111290 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-17 11:46:44 PDT
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.
Comment 3 Joshua Bell 2011-10-17 11:47:51 PDT
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.)
Comment 4 David Grogan 2011-10-17 14:30:06 PDT
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?
Comment 5 Joshua Bell 2011-10-17 14:39:21 PDT
(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.
Comment 6 Joshua Bell 2011-10-18 14:32:24 PDT
Created attachment 111502 [details]
Patch
Comment 7 Adam Barth 2011-10-18 15:02:45 PDT
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?
Comment 8 Joshua Bell 2011-10-18 15:11:18 PDT
(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.
Comment 9 Adam Barth 2011-10-18 15:13:12 PDT
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.
Comment 10 Joshua Bell 2011-10-20 12:17:20 PDT
Created attachment 111823 [details]
Patch
Comment 11 David Grogan 2011-10-20 12:40:17 PDT
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.)
Comment 12 Joshua Bell 2011-10-20 15:43:46 PDT
Created attachment 111860 [details]
Patch
Comment 13 Joshua Bell 2011-10-20 15:46:35 PDT
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?
Comment 14 Adam Barth 2011-10-25 16:53:26 PDT
Comment on attachment 111860 [details]
Patch

Looks fantastic.  Thanks.
Comment 15 WebKit Review Bot 2011-10-25 18:19:47 PDT
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
Comment 16 Joshua Bell 2011-10-26 08:58:34 PDT
Created attachment 112517 [details]
Patch
Comment 17 Joshua Bell 2011-10-26 09:49:55 PDT
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.)
Comment 18 Adam Barth 2011-10-26 18:26:06 PDT
Comment on attachment 112517 [details]
Patch

Ok.  I think the SVN server might be down, but we'll give it a try.
Comment 19 WebKit Review Bot 2011-10-27 06:36:08 PDT
Comment on attachment 112517 [details]
Patch

Clearing flags on attachment: 112517

Committed r98563: <http://trac.webkit.org/changeset/98563>
Comment 20 WebKit Review Bot 2011-10-27 06:36:14 PDT
All reviewed patches have been landed.  Closing bug.