Bug 91679 – IndexedDB: Throw native TypeErrors per spec

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Throw native TypeErrors per spec

[Joshua Bell]

Created attachment 153102 [details]
Patch

[Joshua Bell]

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.

[Joshua Bell]

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.

[Joshua Bell]

*** Bug 87987 has been marked as a duplicate of this bug. ***

[Joshua Bell]

https://bugs.webkit.org/show_bug.cgi?id=91687 has the "direction" fixes. Once that lands I'll rebase this.

[Joshua Bell]

Created attachment 153144 [details]
Patch

[Joshua Bell]

Rebased, now just the TypeError changes.

haraken@ - any thoughts?

[Kentaro Hara]

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)

[Joshua Bell]

(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.

[Kentaro Hara]

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.

[Joshua Bell]

(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.

[WebKit Review Bot]

Comment on attachment 153144 [details]
Patch

Clearing flags on attachment: 153144

Committed r123112: <http://trac.webkit.org/changeset/123112>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Joshua Bell]

(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 ?

[Darin Adler]

(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.

[Joshua Bell]

(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

[Darin Adler]

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.

[Erik Arvidsson]

(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.

[Joshua Bell]

(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)

[Joshua Bell]

> and it seems presumptuous to squat on WebCore::Error in this enum.

FWIW, the v8 binding internally uses "GeneralError" to avoid this.

[Joshua Bell]

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. :)