[JSC] retain original stacktrace when rethrowing an ErrorInstance
Created attachment 315147 [details] Patch
Created attachment 315149 [details] Patch
Comment on attachment 315149 [details] Patch Attachment 315149 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4102617 New failing tests: http/tests/security/regress-52192.html imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.html imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html
Created attachment 315166 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 315149 [details] Patch Attachment 315149 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4102650 New failing tests: imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.html http/tests/security/regress-52192.html imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html
Created attachment 315168 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 315149 [details] Patch Attachment 315149 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4102659 New failing tests: imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.html http/tests/security/regress-52192.html imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html
Created attachment 315170 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 315149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315149&action=review > Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp:172 > + if (ErrorInstance* errorInstance = jsDynamicCast<ErrorInstance*>(exec->vm(), exception->value())) { It might make sense to use auto* here and below in similar statements, because type is already written out on the right side as js(Dynamic)Cast<ErrorInstance*>
Comment on attachment 315149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315149&action=review r- since the bots are red, and also because I suspect this is not the right fix for the issue. I would like to understand better what the actual issue is first. > Source/JavaScriptCore/ChangeLog:16 > + Associate the Exception object from the first time an ErrorInstance > + object has been thrown, and use that original Exception to retain > + the original stacktrace. This is useful when reporting uncaught > + exceptions which have been rethrown at some point. > + > + This behaviour matches the behaviour of Chromium and Firefox, in that > + only subclasses of Error retain the stacktrace. Non-ErrorInstance > + exceptions will continue to produce an incorrect stacktrace when > + rethrown. I do not understand why this is the desired behavior. The Exception object captures where the object was last thrown from. When would the Exception object's stack not show where the ErrorInstance was thrown from? Also, if the ErrorInstance was caught and then rethrown, shouldn't we be showing the new stack for the new throw location? Hence, something doesn't add up here. Can you please give an example of what error is being observed by the user? I suspect your fix is not correct. > Source/JavaScriptCore/runtime/Exception.cpp:79 > + if (thrownValue.isObject() && thrownValue.inherits(vm, ErrorInstance::info())) { > + ErrorInstance* errorInstance = jsCast<ErrorInstance*>(thrownValue); > + errorInstance->setFirstThrownException(vm, this); > + } You can simplify this as: if (auto* errorInstance = jsDynamicCast<ErrorInstance>(thrownValue)) errorInstance->setFirstThrownException(vm, this);
Is there a test that covers this, or a test we can add?
Comment on attachment 315149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315149&action=review Thanks for the quick look, I'll address these shortly. I assume this cycle (ErrorInstance <-> Exception) doesn't leak (assuming Exceptions aren't treated as roots), but is that actually true? Not sure if the EWS failures are actually me or not. >> Source/JavaScriptCore/ChangeLog:16 >> + rethrown. > > I do not understand why this is the desired behavior. The Exception object captures where the object was last thrown from. When would the Exception object's stack not show where the ErrorInstance was thrown from? Also, if the ErrorInstance was caught and then rethrown, shouldn't we be showing the new stack for the new throw location? Hence, something doesn't add up here. Can you please give an example of what error is being observed by the user? I suspect your fix is not correct. The problem is with messages in the inspector (such as, for uncaught Exceptions). When an ErrorInstance is rethrown, the rethrown Exception's stack is used, and the original stack is unavailable. This can make it hard to identify the original point of failure, which is why bug 167934 was filed. Other browsers only show the original stack for Error objects, though.
(In reply to Caitlin Potter (:caitp) from comment #12) > Comment on attachment 315149 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315149&action=review > > Thanks for the quick look, I'll address these shortly. > > I assume this cycle (ErrorInstance <-> Exception) doesn't leak (assuming > Exceptions aren't treated as roots), but is that actually true? Not sure if > the EWS failures are actually me or not. > > >> Source/JavaScriptCore/ChangeLog:16 > >> + rethrown. > > > > I do not understand why this is the desired behavior. The Exception object captures where the object was last thrown from. When would the Exception object's stack not show where the ErrorInstance was thrown from? Also, if the ErrorInstance was caught and then rethrown, shouldn't we be showing the new stack for the new throw location? Hence, something doesn't add up here. Can you please give an example of what error is being observed by the user? I suspect your fix is not correct. > > The problem is with messages in the inspector (such as, for uncaught > Exceptions). > > When an ErrorInstance is rethrown, the rethrown Exception's stack is used, > and the original stack is unavailable. This can make it hard to identify the > original point of failure, which is why bug 167934 was filed. Other browsers > only show the original stack for Error objects, though. Please provide a series of steps that reproduces this bug. From your description, the uncaught exception should have the original stack trace. If it does not, then by definition, it was caught (i.e. not uncaught) and someone else threw it again. Hence, it is proper for this the uncaught exception to report this second stack trace because that's the one that is uncaught. If the error wasn't actually caught in between by some JS code, but was actually rethrown by some C++ code that replaced the stack, then the issue lies in the code that is supposed to chain the exception. I still do not see a case where we should ever replace the exception stack like this with a prior one that was caught. So, if you can provide some repro steps, we can take a closer look at what actually went wrong. Thanks.
(In reply to Mark Lam from comment #13) > (In reply to Caitlin Potter (:caitp) from comment #12) > > Comment on attachment 315149 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=315149&action=review > > > > Thanks for the quick look, I'll address these shortly. > > > > I assume this cycle (ErrorInstance <-> Exception) doesn't leak (assuming > > Exceptions aren't treated as roots), but is that actually true? Not sure if > > the EWS failures are actually me or not. > > > > >> Source/JavaScriptCore/ChangeLog:16 > > >> + rethrown. > > > > > > I do not understand why this is the desired behavior. The Exception object captures where the object was last thrown from. When would the Exception object's stack not show where the ErrorInstance was thrown from? Also, if the ErrorInstance was caught and then rethrown, shouldn't we be showing the new stack for the new throw location? Hence, something doesn't add up here. Can you please give an example of what error is being observed by the user? I suspect your fix is not correct. > > > > The problem is with messages in the inspector (such as, for uncaught > > Exceptions). > > > > When an ErrorInstance is rethrown, the rethrown Exception's stack is used, > > and the original stack is unavailable. This can make it hard to identify the > > original point of failure, which is why bug 167934 was filed. Other browsers > > only show the original stack for Error objects, though. > > Please provide a series of steps that reproduces this bug. From your > description, the uncaught exception should have the original stack trace. > If it does not, then by definition, it was caught (i.e. not uncaught) and > someone else threw it again. Hence, it is proper for this the uncaught > exception to report this second stack trace because that's the one that is > uncaught. > > If the error wasn't actually caught in between by some JS code, but was > actually rethrown by some C++ code that replaced the stack, then the issue > lies in the code that is supposed to chain the exception. I still do not > see a case where we should ever replace the exception stack like this with a > prior one that was caught. > > So, if you can provide some repro steps, we can take a closer look at what > actually went wrong. Thanks. Note there's also a difference between Error.stack (which is the stack captured at the time that the ErrorInstance was instantiated) and the Exception object's stack (which is the stack captured at the time that the ErrorInstance was thrown). Perhaps there is an Inspector bug that is showing the throw point stack when it should be showing Error.stack. Regardless, if you provide a test case and repro steps, we'll be able to see better what the issue is.
Created attachment 315191 [details] With patch applied Reproduction with patch applied in MiniBrowser
Comment on attachment 315149 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315149&action=review >>>> Source/JavaScriptCore/ChangeLog:16 >>>> + rethrown. >>> >>> I do not understand why this is the desired behavior. The Exception object captures where the object was last thrown from. When would the Exception object's stack not show where the ErrorInstance was thrown from? Also, if the ErrorInstance was caught and then rethrown, shouldn't we be showing the new stack for the new throw location? Hence, something doesn't add up here. Can you please give an example of what error is being observed by the user? I suspect your fix is not correct. >> >> The problem is with messages in the inspector (such as, for uncaught Exceptions). >> >> When an ErrorInstance is rethrown, the rethrown Exception's stack is used, and the original stack is unavailable. This can make it hard to identify the original point of failure, which is why bug 167934 was filed. Other browsers only show the original stack for Error objects, though. > > Please provide a series of steps that reproduces this bug. From your description, the uncaught exception should have the original stack trace. If it does not, then by definition, it was caught (i.e. not uncaught) and someone else threw it again. Hence, it is proper for this the uncaught exception to report this second stack trace because that's the one that is uncaught. > > If the error wasn't actually caught in between by some JS code, but was actually rethrown by some C++ code that replaced the stack, then the issue lies in the code that is supposed to chain the exception. I still do not see a case where we should ever replace the exception stack like this with a prior one that was caught. > > So, if you can provide some repro steps, we can take a closer look at what actually went wrong. Thanks. Here's a minimal reproduction: https://jsfiddle.net/om4pzc38/ (further reproduction is on the bug marked as a blocker). Look in the inspector, and notice that the rethrown uncaught exception appears to lose its original stacktrace. With the appatch applied, looks like https://bug-174380-attachments.webkit.org/attachment.cgi?id=315191
(In reply to Mark Lam from comment #14) > (In reply to Mark Lam from comment #13) > > (In reply to Caitlin Potter (:caitp) from comment #12) > > > Comment on attachment 315149 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=315149&action=review > > > > > > Thanks for the quick look, I'll address these shortly. > > > > > > I assume this cycle (ErrorInstance <-> Exception) doesn't leak (assuming > > > Exceptions aren't treated as roots), but is that actually true? Not sure if > > > the EWS failures are actually me or not. > > > > > > >> Source/JavaScriptCore/ChangeLog:16 > > > >> + rethrown. > > > > > > > > I do not understand why this is the desired behavior. The Exception object captures where the object was last thrown from. When would the Exception object's stack not show where the ErrorInstance was thrown from? Also, if the ErrorInstance was caught and then rethrown, shouldn't we be showing the new stack for the new throw location? Hence, something doesn't add up here. Can you please give an example of what error is being observed by the user? I suspect your fix is not correct. > > > > > > The problem is with messages in the inspector (such as, for uncaught > > > Exceptions). > > > > > > When an ErrorInstance is rethrown, the rethrown Exception's stack is used, > > > and the original stack is unavailable. This can make it hard to identify the > > > original point of failure, which is why bug 167934 was filed. Other browsers > > > only show the original stack for Error objects, though. > > > > Please provide a series of steps that reproduces this bug. From your > > description, the uncaught exception should have the original stack trace. > > If it does not, then by definition, it was caught (i.e. not uncaught) and > > someone else threw it again. Hence, it is proper for this the uncaught > > exception to report this second stack trace because that's the one that is > > uncaught. > > > > If the error wasn't actually caught in between by some JS code, but was > > actually rethrown by some C++ code that replaced the stack, then the issue > > lies in the code that is supposed to chain the exception. I still do not > > see a case where we should ever replace the exception stack like this with a > > prior one that was caught. > > > > So, if you can provide some repro steps, we can take a closer look at what > > actually went wrong. Thanks. > > Note there's also a difference between Error.stack (which is the stack > captured at the time that the ErrorInstance was instantiated) and the > Exception object's stack (which is the stack captured at the time that the > ErrorInstance was thrown). Perhaps there is an Inspector bug that is > showing the throw point stack when it should be showing Error.stack. > Regardless, if you provide a test case and repro steps, we'll be able to see > better what the issue is. Yeah, I've been trying to explain (by pointing to the referenced issue) that this is an inspector bug. It's unrelated to accesses to `error.stack`, but affects Console.StackTrace.UncaughtException (and possibly other cases).
> Here's a minimal reproduction: https://jsfiddle.net/om4pzc38/ (further > reproduction is on the bug marked as a blocker). Look in the inspector, and > notice that the rethrown uncaught exception appears to lose its original > stacktrace. I think what Web Inspector shows is intentional. Take for example this test case: <script> let error = null; function a() { b(); } function b() { c(); } function c() { d(); } function d() { try { [].x.x } catch (e) { error = e }; } function z() { throw error; } a(); console.log(error.stack); z(); </script> • An exception is thrown in a() => b() => c() => d() and caught by user code. • Then, that error object is then thrown in z() and uncaught. Web Inspector shows: The Error (and its `stack` property) shows where the error was originally thrown: (d, c, b, a) [Log] TypeError: undefined is not an object (evaluating '[].x.x') (error-test.html, line 8) d — error-test.html:6 c — error-test.html:5 b — error-test.html:4 a — error-test.html:3 Global Code — error-test.html:8 The Uncaught Exception message shows where the uncaught exception was thrown: (z) [Error] TypeError: undefined is not an object (evaluating '[].x.x') z (error-test.html:7) Global Code (error-test.html:8) Both are what I would expect to see. You are correct in that Chrome and Firefox in their tools differ, but I'm not sure what they have is better. Since it is not showing you where the uncaught exception is thrown from, which is valuable information to have.
(In reply to Joseph Pecoraro from comment #18) > > Here's a minimal reproduction: https://jsfiddle.net/om4pzc38/ (further > > reproduction is on the bug marked as a blocker). Look in the inspector, and > > notice that the rethrown uncaught exception appears to lose its original > > stacktrace. > > I think what Web Inspector shows is intentional. > > Take for example this test case: > > <script> > let error = null; > function a() { b(); } > function b() { c(); } > function c() { d(); } > function d() { try { [].x.x } catch (e) { error = e }; } > function z() { throw error; } > a(); console.log(error.stack); z(); > </script> > > • An exception is thrown in a() => b() => c() => d() and caught by user code. > • Then, that error object is then thrown in z() and uncaught. > > Web Inspector shows: > > The Error (and its `stack` property) shows where the error was originally > thrown: (d, c, b, a) > > [Log] TypeError: undefined is not an object (evaluating '[].x.x') > (error-test.html, line 8) > d — error-test.html:6 > c — error-test.html:5 > b — error-test.html:4 > a — error-test.html:3 > Global Code — error-test.html:8 > > The Uncaught Exception message shows where the uncaught exception was > thrown: (z) > > [Error] TypeError: undefined is not an object (evaluating '[].x.x') > z (error-test.html:7) > Global Code (error-test.html:8) > > Both are what I would expect to see. > > You are correct in that Chrome and Firefox in their tools differ, but I'm > not sure what they have is better. Since it is not showing you where the > uncaught exception is thrown from, which is valuable information to have. I can see the value in showing the stack at the rethrown location too, but I think it's hard to see the value of the Chromium behaviour from such a simple example. In complex apps, it can be hard to identify where the exception originally came from. For example, if liveedit is unavailable (I don't know if WebKit's inspector supports liveedit at all --- does it?), it could be very hard to add a log statement where the exception was rethrown. Even if it were possible, the exception might not be something which occurs deterministically, and who knows when the next chance to inspect it might be? There are good reasons why other browsers choose to show the original stack in the inspector. Now, it could be very simple to combine both stacks for the message. That might be something worth trying.
> I can see the value in showing the stack at the rethrown location too, but I > think it's hard to see the value of the Chromium behaviour from such a > simple example. In complex apps, it can be hard to identify where the > exception originally came from. For example, if liveedit is unavailable (I > don't know if WebKit's inspector supports liveedit at all --- does it?), it > could be very hard to add a log statement where the exception was rethrown. > Even if it were possible, the exception might not be something which occurs > deterministically, and who knows when the next chance to inspect it might > be? There are good reasons why other browsers choose to show the original > stack in the inspector. Right now, if you enable Pause at Uncaught Exceptions and encounter an issue with Web Inspector open, then you would pause and be able to examine the error object being re-thrown to see its original stack. That gets you the best of both worlds. Likewise, in all browsers if user script handles the error, with `window.onerror` for example, they will all get the original error stack and have equivalent behavior. This is about improving the uncaught exception console message in cases of rethrown errors. > Now, it could be very simple to combine both stacks for the message. That > might be something worth trying. I would agree with this. Would it make sense to retitle this to be like one of these? Web Inspector: Include original error stack in uncaught exception console messages for rethrown errors Web Inspector: Include exception value in uncaught exception console messages I think a couple possible UIs would be: (1) If the Uncaught Exception is a rethrown Error, Combine the Stacks: [Error] TypeError: undefined is not an object (evaluating '[].x.x') z (error-test.html:7) Global Code (error-test.html:8) ---- original error stack ---- d (error-test.html:6) c (error-test.html:5) b (error-test.html:4) a (error-test.html:3) Global Code — error-test.html:8 (2) For any Uncaught Exception console message, include the thrown value in the message - This is kind of like `console.log("message", object)`, where you can interact with object - If the object is a rethrown error, users can examine look at the object's stack property // throw myObject [Error] [Object MyObject] z (error-test.html:7) Global Code (error-test.html:8) ▶︎ MyObject { ... } // throw error [Error] TypeError: undefined is not an object (evaluating '[].x.x') z (error-test.html:7) Global Code (error-test.html:8) ▼ TypeError: undefined is not an object (evaluating '[].x.x') d (error-test.html:6) c (error-test.html:5) b (error-test.html:4) a (error-test.html:3) Global Code — error-test.html:8 (3) A hybrid of the two approaches The advantage of (1) is that we would record the stack even if Web Inspector is not open. So that if an uncaught exception happens before the tools are connected, opening Web Inspector would show up the uncaught stack and original error stack.
(In reply to Joseph Pecoraro from comment #20) > > I can see the value in showing the stack at the rethrown location too, but I > > think it's hard to see the value of the Chromium behaviour from such a > > simple example. In complex apps, it can be hard to identify where the > > exception originally came from. For example, if liveedit is unavailable (I > > don't know if WebKit's inspector supports liveedit at all --- does it?), it > > could be very hard to add a log statement where the exception was rethrown. > > Even if it were possible, the exception might not be something which occurs > > deterministically, and who knows when the next chance to inspect it might > > be? There are good reasons why other browsers choose to show the original > > stack in the inspector. > > Right now, if you enable Pause at Uncaught Exceptions and encounter an issue > with Web Inspector open, then you would pause and be able to examine the > error object being re-thrown to see its original stack. That gets you the > best of both worlds. > > Likewise, in all browsers if user script handles the error, with > `window.onerror` for example, they will all get the original error stack and > have equivalent behavior. There are certainly workarounds. I'm working on this (at the request of my colleagues) this because presumably the workarounds are less friendly for developers (though I don't know who those developers are or what their specific pain points are). > > This is about improving the uncaught exception console message in cases of > rethrown errors. > > > > Now, it could be very simple to combine both stacks for the message. That > > might be something worth trying. > > I would agree with this. Would it make sense to retitle this to be like one > of these? > > Web Inspector: Include original error stack in uncaught exception > console messages for rethrown errors > Web Inspector: Include exception value in uncaught exception console > messages > > I think a couple possible UIs would be: > > (1) If the Uncaught Exception is a rethrown Error, Combine the Stacks: > > [Error] TypeError: undefined is not an object (evaluating '[].x.x') > z (error-test.html:7) > Global Code (error-test.html:8) > ---- original error stack ---- > d (error-test.html:6) > c (error-test.html:5) > b (error-test.html:4) > a (error-test.html:3) > Global Code — error-test.html:8 > > (2) For any Uncaught Exception console message, include the thrown value > in the message > - This is kind of like `console.log("message", object)`, where you can > interact with object > - If the object is a rethrown error, users can examine look at the > object's stack property > > // throw myObject > [Error] [Object MyObject] > z (error-test.html:7) > Global Code (error-test.html:8) > ▶︎ MyObject { ... } > > // throw error > [Error] TypeError: undefined is not an object (evaluating '[].x.x') > z (error-test.html:7) > Global Code (error-test.html:8) > ▼ TypeError: undefined is not an object (evaluating '[].x.x') > d (error-test.html:6) > c (error-test.html:5) > b (error-test.html:4) > a (error-test.html:3) > Global Code — error-test.html:8 > > (3) A hybrid of the two approaches > > The advantage of (1) is that we would record the stack even if Web Inspector > is not open. So that if an uncaught exception happens before the tools are > connected, opening Web Inspector would show up the uncaught stack and > original error stack. What do you think would be the easiest to upstream? Option 2 seems like the least invasive. I believe Chromium is doing something closer to option 3, a hybrid of both (but still showing only the original stacktrace).
(In reply to Joseph Pecoraro from comment #20) > I think a couple possible UIs would be: > > (1) If the Uncaught Exception is a rethrown Error, Combine the Stacks: > > [Error] TypeError: undefined is not an object (evaluating '[].x.x') > z (error-test.html:7) > Global Code (error-test.html:8) > ---- original error stack ---- > d (error-test.html:6) > c (error-test.html:5) > b (error-test.html:4) > a (error-test.html:3) > Global Code — error-test.html:8 This solution does not scale, and is not performant. Imagine a scenario where an object is caught, rethrown, caught, rethrown, ... and repeat N times. Which stack do you keep then? Only the first one? All of them? Keeping the first one won't solve the problem for the other rethrow cases. Logging all of them will easily add up in memory consumption and processing time, and usually for no gain because no one will want these stacks (until they do). For this reason, I don't agree with (3) either. Instead, I think what would be useful would be able to automatically watch objects that are thrown. To achieve this, I propose adding the ability to attach breakpoint actions to breaks on all / uncaught exceptions thrown. This way the user can dump the thrown object and the stack on each throw, and get the data they want. The action can also be customized to the idiosyncrasies needs of the codebase being debugged. Alternatively, the action can capture the stack dump and stash it in a global dictionary. With this, the user can go back and slowly inspect each throw location as needed. The user can also have the action only capture the stack dump on the first throw of an Error. This satisfies the original problem Caitlin was trying to solve. I think this solution is more generic and useful, and it doesn't force the baggage of carrying more exception stack traces than most people would want.
(In reply to Mark Lam from comment #22) > (In reply to Joseph Pecoraro from comment #20) > > I think a couple possible UIs would be: > > > > (1) If the Uncaught Exception is a rethrown Error, Combine the Stacks: > > > > [Error] TypeError: undefined is not an object (evaluating '[].x.x') > > z (error-test.html:7) > > Global Code (error-test.html:8) > > ---- original error stack ---- > > d (error-test.html:6) > > c (error-test.html:5) > > b (error-test.html:4) > > a (error-test.html:3) > > Global Code — error-test.html:8 > > > This solution does not scale, and is not performant. Imagine a scenario > where an object is caught, rethrown, caught, rethrown, ... and repeat N > times. Which stack do you keep then? Only the first one? All of them? > Keeping the first one won't solve the problem for the other rethrow cases. > Logging all of them will easily add up in memory consumption and processing > time, and usually for no gain because no one will want these stacks (until > they do). > > For this reason, I don't agree with (3) either. > > Instead, I think what would be useful would be able to automatically watch > objects that are thrown. To achieve this, I propose adding the ability to > attach breakpoint actions to breaks on all / uncaught exceptions thrown. > This way the user can dump the thrown object and the stack on each throw, > and get the data they want. The action can also be customized to the > idiosyncrasies needs of the codebase being debugged. > > Alternatively, the action can capture the stack dump and stash it in a > global dictionary. With this, the user can go back and slowly inspect each > throw location as needed. > The user can also have the action only capture the stack dump on the first > throw of an Error. This satisfies the original problem Caitlin was trying > to solve. > > I think this solution is more generic and useful, and it doesn't force the > baggage of carrying more exception stack traces than most people would want. I'm not sure I agree that this is the most developer-friendly and useful way to get at the original error stacks. Breaking on every thrown exception (not just uncaught) could take a very long time to sort through. Unlike lldb, there isn't an option (as far as I can tell) to skip a fixed number of breakpoints to get past ones you don't care about. Even if there were, that's only helpful in cases where the behaviour is deterministic and the same number of throws always occur. Saving a collection of Error objects in userspace is better, but there's a bit of an immediacy issue. Keeping track of which exception was actually important for your purposes, etc. It's very immediate and useful to be able to click on the file/line no link in the inspector log and jump immediately to the location the error came from, originally, and inspect the source around it to analyze the issue. Not to mention, logging these to the inspector is virtually automatic, whereas setting up more complicated setups (just to analyze a single exception) take more time and energy. So, the exception trace (in the Inspector) from a rethrown error is not particularly useful for anything --- it provides very little context for what actually produced the error. It makes sense for the inspector to print this original context. I don't think this requires saving piles and piles of traces, and don't think it's important to log every single stacktrace for each time the same exception is rethrown. The one that is most important is the original trace (actually provides some context indicating where the error came from), and the one (of secondary or no importance) is the final rethrow leading to logging an uncaught exception. ---- Anyways, it looked like those were objections to options 1) and 3). Does that mean you'd be satisfied with 2)?
(In reply to Caitlin Potter (:caitp) from comment #23) > Anyways, it looked like those were objections to options 1) and 3). Does > that mean you'd be satisfied with 2)? I agree that option 2 is a good solution. It is the least invasive and it'd probably be easiest to get working. I should have read more carefully before I made my proposal, though I still think that if we have breakpoint actions on exception breakpoints, the user can do much more. I do agree that option 2 is good default behavior to have, and it is less effort and friction to get that minimum set of info. I disagree with the characterization that knowing the final throw location is of secondary or no importance. It all depends on the scenario you are trying to debug. In your case, it happens to be the case that you have intermediate code that is doing this catching and re-throwing, and the bug you're interested in at the current moment happens to not be in the intermediate code itself. If the bug is in the intermediate code itself, then suddenly, knowing the last throw location will become immensely valuable. Anyway, I believe we can move forward with option 2.
Created attachment 315679 [details] Patch
Comment on attachment 315679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315679&action=review > Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp:174 > + if (firstThrownException != exception && maxStackSize >= 5) { as discussed offline, this seemed like the simplest way to include both the original and rethrown stack in the message. The UI and localization of this could be improved by building on top of the protocol extensions for async stacktraces, or extending the protocol separately for non-async traces.
Comment on attachment 315679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315679&action=review >> Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp:174 >> + if (firstThrownException != exception && maxStackSize >= 5) { > > as discussed offline, this seemed like the simplest way to include both the original and rethrown stack in the message. The UI and localization of this could be improved by building on top of the protocol extensions for async stacktraces, or extending the protocol separately for non-async traces. I think we have a misunderstanding. My understanding is that we'll use Error.stack here, not an additional capture of the throw stack. We also agreed on going with option 2, not option 1. I think you've implemented option 1 here. That said ... On my side, we've discussed this issue further, and we think that we'll eventually go with appending the throw stack every time the exception is thrown / re-thrown (with some edge cases e.g. special handling for StackOverflowError, and OutOfMemory) i.e. we'll be switching to option 1 (with some limitations to keep memory usage sane). However, in order to do that properly, we'll need to modify how re-throws work in catch blocks at the bytecode level. We also don't want to append an additional stack if the new throw stack is the same as the existing throw stack. That said, for this immediate bug, can we go with: 1. If thrown value is an Error instance, report Error.stack. 2. If Error.stack is different than the Exception throw stack, then append "thrown from" Exception stack. Else, don't append anything. 3. Don't add the firstThrownException to the Error instance. I think Chrome and FF aren't actually reporting the first thrown stack. They are only reporting Error.stack (I've confirmed this for Chrome). Hence, the above will give you the info you want. Do you agree?
(In reply to Mark Lam from comment #27) > Comment on attachment 315679 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315679&action=review > > >> Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp:174 > >> + if (firstThrownException != exception && maxStackSize >= 5) { > > > > as discussed offline, this seemed like the simplest way to include both the original and rethrown stack in the message. The UI and localization of this could be improved by building on top of the protocol extensions for async stacktraces, or extending the protocol separately for non-async traces. > > I think we have a misunderstanding. My understanding is that we'll use > Error.stack here, not an additional capture of the throw stack. We also > agreed on going with option 2, not option 1. I think you've implemented > option 1 here. That said ... There was some offline discussion with Joseph, where this approach was suggested, so that a useful stack is provided when the inspector is not open at the time the error is logged. Option 2) does not support this due to not retaining each thrown value when the exception is closed. That's what the last patch set went about implementing, though the comment on the patch should clarify that it's likely not the final approach to go with. > On my side, we've discussed this issue further, and we think that we'll > eventually go with appending the throw stack every time the exception is > thrown / re-thrown (with some edge cases e.g. special handling for > StackOverflowError, and OutOfMemory) i.e. we'll be switching to option 1 > (with some limitations to keep memory usage sane). However, in order to do > that properly, we'll need to modify how re-throws work in catch blocks at > the bytecode level. We also don't want to append an additional stack if the > new throw stack is the same as the existing throw stack. > > That said, for this immediate bug, can we go with: > 1. If thrown value is an Error instance, report Error.stack. > 2. If Error.stack is different than the Exception throw stack, then append > "thrown from" Exception stack. Else, don't append anything. > 3. Don't add the firstThrownException to the Error instance. > > I think Chrome and FF aren't actually reporting the first thrown stack. > They are only reporting Error.stack (I've confirmed this for Chrome). > Hence, the above will give you the info you want. Do you agree? I agree with this. The issue is the constraint with the original thrown value not being retained for logging to the console if the inspector is not open. I'm not sure if this constraint is considered equally important by all parties here, and implementing a simpler version of Option 1 actually turned out to be easier than Option 2, though I admit that it will get more complicated if changes to bytecode generation and/or the inspector protocol end up being required.
(In reply to Mark Lam from comment #27) > That said, for this immediate bug, can we go with: > 1. If thrown value is an Error instance, report Error.stack. > 2. If Error.stack is different than the Exception throw stack, then append > "thrown from" Exception stack. Else, don't append anything. > 3. Don't add the firstThrownException to the Error instance. This sounds right to me. There are two things we care about: - The original runtime exception call stack (Error.stack) - The uncaught exception's call stack (Exception stack) if different And this gets us both when handling an uncaught exception. (In reply to Caitlin Potter (:caitp) from comment #28) > I agree with this. The issue is the constraint with the original thrown value > not being retained for logging to the console if the inspector is not open. I think at this point we've all agreed to Option 1. Simply because it will be more useful to users if they open Web Inspector after the exception has already occurred. So now we don't need to worry about keeping extra values around.
Comment on attachment 315679 [details] Patch Attachment 315679 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4136729 New failing tests: http/tests/security/regress-52192.html
Created attachment 315688 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 315679 [details] Patch Attachment 315679 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4136745 New failing tests: http/tests/security/regress-52192.html
Created attachment 315690 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 315679 [details] Patch Attachment 315679 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4136846 New failing tests: http/tests/security/regress-52192.html http/tests/cache/disk-cache/speculative-validation/cacheable-redirect.html
Created attachment 315694 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315679 [details] Patch Attachment 315679 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4136850 New failing tests: http/tests/security/regress-52192.html
Created attachment 315696 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Caitlin, do you want to keep working on this? How will we implement option 1? Will we turn the stack into a string every time we throw? If so, that seems like a bad solution since Filip just went through the effort of us lazily reifying Error's "stack" property. Maybe we can just retain GC roots up to the last N thrown values, where N is small (~10-20), and perhaps we can even just limit it to things that inherit from Error? That way we don't need to reify the stack into a string when throwing/re-throwing.
*** Bug 167934 has been marked as a duplicate of this bug. ***
(In reply to Saam Barati from comment #38) > Caitlin, do you want to keep working on this? I don't intend to. > How will we implement option 1? > > Will we turn the stack into a string every time we throw? If so, that seems > like a bad solution since Filip just went through the effort of us lazily > reifying Error's "stack" property. Maybe we can just retain GC roots up to > the last N thrown values, where N is small (~10-20), and perhaps we can even > just limit it to things that inherit from Error? That way we don't need to > reify the stack into a string when throwing/re-throwing.
This seems to be tracked by this radar: <rdar://30433531>