Bug 85078 – Make DOM Exceptions Errors

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

There is currently work undergoing to add "stack" to DOM exceptions. Since stack is available on Error object (part of its "interface" in both JSC and V8) it would make sense to make DOM exceptions Error objects (or at least have Error.prototype on its prototype chain).

http://lists.w3.org/Archives/Public/public-script-coord/2012AprJun/0136.html

[Erik Arvidsson]

Actually, the spec requires this. I missed that the first time.

http://www.w3.org/TR/WebIDL/#es-exception-interface-prototype-object

"Otherwise, the exception is not declared to inherit from another exception. The value of the internal [[Prototype]] property is the Error prototype object ([ECMA-262], section 15.11.3.1)."

[Kentaro Hara]

(In reply to comment #1)
> Actually, the spec requires this. I missed that the first time.
> 
> http://www.w3.org/TR/WebIDL/#es-exception-interface-prototype-object
> 
> "Otherwise, the exception is not declared to inherit from another exception. The value of the internal [[Prototype]] property is the Error prototype object ([ECMA-262], section 15.11.3.1)."

Sounds great!

[Oliver Hunt]

In JSC it should just magically acquire the stack property.

[Erik Arvidsson]

Created attachment 140097 [details]
Patch

Work in progress

[Erik Arvidsson]

Created attachment 140333 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140333 [details]
Patch

Attachment 140333 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12629306

New failing tests:
svg/filters/feBlend-invalid-mode.xhtml

[WebKit Review Bot]

Created attachment 140350 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Erik Arvidsson]

Created attachment 141192 [details]
Patch

[Erik Arvidsson]

Kentaro/Adam, can you look at the V8 bindings?

Olliver/Gavin, can you look at the JSC bindings?

[Adam Barth]

Comment on attachment 141192 [details]
Patch

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

V8 changes LGTM

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:-1830
> -    if (exec->argumentCount() < 2)
> -        return throwVMError(exec, createNotEnoughArgumentsError(exec));

What's the reason for removing this argumentCount check?

[Erik Arvidsson]

(In reply to comment #10)
> > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:-1830
> > -    if (exec->argumentCount() < 2)
> > -        return throwVMError(exec, createNotEnoughArgumentsError(exec));
> 
> What's the reason for removing this argumentCount check?

Sigh... This is just a rebaseline of run-binding-tests. Let me fix that.

[Erik Arvidsson]

Created attachment 141215 [details]
Patch

[Kentaro Hara]

Comment on attachment 141215 [details]
Patch

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

V8 change looks good.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:123
> +        if (!prototypeValue.IsEmpty() && prototypeValue->IsObject())

Nit: Maybe "!prototypeValue.IsEmpty() &&" is not necessary.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:127
> +        if (!prototypeValue.IsEmpty() && prototypeValue->IsObject())

Nit: Ditto.

[Erik Arvidsson]

Created attachment 141508 [details]
Patch

[Erik Arvidsson]

Created attachment 141516 [details]
Patch

[Erik Arvidsson]

I expanded the test a bit and incorporated Haraken's feedback.

Oliver/Gavin?

[Gavin Barraclough]

Comment on attachment 141516 [details]
Patch

JSC changes look fine to me, I'm hoping Oliver will chime in since he may know more than me about arrays & the DOM.  But feel free to go ahead.  I'll leave for someone else to  r+, since more of the patch is v8 related.

[Oliver Hunt]

Can we separate out the ArrayClass additions from this patch - i don't see any compelling reason to make both changes in the same patch.

[Erik Arvidsson]

Created attachment 141784 [details]
Patch

[Erik Arvidsson]

(In reply to comment #18)
> Can we separate out the ArrayClass additions from this patch - i don't see any compelling reason to make both changes in the same patch.

Done. PTAL.

[Kentaro Hara]

Comment on attachment 141784 [details]
Patch

r+ for the V8 part

[Darin Adler]

Comment on attachment 141784 [details]
Patch

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

> LayoutTests/platform/chromium/fast/dom/insertAdjacentHTML-DocumentFragment-crash-expected.txt:1
> +CONSOLE MESSAGE: line 9: Uncaught NO_MODIFICATION_ALLOWED_ERR: NO_MODIFICATION_ALLOWED_ERR: DOM Exception 7

These are really strange error messages, repeating the name of the error twice. Are you sure this is the format we want?

[Erik Arvidsson]

(In reply to comment #22)
> (From update of attachment 141784 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141784&action=review
> 
> > LayoutTests/platform/chromium/fast/dom/insertAdjacentHTML-DocumentFragment-crash-expected.txt:1
> > +CONSOLE MESSAGE: line 9: Uncaught NO_MODIFICATION_ALLOWED_ERR: NO_MODIFICATION_ALLOWED_ERR: DOM Exception 7
> 
> These are really strange error messages, repeating the name of the error twice. Are you sure this is the format we want?

I started investigating this last week but I could not figure it out. The error that is printed in the console is

"Error: HIERARCHY_REQUEST_ERR: DOM Exception 3"

but the error that is printed by DumpRenderTree is

"HIERARCHY_REQUEST_ERR: HIERARCHY_REQUEST_ERR: DOM Exception 3"

I'll post a new patch once/if I figure this out.

[Erik Arvidsson]

This is blocked by http://code.google.com/p/v8/issues/detail?id=2138

[Erik Arvidsson]

Created attachment 142762 [details]
Patch

[Erik Arvidsson]

Please take another look.

Darin: The V8 bug has been fixed so the *-expected.txt do not print the name twice.

Oliver: ArrayClass has been removed from this patch.

[WebKit Review Bot]

Comment on attachment 142762 [details]
Patch

Attachment 142762 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12718963

New failing tests:
fast/dynamic/015.html
fast/dom/timer-clear-interval-in-handler-and-generate-error.html
fast/inspector-support/uncaught-dom8-exception.html
fast/inspector-support/uncaught-dom3-exception.html
fast/inspector-support/uncaught-dom1-exception.html
fast/dom/NamedNodeMap-setNamedItem-crash.html
fast/events/remove-target-with-shadow-in-drag.html
fast/dom/insertAdjacentHTML-DocumentFragment-crash.html

[WebKit Review Bot]

Created attachment 142778 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Erik Arvidsson]

Strange, I thought the V8 roll had happened. I'll check back next week

[Kentaro Hara]

(In reply to comment #29)
> Strange, I thought the V8 roll had happened. I'll check back next week

Maybe you need to ask the sheriff-bot to roll out DEPS of chromium-ews?

[Erik Arvidsson]

Created attachment 143914 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 143914 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/JS/JSFloat64Array.h:27
> +#include <runtime/ObjectPrototype.h>

I'm confused why this is needed.

> Source/WebCore/bindings/scripts/test/JS/JSTestNode.h:28
> +#include <runtime/ObjectPrototype.h>

Why is this needed?  Is this auto-generated code?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
> +#define V8_STORE_PRIMORDIAL(name, Name) \

Macro sadness. :(

> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:-39
> -    V(objectPrototype) \

?

[Erik Arvidsson]

Comment on attachment 143914 [details]
Patch

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

>> Source/WebCore/bindings/scripts/test/JS/JSTestNode.h:28
>> +#include <runtime/ObjectPrototype.h>
> 
> Why is this needed?  Is this auto-generated code?

It is auto generated but I can change the generated code to not add this here

>> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
>> +#define V8_STORE_PRIMORDIAL(name, Name) \
> 
> Macro sadness. :(

V8 API is pretty verbose. Without a macro this code has to be duplicated (and one more time for ArrayClass if we ever dare try that again).

>> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:-39
>> -    V(objectPrototype) \
> 
> ?

Not used. I guess someone forgot to remove it earlier (probably me).

[WebKit Review Bot]

Comment on attachment 143914 [details]
Patch

Attachment 143914 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12793327

New failing tests:
svg/filters/feBlend-invalid-mode.xhtml

[WebKit Review Bot]

Created attachment 143938 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Erik Arvidsson]

Created attachment 144602 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 144602 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2441
> +    my $WrapperTypePrototype = $dataNode->isException ? "WrapperTypeErrorPrototype" : "WrapperTypeObjectPrototype";

I'm slightly confused as to why you have to add this enum.  I guess the info structs are short-cuts for initializing objects?  We must have objects in V8 which inherit from something other than object directly.  How do they get initialized?  Why wouldn't objects which inherit from Error do the same?  Is this enum just to make it so the shortcut can be used for either Object or Error subclasses?

[Erik Arvidsson]

(In reply to comment #37)
> (From update of attachment 144602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144602&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2441
> > +    my $WrapperTypePrototype = $dataNode->isException ? "WrapperTypeErrorPrototype" : "WrapperTypeObjectPrototype";
> 
> I'm slightly confused as to why you have to add this enum.  I guess the info structs are short-cuts for initializing objects?  We must have objects in V8 which inherit from something other than object directly.  How do they get initialized?  Why wouldn't objects which inherit from Error do the same?  Is this enum just to make it so the shortcut can be used for either Object or Error subclasses?

DOM wrappers are created from FunctionTemplates. A FunctionTemplate can inherit from another FunctionTemplate and that causes the prototype chain to be set up correctly. However, FunctionTemplate cannot inherit from an object. To achieve that we therefore set the prototype when we create the constructor function (this happens once per context since the constructor objects are cached).

[Eric Seidel (no email)]

Comment on attachment 144602 [details]
Patch

The code looks fine, I'd like someone with V8 knowledge to take a look.

[Erik Arvidsson]

Anton, Haraken: Any comments?

[Kentaro Hara]

Comment on attachment 144602 [details]
Patch

LGTM

[Erik Arvidsson]

Created attachment 144896 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 144896 [details]
Patch

LGTM.

[Erik Arvidsson]

Oliver: Any final comments?

[Oliver Hunt]

Comment on attachment 144896 [details]
Patch

If you're adding a new reference to the ErrorPrototype you need to mark it - look for how m_regExpPrototype, etc are marked

[Erik Arvidsson]

(In reply to comment #45)
> (From update of attachment 144896 [details])
> If you're adding a new reference to the ErrorPrototype you need to mark it - look for how m_regExpPrototype, etc are marked

I assume you mean like this?

diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
index 645fa04..fb523ec 100644
--- a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
+++ b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
@@ -374,6 +374,7 @@ void JSGlobalObject::visitChildren(JSCell* cell, SlotVisitor& visitor)
     visitIfNeeded(visitor, &thisObject->m_numberPrototype);
     visitIfNeeded(visitor, &thisObject->m_datePrototype);
     visitIfNeeded(visitor, &thisObject->m_regExpPrototype);
+    visitIfNeeded(visitor, &thisObject->m_errorPrototype);
 
     visitIfNeeded(visitor, &thisObject->m_argumentsStructure);
     visitIfNeeded(visitor, &thisObject->m_arrayStructure);

[Erik Arvidsson]

Created attachment 144952 [details]
Patch

[Oliver Hunt]

Comment on attachment 144952 [details]
Patch

r=me, assuming no changes to the V8 stuff

[WebKit Review Bot]

Comment on attachment 144952 [details]
Patch

Clearing flags on attachment: 144952

Committed r119124: <http://trac.webkit.org/changeset/119124>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

Comment on attachment 144602 [details]
Patch

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

Sorry for late response, I was on vacation.

Where do you initlialize m_errorPrototype in V8 bindings?

Overall, cannot we move logic to setup prototype in function template generating?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
> +#define V8_STORE_PRIMORDIAL(name, Name) \

up to you, but I would rather put most of the logic into a function and keep macro minimal to ease debugging/stack trace analysis.

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:63
> +    if (symbol.IsEmpty()) \

do you really need this check?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:65
> +    v8::Handle<v8::Object> object = v8::Handle<v8::Object>::Cast(m_context->Global()->Get(symbol)); \

.As<v8::Object>() is a better option imho

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:77
> +    if (prototypeString.IsEmpty())

ditto

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:119
> +        v8::Local<v8::Value> prototypeValue = function->Get(v8::String::NewSymbol("prototype"));

do you need a designated local for prototypeValue?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:120
> +        if (prototypeValue->IsObject())

when this check fails?

> Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:121
> +            v8::Local<v8::Object>::Cast(prototypeValue)->SetPrototype(m_errorPrototype.get());

again, .As<...>

[Erik Arvidsson]

(In reply to comment #51)
> (From update of attachment 144602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144602&action=review
> 
> Sorry for late response, I was on vacation.
> 
> Where do you initlialize m_errorPrototype in V8 bindings?

It is done in V8BindingPerContextData::init()

> Overall, cannot we move logic to setup prototype in function template generating?

No. FunctionTemplate can only inherit another FunctionTemplate, not a JS object. Yeah, this is backwards, maybe we should update the V8 API here?

> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
> > +#define V8_STORE_PRIMORDIAL(name, Name) \
> 
> up to you, but I would rather put most of the logic into a function and keep macro minimal to ease debugging/stack trace analysis.
> 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:63
> > +    if (symbol.IsEmpty()) \
> 
> do you really need this check?

OOM check. All V8 functions can return an empty object. Maybe this is too defensive?

> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:65
> > +    v8::Handle<v8::Object> object = v8::Handle<v8::Object>::Cast(m_context->Global()->Get(symbol)); \
> 
> .As<v8::Object>() is a better option imho

I go back and forth on this. I think that C++ programmers are more used to Cast but As is slight;y shorter.
 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:77
> > +    if (prototypeString.IsEmpty())
> 
> ditto
> 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:119
> > +        v8::Local<v8::Value> prototypeValue = function->Get(v8::String::NewSymbol("prototype"));
> 
> do you need a designated local for prototypeValue?

Same here... something might go wrong.

> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:120
> > +        if (prototypeValue->IsObject())
> 
> when this check fails?
> 
> > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:121
> > +            v8::Local<v8::Object>::Cast(prototypeValue)->SetPrototype(m_errorPrototype.get());
> 
> again, .As<...>

This code is only run once so the extra checks are not really a perf issue but I agree with you that this code is maybe too defensive and if any of these checks fail we are pretty much screwed any way.

I can do a follow up patch if you want?

[anton muhin]

(In reply to comment #52)
> (In reply to comment #51)
> > (From update of attachment 144602 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=144602&action=review
> > 
> > Sorry for late response, I was on vacation.
> > 
> > Where do you initlialize m_errorPrototype in V8 bindings?
> 
> It is done in V8BindingPerContextData::init()

Thanks, missed that behind the macros.

> 
> > Overall, cannot we move logic to setup prototype in function template generating?
> 
> No. FunctionTemplate can only inherit another FunctionTemplate, not a JS object. Yeah, this is backwards, maybe we should update the V8 API here?

Alas---templates are not context-sensitive while objects are, hence it's that way.  I was just hoping we may construct this Error from bindings code, but it's apparently not the case.

I am just really concerned with WrapperTypeInfo growing bigger and bigger.

> 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:59
> > > +#define V8_STORE_PRIMORDIAL(name, Name) \
> > 
> > up to you, but I would rather put most of the logic into a function and keep macro minimal to ease debugging/stack trace analysis.
> > 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:63
> > > +    if (symbol.IsEmpty()) \
> > 
> > do you really need this check?
> 
> OOM check. All V8 functions can return an empty object. Maybe this is too defensive?

No, v8 API never returns empty handle due to OOM: it'll try to GC heap and if even after GC the object cannot be allocated, VM will just kill the process.  Typical reason for V8 API to return an empty handle is an exception thrown.

> 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:65
> > > +    v8::Handle<v8::Object> object = v8::Handle<v8::Object>::Cast(m_context->Global()->Get(symbol)); \
> > 
> > .As<v8::Object>() is a better option imho
> 
> I go back and forth on this. I think that C++ programmers are more used to Cast but As is slight;y shorter.

At least when conceived, As<...> were planned as replacement for Cast :)  But it's stylistic.

> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:77
> > > +    if (prototypeString.IsEmpty())
> > 
> > ditto
> > 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:119
> > > +        v8::Local<v8::Value> prototypeValue = function->Get(v8::String::NewSymbol("prototype"));
> > 
> > do you need a designated local for prototypeValue?
> 
> Same here... something might go wrong.
> 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:120
> > > +        if (prototypeValue->IsObject())
> > 
> > when this check fails?
> > 
> > > Source/WebCore/bindings/v8/V8BindingPerContextData.cpp:121
> > > +            v8::Local<v8::Object>::Cast(prototypeValue)->SetPrototype(m_errorPrototype.get());
> > 
> > again, .As<...>
> 
> This code is only run once so the extra checks are not really a perf issue but I agree with you that this code is maybe too defensive and if any of these checks fail we are pretty much screwed any way.

I am not concerned with performance, but rather with making code harder to understand: no one should go paranoid what's the edge case is.

> I can do a follow up patch if you want?

That's completely up to you.

[Tim Horton]

Why did you add new chromium platform-specific layout test results instead of updating the shared results?

[Erik Arvidsson]

(In reply to comment #54)
> Why did you add new chromium platform-specific layout test results instead of updating the shared results?

Not sure what you mean here. I consolidated chromium-win/chromium-mac to chromium

[Tim Horton]

(In reply to comment #55)
> (In reply to comment #54)
> > Why did you add new chromium platform-specific layout test results instead of updating the shared results?
> 
> Not sure what you mean here. I consolidated chromium-win/chromium-mac to chromium

Ah. Still:  http://trac.webkit.org/changeset/119137