Bug 91679

Summary: IndexedDB: Throw native TypeErrors per spec
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: WebCore Misc.Assignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alecflett, arv, darin, dgrogan, haraken, japhet, jochen, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 91687    
Bug Blocks: 85513, 102114    
Attachments:
Description Flags
Patch
none
Patch none

Description Joshua Bell 2012-07-18 15:15:53 PDT
IndexedDB: Throw native TypeErrors per spec
Comment 1 Joshua Bell 2012-07-18 15:25:26 PDT
Created attachment 153102 [details]
Patch
Comment 2 Joshua Bell 2012-07-18 15:27:11 PDT
public-webapps thread: http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/0949.html

This hooks up native JS TypeErrors throwable from the DOM with minimal work. Not sure this is the direction we want to go, but apparently this is how Mozilla does it as well.
Comment 3 Joshua Bell 2012-07-18 15:30:13 PDT
Comment on attachment 153102 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:111
> +        ec = NATIVE_TYPE_ERR;

Note: there is no test for this. I believe it should actually get the string "null"; this should change to an assertion and add a test (separate bug?)

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:125
> +        ec = NATIVE_TYPE_ERR;

Ditto.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:103
> +    return openCursor(context, keyRange.release(), direction, ec);

I found this (and 3 similar lines in the diff) as existing bugs where we drop the direction parameter on the floor. Oops! I'll file a separate bug and test case, and remove it from this patch before landing.
Comment 4 Joshua Bell 2012-07-18 16:06:18 PDT
*** Bug 87987 has been marked as a duplicate of this bug. ***
Comment 5 Joshua Bell 2012-07-18 16:07:06 PDT
https://bugs.webkit.org/show_bug.cgi?id=91687 has the "direction" fixes. Once that lands I'll rebase this.
Comment 6 Joshua Bell 2012-07-18 17:19:06 PDT
Created attachment 153144 [details]
Patch
Comment 7 Joshua Bell 2012-07-18 17:20:26 PDT
Rebased, now just the TypeError changes.

haraken@ - any thoughts?
Comment 8 Kentaro Hara 2012-07-18 23:37:55 PDT
Comment on attachment 153144 [details]
Patch

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

The implementation looks good.

> Source/WebCore/ChangeLog:10
> +        Per the IDB spec, the advance(), openCursor(), openKeyCursor() and transaction()
> +        methods supposed to throw true native JavaScript TypeError objects as exceptions
> +        rather than DOMException objects. Implement this by adding a special DOMException

IMHO, this behavior sounds a bit strange to me. Given that these exceptions are raised from inside DOM, DOMException objects sounds more reasonable to me. What about other browsers?

> Source/WebCore/ChangeLog:11
> +        code that is tested for in the binding layer.

Do you know any other code that is doing something like this? (i.e. convert a special DOMException to JavaScript Error in the binding layer)
Comment 9 Joshua Bell 2012-07-18 23:43:24 PDT
(In reply to comment #8)
> 
> IMHO, this behavior sounds a bit strange to me. Given that these exceptions are raised from inside DOM, DOMException objects sounds more reasonable to me. What about other browsers?

See comment #2.
 
> > Source/WebCore/ChangeLog:11
> > +        code that is tested for in the binding layer.
> 
> Do you know any other code that is doing something like this? (i.e. convert a special DOMException to JavaScript Error in the binding layer)

No. Alternate suggestions welcome.
Comment 10 Kentaro Hara 2012-07-18 23:51:12 PDT
Comment on attachment 153144 [details]
Patch

>> IMHO, this behavior sounds a bit strange to me. Given that these exceptions are raised from inside DOM, DOMException objects sounds more reasonable to me. What about other browsers?
>
> See comment #2.

Thanks. I understood. (though TypeError sounds a bit strange to me:-)

>> Do you know any other code that is doing something like this? (i.e. convert a special DOMException to JavaScript Error in the binding layer)
>
> No. Alternate suggestions welcome.

Given that this fix is needed in IDB only, I think your approach is simple and good. If we encountered other code that needs the fix, let's consider how we can generalize it.
Comment 11 Joshua Bell 2012-07-19 08:50:41 PDT
(In reply to comment #10)
> (From update of attachment 153144 [details])
> >> IMHO, this behavior sounds a bit strange to me. Given that these exceptions are raised from inside DOM, DOMException objects sounds more reasonable to me. What about other browsers?
> >
> > See comment #2.
> 
> Thanks. I understood. (though TypeError sounds a bit strange to me:-)

Yeah - it was weird to us too. In the linked email thread we did try pushing back, but the other vendors had already implemented it.

Thanks, and I'll keep an eye out for similar needs in other specs.
Comment 12 WebKit Review Bot 2012-07-19 09:37:32 PDT
Comment on attachment 153144 [details]
Patch

Clearing flags on attachment: 153144

Committed r123112: <http://trac.webkit.org/changeset/123112>
Comment 13 WebKit Review Bot 2012-07-19 09:37:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Darin Adler 2012-11-13 09:36:14 PST
I don’t see code in ExceptionCodeDescription to handle NATIVE_TYPE_ERR. Nor assertions to make sure it’s not used with NATIVE_TYPE_ERR. That’s not good.

The code to convert exceptions to Objective-C exceptions in bindings/objc/ExceptionHandlers.mm was not revised when NATIVE_TYPE_ERR was added, so that won’t handle NATIVE_TYPE_ERR.

I imagine there are other DOM bindings like the Objective-C one that won’t handle this new error code correctly.

The only reason this might be OK for now is that IndexedDB is not built on platforms that use Objective-C bindings, but I think there may be problems coming.

----

I don’t like the name NATIVE_TYPE_ERR.

The other exception codes are named what they are because that’s the name from the DOM specification.

For the an exception code that we translate to a JavaScript exception, I don’t like the capitalization, I don’t like the abbreviation “ERR”, and I don’t like the word “NATIVE” to mean a JavaScript language exception, because the word native is ambiguous.

I’d like us come up with a better name and do a rename.
Comment 15 Joshua Bell 2012-11-13 09:54:54 PST
(In reply to comment #14)
> I don’t see code in ExceptionCodeDescription to handle NATIVE_TYPE_ERR. Nor assertions to make sure it’s not used with NATIVE_TYPE_ERR. That’s not good.
> 
> The code to convert exceptions to Objective-C exceptions in bindings/objc/ExceptionHandlers.mm was not revised when NATIVE_TYPE_ERR was added, so that won’t handle NATIVE_TYPE_ERR.
> 
> I imagine there are other DOM bindings like the Objective-C one that won’t handle this new error code correctly.
> 
> The only reason this might be OK for now is that IndexedDB is not built on platforms that use Objective-C bindings, but I think there may be problems coming.
> 

It looks like http://svn.webkit.org/changeset/134345 attempted to add support for NATIVE_TYPE_ERR to CodeGeneratorObjC.pm but it appears the patch was not complete. Agreed it should be rolled out for now.

> ----
> 
> I don’t like the name NATIVE_TYPE_ERR.
> 
> The other exception codes are named what they are because that’s the name from the DOM specification.
> 
> For the an exception code that we translate to a JavaScript exception, I don’t like the capitalization, I don’t like the abbreviation “ERR”, and I don’t like the word “NATIVE” to mean a JavaScript language exception, because the word native is ambiguous.
> 
> I’d like us come up with a better name and do a rename.

I'm happy to do the rename. 

Suggestions? Just TypeError ?
Comment 16 Darin Adler 2012-11-13 09:58:35 PST
(In reply to comment #15)
> Suggestions? Just TypeError ?

Sure, that seems good.

We could also look and see what language the HTML specification uses for this? That might lead us to a good name.
Comment 17 Joshua Bell 2012-11-13 10:06:59 PST
(In reply to comment #16)
> (In reply to comment #15)
> > Suggestions? Just TypeError ?
> 
> Sure, that seems good.
> 
> We could also look and see what language the HTML specification uses for this? That might lead us to a good name.

WebIDL http://dev.w3.org/2006/webapi/WebIDL/ says: "A number of predefined exceptions are also available to be thrown from specifications. These predefined exceptions correspond to the ECMAScript error objects ([ECMA-262], section 15.11). Specifically, the list of predefined exceptions is as follows: Error, EvalError, RangeError, ReferenceError, SyntaxError, TypeError and URIError."

... and then in the ECMAScript binding section has "When an algorithm says to “throw a SomethingError” then this means to construct a new ECMAScript SomethingError object and to throw it, just as the algorithms in ECMA-262 do."

Which would imply that specs defining WebIDL fragments and behavior should use the term TypeError.

It looks like the HTML spec itself has not yet been updated:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=19921
Comment 18 Darin Adler 2012-11-13 10:24:47 PST
So I think we should use the name TypeError, and then look over each binding to make sure it does something sensible and possibly even something backwards compatible with the error. And plan to do the same with other errors that correspond to JavaScript errors over time as they are used in the DOM.

I think there are 5 actively maintained bindings, although I am not personally certain about the status of the cpp and gobject bindings.
Comment 19 Erik Arvidsson 2012-11-13 10:40:57 PST
(In reply to comment #18)
> I think there are 5 actively maintained bindings, although I am not personally certain about the status of the cpp and gobject bindings.

However, WebIDL only defines bindings for ECMAScript. We really need someone from Apple to take care of the ObjC bindings, someone from QT for CPP bindings and someone for GTK to take care of the GTK bindings.
Comment 20 Joshua Bell 2012-11-13 11:13:47 PST
(In reply to comment #18)
> So I think we should use the name TypeError,

FWIW, a naive rename of NATIVE_TYPE_ERR to TypeError collides with bindings/v8/V8ThrowTypeException.h. 

While that's manageable, looking at the list again, we'd probably want to define the full set of WebIDL error types: Error, EvalError, RangeError, ReferenceError, SyntaxError, TypeError and URIError - and it seems presumptuous to squat on WebCore::Error in this enum.

So, other possibilities would be IDLTypeError or WebIDLTypeError (giving IDLError or WebIDLError)
Comment 21 Joshua Bell 2012-11-13 11:18:20 PST
> and it seems presumptuous to squat on WebCore::Error in this enum.

FWIW, the v8 binding internally uses "GeneralError" to avoid this.
Comment 22 Joshua Bell 2012-11-13 12:09:43 PST
Rename patch over in https://bugs.webkit.org/show_bug.cgi?id=102114 for anyone who's interested. More bikeshedding can take place over there. :)