Bug 58465

Summary: IndexedDB createObjectStore should throw if name is null
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, dgrogan, eric, fishd, hans, pilgrim, tony
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
test case
none
Patch
none
Throw NON_TRANSIENT_ERR instead
none
Patch for landing none

Description Mark Pilgrim (Google) 2011-04-13 11:47:41 PDT
http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBDatabase-createObjectStore states that the name property can not be null. Mozilla throws, but WebKit does not. Instead, we create an object store whose name is the 4-character string "null".
Comment 1 Mark Pilgrim (Google) 2011-04-13 11:48:33 PDT
Created attachment 89419 [details]
test case
Comment 2 Mark Pilgrim (Google) 2011-04-13 11:49:08 PDT
Similar to, but not a duplicate of, bug 58365.
Comment 3 Mark Pilgrim (Google) 2011-04-27 19:26:21 PDT
Created attachment 91410 [details]
Patch
Comment 4 Mark Pilgrim (Google) 2011-04-27 19:28:12 PDT
Similar to bug 58365, the root problem is in the IDL file which needs to specify that a null DOMString is treated as null. Then the C++ isNull() check works as expected.
Comment 5 David Grogan 2011-04-27 19:33:57 PDT
Comment on attachment 91410 [details]
Patch

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

r+

> LayoutTests/storage/indexeddb/mozilla/create-objectstore-null-name.html:51
> +    evalAndExpectException("db.createObjectStore(null);", "IDBDatabaseException.CONSTRAINT_ERR");

It looks like the specific exception isn't dictated.  I'm assuming firefox throws CONSTRAINT_ERR?
Comment 6 Mark Pilgrim (Google) 2011-04-27 20:11:48 PDT
(In reply to comment #5)
> > LayoutTests/storage/indexeddb/mozilla/create-objectstore-null-name.html:51
> > +    evalAndExpectException("db.createObjectStore(null);", "IDBDatabaseException.CONSTRAINT_ERR");
> 
> It looks like the specific exception isn't dictated.  I'm assuming firefox throws CONSTRAINT_ERR?

I thought so, but upon double-checking, it throws NON_TRANSIENT_ERR instead. Updating patch...
Comment 7 Mark Pilgrim (Google) 2011-04-27 20:12:57 PDT
Created attachment 91414 [details]
Throw NON_TRANSIENT_ERR instead

This patch better matches Firefox's behavior.
Comment 8 David Grogan 2011-04-27 20:13:38 PDT
Comment on attachment 91414 [details]
Throw NON_TRANSIENT_ERR instead

r+
Comment 9 WebKit Commit Bot 2011-05-18 15:58:16 PDT
Comment on attachment 91414 [details]
Throw NON_TRANSIENT_ERR instead

Rejecting attachment 91414 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2

Last 500 characters of output:
-objectstore-null-name.html
patch unexpectedly ends in middle of line
patch: **** malformed patch at line 67:  

fatal: pathspec 'LayoutTests/storage/indexeddb/mozilla/create-objectstore-null-name.html' did not match any files
Failed to git add LayoutTests/storage/indexeddb/mozilla/create-objectstore-null-name.html. at /Projects/CommitQueue/Tools/Scripts/svn-apply line 439.

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Tony Chang', u'--force']" exit_code: 2

Full output: http://queues.webkit.org/results/8710633
Comment 10 Mark Pilgrim (Google) 2011-05-20 06:36:33 PDT
Any idea why this failed to commit? The queue tells me nothing.
Comment 11 Tony Chang 2011-05-20 11:01:48 PDT
Created attachment 94240 [details]
Patch for landing
Comment 12 Tony Chang 2011-05-20 11:02:33 PDT
(In reply to comment #10)
> Any idea why this failed to commit? The queue tells me nothing.

The diff was missing a newline at the end of the file.
Comment 13 WebKit Commit Bot 2011-05-20 11:30:46 PDT
Comment on attachment 94240 [details]
Patch for landing

Clearing flags on attachment: 94240

Committed r86970: <http://trac.webkit.org/changeset/86970>
Comment 14 WebKit Commit Bot 2011-05-20 11:30:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Daniel Bates 2011-05-20 12:51:08 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > Any idea why this failed to commit? The queue tells me nothing.
> 
> The diff was missing a newline at the end of the file.

Did you edit this patch by hand? I added the missing new line, applied the patch using svn-apply then used svn-create-patch to recreate the patch P. And P has a new line at the end of the file. Similarly, I was able to create a valid patch by following the aforementioned steps and calling svn diff instead of  svn-create-patch.
Comment 16 Mark Pilgrim (Google) 2011-05-25 18:32:02 PDT
(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > Any idea why this failed to commit? The queue tells me nothing.
> > 
> > The diff was missing a newline at the end of the file.
> 
> Did you edit this patch by hand? I added the missing new line

Aha, that's it. I've fixed my editor so it doesn't strip final newlines.

Thanks.
Comment 17 Mark Pilgrim (Google) 2011-05-25 18:36:51 PDT
Patch that landed has repeated test results, causing test to fail.
Comment 18 Tony Chang 2011-05-26 09:27:26 PDT
(In reply to comment #17)
> Patch that landed has repeated test results, causing test to fail.

Sorry, probably patched twice into a new file.  Will fix.
Comment 19 Tony Chang 2011-05-26 09:32:29 PDT
http://trac.webkit.org/changeset/87393