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.
Created attachment 139196 [details] Patch to add desired functionality into V8 binding. The stack property is implemented as a getter.
Created attachment 139204 [details] Updated patch removing dependency to path names from the test expectations. Supersedes the previous patch.
Created attachment 139205 [details] Updated patch.
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
Is your plan to only do this for v8?
Do you have any idea on the spec compliance?
(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.
(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?
(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 #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.
(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.
Created attachment 140504 [details] updated patch.
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?
Created attachment 141250 [details] Patch
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 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 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
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
Created attachment 141448 [details] Crash log for chromium mac debug
Created attachment 141503 [details] Patch
(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 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
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 on attachment 141503 [details] Patch Looks good. Please fix the layout test failure.
(In reply to comment #24) > (From update of attachment 141503 [details]) > Looks good. Please fix the layout test failure. Your wish is my command.
Created attachment 141802 [details] Patch for landing
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.
(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 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.
Created attachment 141808 [details] Patch for landing
Comment on attachment 141808 [details] Patch for landing Clearing flags on attachment: 141808 Committed r117016: <http://trac.webkit.org/changeset/117016>
All reviewed patches have been landed. Closing bug.
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"
(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.