Bug 120460 – Web Inspector: exceptions triggered from console evaluation do not pause the debugger

• Component: Web Inspector
• Status: RESOLVED FIXED

[Antoine Quint]

Created attachment 209963 [details]
Test case

Steps to reproduce:

1. open attached test case
2. open the inspector
3. ensure you have the inspector set up to break on all exceptions
4. jump to the console
5. type "foo()" and return

Actual results: an exception is logged to the console

Expected results: the debugger pauses and lets you debug the error, then the exception is logged to the console once the debugging session is over.

[Radar WebKit Bug Importer]

<rdar://problem/14866489>

[Antoine Quint]

So the quick fix is to change the call to _evaluateInInspectedWindow() in JavaScriptLogViewController.consolePromptTextCommitted() to set the doNotPauseOnExceptionsAndMuteConsole parameter to false, but this also shows Web Inspector UI code in the stack, which shouldn't show. However, this behaviour matches the Chrome behaviour.

[Antoine Quint]

Can we assume that a call frame that has no URL in its sourceCodeLocation should be excluded from the stack shown in the debugger panel?

[Antoine Quint]

Created attachment 209979 [details]
Patch

[Timothy Hatcher]

Comment on attachment 209979 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:346
> +            if (!sourceCodeLocation || !sourceCodeLocation._sourceCode._url)

This is not correct. A page script can not have a URL if it is an eval() and those still should be shown as anon scripts. We could still check for Inspector scripts by adding a "// #sourceURL=__WebInspectorConsole__" to the user typed strings to evaluate in InjectedScriptSource.js in WebCore or maybe even from the front-end. Then look for that URL and block it.

> Source/WebInspectorUI/UserInterface/DebuggerSidebarPanel.js:340
> +        // It is useful to turn off the step out button when there is no call frame to go through
> +        // since there might be call frames in the backend that we elected to remove in the frontend.

Where are call frames removed?

[Antoine Quint]

(In reply to comment #5)
> (From update of attachment 209979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209979&action=review
> 
> > Source/WebInspectorUI/UserInterface/DebuggerManager.js:346
> > +            if (!sourceCodeLocation || !sourceCodeLocation._sourceCode._url)
> 
> This is not correct. A page script can not have a URL if it is an eval() and those still should be shown as anon scripts. We could still check for Inspector scripts by adding a "// #sourceURL=__WebInspectorConsole__" to the user typed strings to evaluate in InjectedScriptSource.js in WebCore or maybe even from the front-end. Then look for that URL and block it.

Cool, will try this.

> > Source/WebInspectorUI/UserInterface/DebuggerSidebarPanel.js:340
> > +        // It is useful to turn off the step out button when there is no call frame to go through
> > +        // since there might be call frames in the backend that we elected to remove in the frontend.
> 
> Where are call frames removed?

In the code you mentioned above where we used to ignore call frames from the payload if they didn't have a URL.

[Antoine Quint]

Created attachment 210088 [details]
Patch

[Build Bot]

Comment on attachment 210088 [details]
Patch

Attachment 210088 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1650081

New failing tests:
inspector/console/command-line-api.html

[Build Bot]

Created attachment 210096 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Antoine Quint]

Created attachment 210099 [details]
Patch

[Darin Adler]

Comment on attachment 210099 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:347
> +            // Exclude the case where the call frame is in the inspector code.
> +            if (!sourceCodeLocation || sourceCodeLocation._sourceCode._url.indexOf("__WebInspector") === 0)
>                  continue;

Is there a more reliable way of doing this check? It seems annoying that this could be fooled by a strange URL in the target code.

> Source/WebInspectorUI/UserInterface/JavaScriptLogViewController.js:235
> +        text = "//@ sourceURL=__WebInspectorConsole__\n" + text;

If the point of this is to add something the other code can find, then I suggest adding a brief “why we are doing this” comment here so someone won’t just remove this later.

[Antoine Quint]

(In reply to comment #11)
> (From update of attachment 210099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210099&action=review
> 
> > Source/WebInspectorUI/UserInterface/DebuggerManager.js:347
> > +            // Exclude the case where the call frame is in the inspector code.
> > +            if (!sourceCodeLocation || sourceCodeLocation._sourceCode._url.indexOf("__WebInspector") === 0)
> >                  continue;
> 
> Is there a more reliable way of doing this check? It seems annoying that this could be fooled by a strange URL in the target code.

I don't know of a way, maybe Tim or Joe knows of one though. I'll hold off for a bit before landing to give them an opportunity to chime in.

> > Source/WebInspectorUI/UserInterface/JavaScriptLogViewController.js:235
> > +        text = "//@ sourceURL=__WebInspectorConsole__\n" + text;
> 
> If the point of this is to add something the other code can find, then I suggest adding a brief “why we are doing this” comment here so someone won’t just remove this later.

That's true, I'll add a comment to that effect.

[Timothy Hatcher]

Comment on attachment 210099 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/DebuggerManager.js:347
>>                  continue;
> 
> Is there a more reliable way of doing this check? It seems annoying that this could be fooled by a strange URL in the target code.

The fact that it requires the URL to be prefixed with "__WebInspector" is fairly safe. Most URLs will start with a scheme like "http://". Someone could also use "//# sourceURL=__WebInspectorFoo__" in their page to hide their functions. I'm not sure that is much of a problem.

Otherwise, a Web Inspector console evaluation and a page using eval() are indistinguishable.

>> Source/WebInspectorUI/UserInterface/JavaScriptLogViewController.js:235
>> +        text = "//@ sourceURL=__WebInspectorConsole__\n" + text;
> 
> If the point of this is to add something the other code can find, then I suggest adding a brief “why we are doing this” comment here so someone won’t just remove this later.

This should be "//# sourceURL", which is the newest syntax. I would also put it after the user typed text, on a new line. That way the first line of the script is still the user code and not our code.

[Timothy Hatcher]

Comment on attachment 210099 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/JavaScriptLogViewController.js:235
>>>> +        text = "//@ sourceURL=__WebInspectorConsole__\n" + text;
>>> 
>>> If the point of this is to add something the other code can find, then I suggest adding a brief “why we are doing this” comment here so someone won’t just remove this later.
>> 
>> That's true, I'll add a comment to that effect.
> 
> This should be "//# sourceURL", which is the newest syntax. I would also put it after the user typed text, on a new line. That way the first line of the script is still the user code and not our code.

You will also need to hide these scripts from the Resources sidebar. By giving them a sourceURL they now show up in the Extra Scripts folder that is reserved for page scripts using sourceURL. You would add a check in ResourcesSidebarPanel.prototype._scriptWasAdded.

[Timothy Hatcher]

Created attachment 210128 [details]
Issue in the Sidebar

[Timothy Hatcher]

Comment on attachment 210099 [details]
Patch

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

>>>>> Source/WebInspectorUI/UserInterface/JavaScriptLogViewController.js:235
>>>>> +        text = "//@ sourceURL=__WebInspectorConsole__\n" + text;
>>>> 
>>>> If the point of this is to add something the other code can find, then I suggest adding a brief “why we are doing this” comment here so someone won’t just remove this later.
>>> 
>>> That's true, I'll add a comment to that effect.
>> 
>> This should be "//# sourceURL", which is the newest syntax. I would also put it after the user typed text, on a new line. That way the first line of the script is still the user code and not our code.
> 
> You will also need to hide these scripts from the Resources sidebar. By giving them a sourceURL they now show up in the Extra Scripts folder that is reserved for page scripts using sourceURL. You would add a check in ResourcesSidebarPanel.prototype._scriptWasAdded.

Putting our //# sourceURL last also lets the user explicitly type their own //# sourceURL, which would win and cause it to show in the sidebar. That would be a hidden tick to get the script visible so breakpoints could be added if they are defining a complex function they want to step through later. Using the "debugger" keyword or throwing exceptions still can cause these console scripts to show up in the UI, so we should prettify/replace the "__WebInspectorConsole__" with a more user friendly strings when it gets shown in the sidebar or navigation bar.

[Timothy Hatcher]

The more that I think about it the more I think this will be annoying. If you evaluate something in the console and it causes an error, I would not want to jump out and show that source in the content browser and pause in the debugger. Often I cause exceptions in the console by simple typos or just general fumbling that is obvious and easy to fix. Pausing in the debugger for those simple issues would be a big hassle and was the original motivation for why the console ignores exceptions.

[Antoine Quint]

(In reply to comment #17)
> The more that I think about it the more I think this will be annoying. If you evaluate something in the console and it causes an error, I would not want to jump out and show that source in the content browser and pause in the debugger. Often I cause exceptions in the console by simple typos or just general fumbling that is obvious and easy to fix. Pausing in the debugger for those simple issues would be a big hassle and was the original motivation for why the console ignores exceptions.

I think wherever the code comes from we should be respecting the All Exceptions breakpoint. I really think we should be fixing this to match the Chrome behaviour (with our enhancements in terms of ignoring the WebInspector injected code).

[Timothy Hatcher]

(In reply to comment #18)
> (In reply to comment #17)
> > The more that I think about it the more I think this will be annoying. If you evaluate something in the console and it causes an error, I would not want to jump out and show that source in the content browser and pause in the debugger. Often I cause exceptions in the console by simple typos or just general fumbling that is obvious and easy to fix. Pausing in the debugger for those simple issues would be a big hassle and was the original motivation for why the console ignores exceptions.
> 
> I think wherever the code comes from we should be respecting the All Exceptions breakpoint. I really think we should be fixing this to match the Chrome behaviour (with our enhancements in terms of ignoring the WebInspector injected code).

In principle I agree. Though in practice I still think it would be annoying. Making it better than Chrome is defiantly the right direction.

Joe and I talked about this briefly on IRC and think an approach where top level/program code errors/exceptions don't break in the debugger would be best (e.g. "foo.bar()" where bar does no exist). If the console calls a function that throws an exception, then it would stop in the debugger. That approach would keep the console approachable for easy hacking and allow for simple mistakes at the top level. It would still do the right thing if the error happened in a deeper function. But debugging program code for the console prompt is not helpful and just gets in the way.

Making this work likely requires changes in WebCore::ScriptDebugger or JSC.

[Antoine Quint]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > The more that I think about it the more I think this will be annoying. If you evaluate something in the console and it causes an error, I would not want to jump out and show that source in the content browser and pause in the debugger. Often I cause exceptions in the console by simple typos or just general fumbling that is obvious and easy to fix. Pausing in the debugger for those simple issues would be a big hassle and was the original motivation for why the console ignores exceptions.
> > 
> > I think wherever the code comes from we should be respecting the All Exceptions breakpoint. I really think we should be fixing this to match the Chrome behaviour (with our enhancements in terms of ignoring the WebInspector injected code).
> 
> In principle I agree. Though in practice I still think it would be annoying. Making it better than Chrome is defiantly the right direction.
> 
> Joe and I talked about this briefly on IRC and think an approach where top level/program code errors/exceptions don't break in the debugger would be best (e.g. "foo.bar()" where bar does no exist). If the console calls a function that throws an exception, then it would stop in the debugger. That approach would keep the console approachable for easy hacking and allow for simple mistakes at the top level. It would still do the right thing if the error happened in a deeper function. But debugging program code for the console prompt is not helpful and just gets in the way.
> 
> Making this work likely requires changes in WebCore::ScriptDebugger or JSC.

Couldn't we technically make this work all in the frontend by checking the depth of the call stack when the debugger is paused as a result of evaluating code from the console and exiting the debugger if we find we have a single call frame in the filtered stack?

[Antoine Quint]

(In reply to comment #20)
> (In reply to comment #19)
> > Joe and I talked about this briefly on IRC and think an approach where top level/program code errors/exceptions don't break in the debugger would be best (e.g. "foo.bar()" where bar does no exist). If the console calls a function that throws an exception, then it would stop in the debugger. That approach would keep the console approachable for easy hacking and allow for simple mistakes at the top level. It would still do the right thing if the error happened in a deeper function. But debugging program code for the console prompt is not helpful and just gets in the way.
> > 
> > Making this work likely requires changes in WebCore::ScriptDebugger or JSC.
> 
> Couldn't we technically make this work all in the frontend by checking the depth of the call stack when the debugger is paused as a result of evaluating code from the console and exiting the debugger if we find we have a single call frame in the filtered stack?

So we definitely can do that, I have a working example and I think it's a reasonable approach. I have one last question: if the error is not in the console prompt, should we show the console prompt frame in the stack or not?

[Timothy Hatcher]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Joe and I talked about this briefly on IRC and think an approach where top level/program code errors/exceptions don't break in the debugger would be best (e.g. "foo.bar()" where bar does no exist). If the console calls a function that throws an exception, then it would stop in the debugger. That approach would keep the console approachable for easy hacking and allow for simple mistakes at the top level. It would still do the right thing if the error happened in a deeper function. But debugging program code for the console prompt is not helpful and just gets in the way.
> > > 
> > > Making this work likely requires changes in WebCore::ScriptDebugger or JSC.
> > 
> > Couldn't we technically make this work all in the frontend by checking the depth of the call stack when the debugger is paused as a result of evaluating code from the console and exiting the debugger if we find we have a single call frame in the filtered stack?
> 
> So we definitely can do that, I have a working example and I think it's a reasonable approach. I have one last question: if the error is not in the console prompt, should we show the console prompt frame in the stack or not?

I think we should hide the console script and call frame in the UI if we don't need to show it. If we do need to show it (debugger statement) then it should show up with a better display name than "__WebInspectorConsole__". Basically it should not break in the debugger if the top call frame is in "__WebInspectorConsole__".

[Timothy Hatcher]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > Joe and I talked about this briefly on IRC and think an approach where top level/program code errors/exceptions don't break in the debugger would be best (e.g. "foo.bar()" where bar does no exist). If the console calls a function that throws an exception, then it would stop in the debugger. That approach would keep the console approachable for easy hacking and allow for simple mistakes at the top level. It would still do the right thing if the error happened in a deeper function. But debugging program code for the console prompt is not helpful and just gets in the way.
> > > > 
> > > > Making this work likely requires changes in WebCore::ScriptDebugger or JSC.
> > > 
> > > Couldn't we technically make this work all in the frontend by checking the depth of the call stack when the debugger is paused as a result of evaluating code from the console and exiting the debugger if we find we have a single call frame in the filtered stack?
> > 
> > So we definitely can do that, I have a working example and I think it's a reasonable approach. I have one last question: if the error is not in the console prompt, should we show the console prompt frame in the stack or not?
> 
> I think we should hide the console script and call frame in the UI if we don't need to show it. If we do need to show it (debugger statement) then it should show up with a better display name than "__WebInspectorConsole__". Basically it should not break in the debugger if the top call frame is in "__WebInspectorConsole__".

But that would even disable the debugger statement, which I don't think is a loss.

[Antoine Quint]

Created attachment 210387 [details]
Patch

[Antoine Quint]

With the latest patch, we don't pause in the debugger if the exception is in the console prompt code itself, and when we pause for exceptions in call frames under the console prompt, we elegantly label the Console Prompt call frame as such.

[Antoine Quint]

Created attachment 210388 [details]
Debugger when paused in code running from the console prompt.

[Timothy Hatcher]

Comment on attachment 210387 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/CallFrameTreeElement.js:43
> +        title = WebInspector.UIString("Console Prompt");

You will still see "__WebInspectorConsole__" in the navigation bar and the Resour

> Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:686
> +        // Exclude inspector scripts.
> +        if (script.url.indexOf("__WebInspector") === 0)
> +            return;

This might cause issues if you select the "Console Prompt" call frame. We will try to show the script, which might work, but the navigation bar will be blank because it wasn't added to the sidebar. We might need to just hide the "__WebInspectorConsole__" frame in the call stack and prevent showing the source.

[Antoine Quint]

(In reply to comment #27)
> (From update of attachment 210387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210387&action=review
> 
> > Source/WebInspectorUI/UserInterface/CallFrameTreeElement.js:43
> > +        title = WebInspector.UIString("Console Prompt");
> 
> You will still see "__WebInspectorConsole__" in the navigation bar and the Resour

"Resource bar"? Not there, we filter it out. It also doesn't show in the navigation bar.

> > Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:686
> > +        // Exclude inspector scripts.
> > +        if (script.url.indexOf("__WebInspector") === 0)
> > +            return;
> 
> This might cause issues if you select the "Console Prompt" call frame. We will try to show the script, which might work, but the navigation bar will be blank because it wasn't added to the sidebar. We might need to just hide the "__WebInspectorConsole__" frame in the call stack and prevent showing the source.

Nothing shows in the navigation bar, indeed, but I don't think that's a big deal, is it? I think it's nice to have the code still because there may multiple lines in the console prompt and this gives you an idea where the code is coming from.

I don't feel strongly about this though, we could simply never show the Console Prompt call frame.

[Joseph Pecoraro]

Comment on attachment 210387 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/CallFrameTreeElement.js:43
>>> +        title = WebInspector.UIString("Console Prompt");
>> 
>> You will still see "__WebInspectorConsole__" in the navigation bar and the Resour
> 
> "Resource bar"? Not there, we filter it out. It also doesn't show in the navigation bar.

How about "Web Inspector Console" instead of "Console Prompt".

[Timothy Hatcher]

Comment on attachment 210387 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:686
>>> +            return;
>> 
>> This might cause issues if you select the "Console Prompt" call frame. We will try to show the script, which might work, but the navigation bar will be blank because it wasn't added to the sidebar. We might need to just hide the "__WebInspectorConsole__" frame in the call stack and prevent showing the source.
> 
> Nothing shows in the navigation bar, indeed, but I don't think that's a big deal, is it? I think it's nice to have the code still because there may multiple lines in the console prompt and this gives you an idea where the code is coming from.
> 
> I don't feel strongly about this though, we could simply never show the Console Prompt call frame.

The navigation bar should never be empty if there is a content view showing. I just wouldn't show the call frame. The user should know where the call came from if they just typed it.

[Antoine Quint]

(In reply to comment #30)
> (From update of attachment 210387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210387&action=review
> 
> >>> Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:686
> >>> +            return;
> >> 
> >> This might cause issues if you select the "Console Prompt" call frame. We will try to show the script, which might work, but the navigation bar will be blank because it wasn't added to the sidebar. We might need to just hide the "__WebInspectorConsole__" frame in the call stack and prevent showing the source.
> > 
> > Nothing shows in the navigation bar, indeed, but I don't think that's a big deal, is it? I think it's nice to have the code still because there may multiple lines in the console prompt and this gives you an idea where the code is coming from.
> > 
> > I don't feel strongly about this though, we could simply never show the Console Prompt call frame.
> 
> The navigation bar should never be empty if there is a content view showing. I just wouldn't show the call frame. The user should know where the call came from if they just typed it.

That works for me, I'll just remove the Console Prompt call frame in all situations.

[Antoine Quint]

Created attachment 210400 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 210400 [details]
Patch

Clearing flags on attachment: 210400

Committed r154998: <http://trac.webkit.org/changeset/154998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 210400 [details]
Patch

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

Drive by comments after this already landed.

> Source/WebInspectorUI/UserInterface/DebuggerManager.js:346
> +            if (!sourceCodeLocation || sourceCodeLocation._sourceCode._url.indexOf("__WebInspector") === 0)

indexOf === 0 => String.prototype.startsWith

> Source/WebInspectorUI/UserInterface/ResourceSidebarPanel.js:685
> +        if (script.url.indexOf("__WebInspector") === 0)

indexOf === 0 => String.prototype.startsWith

"startsWith" has a clever way of using lastIndexOf to only check the start of a string, instead of indexOf === 0 which will check the entire string, unnecessarily checking past the first character.