Bug 28622 - Caught exceptions still pause the debugger
Summary: Caught exceptions still pause the debugger
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Brian Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-21 13:13 PDT by Kevin Lindeman
Modified: 2010-01-22 04:53 PST (History)
5 users (show)

See Also:


Attachments
proposed patch (1.42 KB, patch)
2009-08-21 13:18 PDT, Kevin Lindeman
abarth: review-
Details | Formatted Diff | Diff
[PATCH] Fix (4.22 KB, patch)
2010-01-19 17:29 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception (7.70 KB, patch)
2010-01-19 18:49 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Tri-State Pause on Exceptions Button (58.99 KB, patch)
2010-01-20 14:47 PST, Brian Weinstein
timothy: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Tri-State Button + Tim's Comments (59.48 KB, patch)
2010-01-20 17:15 PST, Brian Weinstein
timothy: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Cleaned up toggled/state Getter/Setter code (59.96 KB, patch)
2010-01-20 17:40 PST, Brian Weinstein
timothy: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Lindeman 2009-08-21 13:13:21 PDT
If you are debugging JavaScript using the web inspector that would cause an exception to happen and you have pause on exceptions enabled, but it is within a try/catch block, the debugger still pauses on it.

To be more in line with other debuggers (including the Dashcode debugger), the web inspector should not pause on an exception if it is caught.
Comment 1 Kevin Lindeman 2009-08-21 13:18:55 PDT
Created attachment 38390 [details]
proposed patch

This patch gets us the proposed behavior. It waits to see until function exit if an exception still remains - one won't remain if we caught it.

I am not sure what the exception() callback should do if we don't pause anymore.
Comment 2 Adam Barth 2009-09-01 16:27:02 PDT
Comment on attachment 38390 [details]
proposed patch

No ChagneLog.  No test.
Comment 3 Timothy Hatcher 2009-09-11 20:27:29 PDT
This is a good idea. Would be great to land this with a ChangeLog and a manual test in WebCore/manual-tests/inspector.
Comment 4 Kevin Lindeman 2009-09-12 16:26:15 PDT
Alright, I will work on that.
Comment 5 Brian Weinstein 2010-01-19 17:29:24 PST
Created attachment 46965 [details]
[PATCH] Fix

Don't break on exceptions that are caught in the same function.
Comment 6 Oliver Hunt 2010-01-19 17:32:42 PST
I disagree, in all debuggers for other languages, pause on exceptions means pause on all exceptions, regardless as to whether there is a handler.  Pause on exceptions is used specifically for the case where you want to pause on an exception they may actually be caught.
Comment 7 Oliver Hunt 2010-01-19 17:34:03 PST
Rather than looking at things like dashcode, people should be looking at real debuggers such as VS, Eclipse, etc, where (if memory serves) th debugger stops on uncaught exceptions by default, and pause on exceptions actually means pause on all exceptions whether or not there is a handler.
Comment 8 Timothy Hatcher 2010-01-19 17:40:33 PST
I can se the use of both options.

I think we should have a tri-state button. States being: don't pause on any exceptions, pause on uncaught exceptions, and pause on all exceptions.
Comment 9 Timothy Hatcher 2010-01-19 17:45:35 PST
Comment on attachment 46965 [details]
[PATCH] Fix

This patch as-is will likely change behaviour in Dashcode or other clients. They might rely on the exeption function always being called.

Since those clients are protected from the C++ layer, I propose we pass another bool to the function: hasHandler.

Then the Debugger can decided if it cares about the exception, and it lets up have a setting. It will also prevent breaking other clients. (You will need to update WebScriptDebugger.mm in WebKit to prevent breaking the build when you add the new argument.)
Comment 10 Brian Weinstein 2010-01-19 18:49:21 PST
Created attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception

This patch has no change in behavior, but enables us to differentiate between Break on All Exceptions and Break on Uncaught Exceptions.
Comment 11 Geoffrey Garen 2010-01-19 18:56:38 PST
Comment on attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception

I think you need to "UNUSED_PARAM(hasHandler);" in WebScriptDebugger too. The rest of the code looks correct, though.

FWIW, I agree with Oliver about this feature: DashCode's behavior is odd. Maybe we could add a tri-state button. But have any real users requested one?
Comment 12 Timothy Hatcher 2010-01-19 18:58:41 PST
Comment on attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception

You don't need UNUSED_PARAM in WebKit.
Comment 13 Eric Seidel (no email) 2010-01-19 23:40:58 PST
Either this or bug 33820 seems to have broken a webgl test.  At least thats' what the bots say:
http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53517%20(9580)/fast/canvas/webgl/texImage2DImageDataTest-pretty-diff.html

The first failure started between r53514 and r53517 inclusive.
Comment 14 Timothy Hatcher 2010-01-19 23:54:02 PST
I highly doubt it was this change, which has nothing to do with layout or rendering.
Comment 15 Brian Weinstein 2010-01-19 23:58:42 PST
(In reply to comment #13)
> Either this or bug 33820 seems to have broken a webgl test.  At least thats'
> what the bots say:
> http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53517%20(9580)/fast/canvas/webgl/texImage2DImageDataTest-pretty-diff.html
> 
> The first failure started between r53514 and r53517 inclusive.

This added a parameter that is unused. It wouldn't have broken any tests I don't think.
Comment 16 Brian Weinstein 2010-01-20 00:06:35 PST
Comment on attachment 46967 [details]
[PATCH] Add a hasHandler parameter to JavaScriptDebugServer::exception

Landed in r53516.
Comment 17 Brian Weinstein 2010-01-20 14:47:34 PST
Created attachment 47064 [details]
[PATCH] Tri-State Pause on Exceptions Button
Comment 18 Timothy Hatcher 2010-01-20 16:39:29 PST
Comment on attachment 47064 [details]
[PATCH] Tri-State Pause on Exceptions Button


> -    bool pauseOnExceptions();
> -    void setPauseOnExceptions(bool pause);
> +    int pauseOnExceptions();
> +    void setPauseOnExceptions(int pause);

I think we should add "State" to these function names. setPauseOnExceptionsState. Why an int here and not long? When the idl uses long.


> -    , m_pauseOnExceptions(false)
> +    , m_pauseOnExceptions(DontPauseOnExceptions)

Add "State" to m_pauseOnExceptions: m_pauseOnExceptionsState.


> +        enum PauseOnExceptionsCondition {
> +            DontPauseOnExceptions,
> +            PauseOnAllExceptions,
> +            PauseOnUncaughtExceptions
> +        };

I think State is better than Condition.

> +        PauseOnExceptionsCondition pauseOnExceptions() const { return m_pauseOnExceptions; }
> +        void setPauseOnExceptions(PauseOnExceptionsCondition);

Add State to the function names.


> +WebInspector.ScriptsPanel.PauseOnExceptionsCondition = {

State.


> +            this.pauseOnExceptionButton.title = WebInspector.UIString("Don't pause on exceptions / Click to Pause on all exceptions.");
> +        else if (InspectorBackend.pauseOnExceptions() == WebInspector.ScriptsPanel.PauseOnExceptionsCondition.PauseOnAllExceptions)
> +            this.pauseOnExceptionButton.title = WebInspector.UIString("Pause on all exceptions  / Click to Pause on uncaught exceptions.");
> +        else if (InspectorBackend.pauseOnExceptions() == WebInspector.ScriptsPanel.PauseOnExceptionsCondition.PauseOnUncaughtExceptions)
> +            this.pauseOnExceptionButton.title = WebInspector.UIString("Pause on uncaught exceptions / Click to Not pause on exceptions.");

Replace the "/" with a \n. I meant to tell you that on IRC.

> +    this.numStates = numStates;
> +    if (!numStates)
> +        this.numStates = 2;

I don't think "num" helps. Just call it "states".

> +    if (numStates == 2)
> +        this._toggled = false;
> +    else
> +        this._toggled = 0;

I think _state would be a better name for _toggled now. 

Overall looks good. Attach another patch if you want me to look over any tweaks.
Comment 19 Brian Weinstein 2010-01-20 17:15:31 PST
Created attachment 47083 [details]
[PATCH] Tri-State Button + Tim's Comments

This renames the functions to pauseOnExceptionsState, and takes care of other feedback from Tim. Also waiting on feedback from Pavel before landing.
Comment 20 Timothy Hatcher 2010-01-20 17:19:33 PST
Comment on attachment 47083 [details]
[PATCH] Tri-State Button + Tim's Comments

>      get toggled()
>      set toggled(x)

You should renamed these too. But that will touch more files. Maybe add a get/set state, and make toggled use that only for states == 2? And throw otherwise.
Comment 21 Brian Weinstein 2010-01-20 17:40:17 PST
Created attachment 47085 [details]
[PATCH] Cleaned up toggled/state Getter/Setter code

Should be a final pass, waiting for Pavel to take a look before landing. Sorry about the numerous iterations.
Comment 22 Timothy Hatcher 2010-01-20 17:42:07 PST
Comment on attachment 47085 [details]
[PATCH] Cleaned up toggled/state Getter/Setter code

> +            throw("Only used toggled when there are 2 states, otherwise, use state");
> +            throw("Only used toggled when there are 2 states, otherwise, use state");

Typo: "used".
Comment 23 Brian Weinstein 2010-01-20 17:43:27 PST
(In reply to comment #22)
> (From update of attachment 47085 [details])
> > +            throw("Only used toggled when there are 2 states, otherwise, use state");
> > +            throw("Only used toggled when there are 2 states, otherwise, use state");
> 
> Typo: "used".

Fixed. Thanks!
Comment 24 Pavel Feldman 2010-01-20 22:26:11 PST
(In reply to comment #21)
> Created an attachment (id=47085) [details]
> [PATCH] Cleaned up toggled/state Getter/Setter code
> 
> Should be a final pass, waiting for Pavel to take a look before landing. Sorry
> about the numerous iterations.

We are now fixing Chromium so that it works with this patch. Will land this one shortly!
Comment 25 Brian Weinstein 2010-01-20 22:59:08 PST
(In reply to comment #24)
> (In reply to comment #21)
> > Created an attachment (id=47085) [details] [details]
> > [PATCH] Cleaned up toggled/state Getter/Setter code
> > 
> > Should be a final pass, waiting for Pavel to take a look before landing. Sorry
> > about the numerous iterations.
> 
> We are now fixing Chromium so that it works with this patch. Will land this one
> shortly!

Let me know when you land it, I can land my part when it's ready. Thanks for following up on this.
Comment 26 Yury Semikhatsky 2010-01-22 04:53:18 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/JavaScriptDebugServer.cpp
	M	WebCore/inspector/JavaScriptDebugServer.h
	M	WebCore/inspector/front-end/InspectorBackendStub.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/StatusBarButton.js
	M	WebCore/inspector/front-end/inspector.css
Committed r53696