Bug 85057 - [V8] Add "stack" property to DOMException
Summary: [V8] Add "stack" property to DOMException
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on: 86128 85078 86442
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-27 06:09 PDT by Yang Guo
Modified: 2012-05-15 09:32 PDT (History)
9 users (show)

See Also:


Attachments
Patch to add desired functionality into V8 binding. The stack property is implemented as a getter. (4.85 KB, patch)
2012-04-27 07:08 PDT, Yang Guo
no flags Details | Formatted Diff | Diff
Updated patch removing dependency to path names from the test expectations. Supersedes the previous patch. (4.78 KB, patch)
2012-04-27 08:07 PDT, Yang Guo
no flags Details | Formatted Diff | Diff
Updated patch. (4.78 KB, patch)
2012-04-27 08:08 PDT, Yang Guo
no flags Details | Formatted Diff | Diff
updated patch. (4.63 KB, patch)
2012-05-07 03:49 PDT, Yang Guo
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2012-05-10 14:15 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (817.59 KB, application/zip)
2012-05-10 22:13 PDT, WebKit Review Bot
no flags Details
Crash log for chromium mac debug (10.23 KB, text/plain)
2012-05-11 10:57 PDT, Erik Arvidsson
no flags Details
Patch (6.77 KB, patch)
2012-05-11 15:03 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (796.76 KB, application/zip)
2012-05-11 17:14 PDT, WebKit Review Bot
no flags Details
Patch for landing (6.79 KB, patch)
2012-05-14 15:53 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (6.92 KB, patch)
2012-05-14 16:33 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yang Guo 2012-04-27 06:09:19 PDT
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.
Comment 1 Yang Guo 2012-04-27 07:08:43 PDT
Created attachment 139196 [details]
Patch to add desired functionality into V8 binding. The stack property is implemented as a getter.
Comment 2 Yang Guo 2012-04-27 08:07:58 PDT
Created attachment 139204 [details]
Updated patch removing dependency to path names from the test expectations. Supersedes the previous patch.
Comment 3 Yang Guo 2012-04-27 08:08:26 PDT
Created attachment 139205 [details]
Updated patch.
Comment 4 Erik Arvidsson 2012-04-27 10:18:30 PDT
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
Comment 5 Alexey Proskuryakov 2012-04-27 10:22:32 PDT
Is your plan to only do this for v8?
Comment 6 Kentaro Hara 2012-04-27 10:23:31 PDT
Do you have any idea on the spec compliance?
Comment 7 Erik Arvidsson 2012-04-27 11:33:28 PDT
(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.
Comment 8 Oliver Hunt 2012-04-27 11:46:49 PDT
(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?
Comment 9 Erik Arvidsson 2012-04-27 11:56:48 PDT
(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.
Comment 10 Yang Guo 2012-04-27 22:53:55 PDT
(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.
Comment 11 Yang Guo 2012-04-27 22:56:14 PDT
(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.
Comment 12 Yang Guo 2012-05-07 03:49:17 PDT
Created attachment 140504 [details]
updated patch.
Comment 13 Erik Arvidsson 2012-05-07 09:39:05 PDT
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?
Comment 14 Erik Arvidsson 2012-05-10 14:15:44 PDT
Created attachment 141250 [details]
Patch
Comment 15 Erik Arvidsson 2012-05-10 14:19:13 PDT
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.
Comment 16 Kentaro Hara 2012-05-10 16:42:26 PDT
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.
Comment 17 WebKit Review Bot 2012-05-10 22:13:26 PDT
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
Comment 18 WebKit Review Bot 2012-05-10 22:13:31 PDT
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
Comment 19 Erik Arvidsson 2012-05-11 10:57:30 PDT
Created attachment 141448 [details]
Crash log for chromium mac debug
Comment 20 Erik Arvidsson 2012-05-11 15:03:09 PDT
Created attachment 141503 [details]
Patch
Comment 21 Erik Arvidsson 2012-05-11 15:10:06 PDT
(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.
Comment 22 WebKit Review Bot 2012-05-11 17:14:19 PDT
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
Comment 23 WebKit Review Bot 2012-05-11 17:14:24 PDT
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
Comment 24 Kentaro Hara 2012-05-13 16:34:05 PDT
Comment on attachment 141503 [details]
Patch

Looks good. Please fix the layout test failure.
Comment 25 Erik Arvidsson 2012-05-14 13:55:45 PDT
(In reply to comment #24)
> (From update of attachment 141503 [details])
> Looks good. Please fix the layout test failure.

Your wish is my command.
Comment 26 Erik Arvidsson 2012-05-14 15:53:55 PDT
Created attachment 141802 [details]
Patch for landing
Comment 27 Erik Arvidsson 2012-05-14 15:55:45 PDT
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.
Comment 28 Kentaro Hara 2012-05-14 16:21:02 PDT
(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?
Comment 29 Erik Arvidsson 2012-05-14 16:22:46 PDT
(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.
Comment 30 Erik Arvidsson 2012-05-14 16:33:53 PDT
Created attachment 141808 [details]
Patch for landing
Comment 31 WebKit Review Bot 2012-05-14 16:54:07 PDT
Comment on attachment 141808 [details]
Patch for landing

Clearing flags on attachment: 141808

Committed r117016: <http://trac.webkit.org/changeset/117016>
Comment 32 WebKit Review Bot 2012-05-14 16:54:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Chris Dumez 2012-05-14 23:12:57 PDT
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"
Comment 34 Erik Arvidsson 2012-05-15 09:32:09 PDT
(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.