Bug 85057 – [V8] Add "stack" property to DOMException

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Yang Guo]

Capture a stack trace at the point where the DOMException is created and make it accessible through the "stack" property in the form of a string. This is to enable capturing the stack trace from inside Javascript.

[Yang Guo]

Created attachment 139196 [details]
Patch to add desired functionality into V8 binding. The stack property is implemented as a getter.

[Yang Guo]

Created attachment 139204 [details]
Updated patch removing dependency to path names from the test expectations. Supersedes the previous patch.

[Yang Guo]

Created attachment 139205 [details]
Updated patch.

[Erik Arvidsson]

Comment on attachment 139205 [details]
Updated patch.

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

> Source/WebCore/bindings/v8/V8Proxy.cpp:579
> +    v8::Handle<v8::Value> message = exceptionObject->Get(v8String("message", isolate));
> +    v8::Handle<v8::Value> error = v8::Exception::Error(message->ToString());

Why do we need the message? Is that required to get the stack?

> Source/WebCore/bindings/v8/V8Proxy.cpp:580
> +    exceptionObject->SetAccessor(v8String("stack", isolate), DOMExceptionStackGetter, 0, error);

What is the intention here? Make sure that setting stack also works since code out there might already do that.

> LayoutTests/fast/dom/DOMException/stack-trace.html:23
> +    }

Please test that the descriptor looks like you expect it to.

It should look something like:

{
  configurable: true,
  enumerable: false,
  get: function () { ... }
  set: function () { ... }
}

> LayoutTests/platform/chromium/fast/dom/DOMException/stack-trace-expected.txt:2
> +layer at (0,0) size 800x600

Please add

layoutTestController.dumpAsText();

we definitely do not care about the layout here

[Alexey Proskuryakov]

Is your plan to only do this for v8?

[Kentaro Hara]

Do you have any idea on the spec compliance?

[Erik Arvidsson]

(In reply to comment #6)
> Do you have any idea on the spec compliance?

WebIDL actually specs that DOM exceptions should have Error.prototype in its prototype chain. "stack" is not part of any standard but since we have it all instances that have Error.prototype in its prototype chain I don't think we are violating any spec by adding it ;-)

Look at how we handle TypeError for example:

var te = new TypeError;
Object.prototype.toString.call(te)
te.hasOwnProperty('stack');  // true
te.__proto__.hasOwnProperty('stack');  // true

This seems really fishy to me. JSC seems to do this in a better way. It adds a "stack" property when an object is thrown that has Error.prototype on its prototype chain. So by fixing bug 85078 we will get JSC support for this bug for free.

[Oliver Hunt]

(In reply to comment #7)
> (In reply to comment #6)
> > Do you have any idea on the spec compliance?
> 
> WebIDL actually specs that DOM exceptions should have Error.prototype in its prototype chain. "stack" is not part of any standard but since we have it all instances that have Error.prototype in its prototype chain I don't think we are violating any spec by adding it ;-)
> 
> Look at how we handle TypeError for example:
> 
> var te = new TypeError;
> Object.prototype.toString.call(te)
> te.hasOwnProperty('stack');  // true
> te.__proto__.hasOwnProperty('stack');  // true
> 
> This seems really fishy to me. JSC seems to do this in a better way. It adds a "stack" property when an object is thrown that has Error.prototype on its prototype chain. So by fixing bug 85078 we will get JSC support for this bug for free.

JSC adds error info to _any_ object that is thrown, error or not -- we should already have a stack property attached to dom errors, if not could you file a jsc specific bug so i can try and work out what's going wrong?

[Erik Arvidsson]

(In reply to comment #8)
> JSC adds error info to _any_ object that is thrown, error or not -- we should already have a stack property attached to dom errors, if not could you file a jsc specific bug so i can try and work out what's going wrong?

Verified that JSC already adds the stack property.

[Yang Guo]

(In reply to comment #9)
> (In reply to comment #8)
> > JSC adds error info to _any_ object that is thrown, error or not -- we should already have a stack property attached to dom errors, if not could you file a jsc specific bug so i can try and work out what's going wrong?
> 
> Verified that JSC already adds the stack property.

(In reply to comment #4)
> (From update of attachment 139205 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139205&action=review
> 
> > Source/WebCore/bindings/v8/V8Proxy.cpp:579
> > +    v8::Handle<v8::Value> message = exceptionObject->Get(v8String("message", isolate));
> > +    v8::Handle<v8::Value> error = v8::Exception::Error(message->ToString());
> 
> Why do we need the message? Is that required to get the stack?

No, but necessary in order to make the stack trace look like the one that's attach to any Error object.

> 
> > Source/WebCore/bindings/v8/V8Proxy.cpp:580
> > +    exceptionObject->SetAccessor(v8String("stack", isolate), DOMExceptionStackGetter, 0, error);
> 
> What is the intention here? Make sure that setting stack also works since code out there might already do that.
> 
> > LayoutTests/fast/dom/DOMException/stack-trace.html:23
> > +    }
> 
> Please test that the descriptor looks like you expect it to.
> 
> It should look something like:
> 
> {
>   configurable: true,
>   enumerable: false,
>   get: function () { ... }
>   set: function () { ... }
> }
> 
> > LayoutTests/platform/chromium/fast/dom/DOMException/stack-trace-expected.txt:2
> > +layer at (0,0) size 800x600
> 
> Please add
> 
> layoutTestController.dumpAsText();
> 
> we definitely do not care about the layout here

Will do.

[Yang Guo]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Do you have any idea on the spec compliance?
> > 
> > WebIDL actually specs that DOM exceptions should have Error.prototype in its prototype chain. "stack" is not part of any standard but since we have it all instances that have Error.prototype in its prototype chain I don't think we are violating any spec by adding it ;-)
> > 
> > Look at how we handle TypeError for example:
> > 
> > var te = new TypeError;
> > Object.prototype.toString.call(te)
> > te.hasOwnProperty('stack');  // true
> > te.__proto__.hasOwnProperty('stack');  // true
> > 
> > This seems really fishy to me. JSC seems to do this in a better way. It adds a "stack" property when an object is thrown that has Error.prototype on its prototype chain. So by fixing bug 85078 we will get JSC support for this bug for free.
> 
> JSC adds error info to _any_ object that is thrown, error or not -- we should already have a stack property attached to dom errors, if not could you file a jsc specific bug so i can try and work out what's going wrong?

Initially I actually had an implementation that creates an Error object, copies all properties of the DOMException onto it and throws that Error object.  That way we indeed get the stack property for free, since the constructor of the Error object takes care of that. I'll upload a patch for that.

[Yang Guo]

Created attachment 140504 [details]
updated patch.

[Erik Arvidsson]

Comment on attachment 140504 [details]
updated patch.

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

Here are some more tests that we should do:

try {
    document.appendChild(document);
} catch (ex) {
    ex.stack = 42;  // we don't want to disallow this since it worked before
    assert(ex.stack === 42);
}


Object.defineProperty(DOMException.prototype, 'stack', {set: function() {
    alert('FAIL');  // We should not invoke setter
});
try {
    document.appendChild(document);
    alert('FAIL');
} catch (ex) {
    alert('PASS');
}

> LayoutTests/platform/chromium/fast/dom/DOMException/stack-trace-expected.txt:5
> +ALERT: get: undefined
> +ALERT: set: undefined

Error objects in V8 uses JS getters and setters. Are we sure we want a data property here?

[Erik Arvidsson]

Created attachment 141250 [details]
Patch

[Erik Arvidsson]

This patch does not use JS accessors (it uses V8 accessors) since V8 does not provide an API to define JS accessors.

However it does allow the stack property to be overridden.

[Kentaro Hara]

Comment on attachment 141250 [details]
Patch

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

The patch looks good.

> Source/WebCore/bindings/v8/V8Proxy.cpp:552
> +    return info.Data()->ToObject()->Get(v8String("stack", info.GetIsolate()));

It is wasteful to generate v8String() every time. But we cannot initialize the string statically because static variables are not Isolate-safe. Maybe we want some mechanism to have V8BindingPerIsolateData cache static v8::Handle s. You might want to fix it in a follow-up patch.

> Source/WebCore/bindings/v8/V8Proxy.cpp:557
> +    info.Data()->ToObject()->Set(v8String("stack", info.GetIsolate()), value);

Ditto.

> Source/WebCore/bindings/v8/V8Proxy.cpp:577
> +    if (exception.IsEmpty())

exception.IsObject() might be better?

> Source/WebCore/bindings/v8/V8Proxy.cpp:582
> +    v8::Handle<v8::Value> message = exceptionObject->Get(v8String("message", isolate));

Want to cache the string in V8BindingPerIsolateData. (a follow-up patch is OK.)

> Source/WebCore/bindings/v8/V8Proxy.cpp:584
> +    exceptionObject->SetAccessor(v8String("stack", isolate), DOMExceptionStackGetter, DOMExceptionStackSetter, error);

The same comment for "stack" string.

> LayoutTests/fast/dom/DOMException/stack-trace.html:35
> +shouldBeTrue('"stack" in e');
> +shouldBeEqualToString('typeof e.stack', 'string');
> +shouldBeTrue('e.stack.contains("innerFunction")');
> +shouldBeTrue('e.stack.contains("outerFunction")');

You might want to add some test cases for e.message, just in case.

[WebKit Review Bot]

Comment on attachment 141250 [details]
Patch

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

New failing tests:
webintents/web-intents-obj-constructor.html

[WebKit Review Bot]

Created attachment 141326 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  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 141448 [details]
Crash log for chromium mac debug

[Erik Arvidsson]

Created attachment 141503 [details]
Patch

[Erik Arvidsson]

(In reply to comment #20)
> Created an attachment (id=141503) [details]
> Patch

This patch differs from the last one in that it used description.description as the message for the Error instead of trying to Get the error property out of the wrapper.

[WebKit Review Bot]

Comment on attachment 141503 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/workers/abort-exception-assert.html

[WebKit Review Bot]

Created attachment 141531 [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

[Kentaro Hara]

Comment on attachment 141503 [details]
Patch

Looks good. Please fix the layout test failure.

[Erik Arvidsson]

(In reply to comment #24)
> (From update of attachment 141503 [details])
> Looks good. Please fix the layout test failure.

Your wish is my command.

[Erik Arvidsson]

Created attachment 141802 [details]
Patch for landing

[Erik Arvidsson]

Kentaro, FYI, the crash in abort-exception-assert.html was due to checking exception->IsObject() instead of !exception.IsEmpty(). It might be good to remember to not skip the IsEmpty check.

[Kentaro Hara]

(In reply to comment #27)
> Kentaro, FYI, the crash in abort-exception-assert.html was due to checking exception->IsObject() instead of !exception.IsEmpty(). It might be good to remember to not skip the IsEmpty check.

But in that case, shouldn't the check be 'exception->IsEmpty() || !exception->IsObject()'? In other words, is it guaranteed that exception->ToObject() succeeds if exception->IsEmpty() is false?

[Erik Arvidsson]

(In reply to comment #28)
> (In reply to comment #27)
> > Kentaro, FYI, the crash in abort-exception-assert.html was due to checking exception->IsObject() instead of !exception.IsEmpty(). It might be good to remember to not skip the IsEmpty check.
> 
> But in that case, shouldn't the check be 'exception->IsEmpty() || !exception->IsObject()'? In other words, is it guaranteed that exception->ToObject() succeeds if exception->IsEmpty() is false?

In this case it is guaranteed but using an ASSERT seems like the right thing to do to me.

[Erik Arvidsson]

Created attachment 141808 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 141808 [details]
Patch for landing

Clearing flags on attachment: 141808

Committed r117016: <http://trac.webkit.org/changeset/117016>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

On EFL build bot, the new test case is failing. The reason for that is that these lines:
  innerFunction@file:///src/WebKit/LayoutTests/fast/dom/DOMException/stack-trace.html:17
  11outerFunction@file:///src/WebKit/LayoutTests/fast/dom/DOMException/stack-trace.html:21

Have absolute path, eg. ."file:///home/chris/.../WebKit/LayoutTests/fast/dom/DOMException/stack-trace.html:17"

[Erik Arvidsson]

(In reply to comment #33)
> On EFL build bot, the new test case is failing. The reason for that is that these lines:
>   innerFunction@file:///src/WebKit/LayoutTests/fast/dom/DOMException/stack-trace.html:17
>   11outerFunction@file:///src/WebKit/LayoutTests/fast/dom/DOMException/stack-trace.html:21
> 
> Have absolute path, eg. ."file:///home/chris/.../WebKit/LayoutTests/fast/dom/DOMException/stack-trace.html:17"

See bug 86442

The paths are only printed when the test fails... but I am going to change this so that it only prints FAIL in that case.